[systemd-devel] [RFC][PATCH] sd-bus: split ref-counting of queues from ref-counting of the rest of the sd-bus object

[Tom Gundersen]

On Sun, Mar 23, 2014 at 11:50 PM, David Herrmann <dh.herrmann at gmail.com> wrote:
> On Sun, Mar 23, 2014 at 4:49 PM, Tom Gundersen <teg at jklm.no> wrote:
>> Introduce a new ref-count, n_ref_queues, which only protects the {r,w}queue of
>> a bus, and introduce bus_{un,}ref(), which are only available internally, and
>> which do not protect these queues.
>>
>> Make sure that sd_bus_message object do not call sd_bus_ref(), but only the
>> internal bus_ref(). This is ok as the {r,w}queues should never be accessed via
>> the message object (as doing so would anyway not be thread-safe).
>>
>> When the refcount on the queues reaches zero (even thought the refcount on the
>> bus itself may not, due to references held by messages in the queues), the
>> queues are flushed and their messages unref'ed. This avoids problems due to
>> mutual references between busses and their queued messages. In particular we
>> don't get an sd_bus_unref() -> sd_bus_message_unref() -> sd_bus_unref()
>> call-chain.
>>
>> Moreover, we can now enforce that sd_bus_{un,}ref() is only ever called from
>> the same thread as created the bus (whereas bus_unref() may be called from a
>> different thread, as part of unref'ing a message being handled in a worker
>> thread).
>
> Code looks good and it should work this way. But as I said earlier,
> I'd prefer if we could avoid dual-refcounts and instead take the refs
> on the object we _actually_ want. Messages don't need the ->bus
> pointer at all except for memfds, as far as I can see. So instead of
> introducing a circular dependency bus->m->bus, why not take a
> reference to the kdbus FD instead? Either by dup()ing the fd or
> extracting it to a separate struct/refcount. We can still take an
> sd_bus argument in message allocations, but just avoid ref'ing them.

Yeah, if that is likely to remain the only use of m->bus, then just
grabbing the memfd directly makes more sense to me too... (and then
just drop all the atomic reference counting from sd_bus to make it
clearer that only sd_bus_message objects may be passed around to
different threads).

-t