[systemd-devel] sd-bus: serving method call message in a separate thread

[Lennart Poettering]

On Di, 05.03.19 20:46, Stanislav Angelovič (angelovic.s at gmail.com) wrote:

> > To do that, allocate a single mutex of some form covering the sd_bus
> > object and all sd_bus_message associated with it. You need to take the
> > lock when you enter any sd_bus_xyz call, and release it after. To
> > ensure that you can use sd-bus APIs from one thread while another one
> > poll()s on the connection make sure that you do not use sd-bus' own
> > polling (i.e. do not use sd_bus_wait(), since that would mean we'd
> > have to wait for IO with the lock taken). Instead, use your own poll()
> > loop that uses sd_bus_get_fd(), sd_bus_get_events(),
> > sd_bus_get_timeout() to figure out what to wait for, and then unlock
> > the mutex before poll() and lock it again after.
> >
>
> Yes, we have played with an approach along these lines in sdbus-c++
> recently.
>
> But, looking at sd-bus code, perhaps not all operations on both bus and
> messages need a mutual lock. If in our library design, if we ensure that
> the bus is running properly, then it seems we can e.g. write output
> arguments to a method reply message (provided that the message is accessed
> only in this one thread) without locking access to the entire bus and other
> messages.

Well, strictly speaking there are some operations that don't need
locking, but it's not something I think we should commit to
API-wise. We should retain the liberty to change things internally if
need be, and thus require all calls into the library to take the lock
first, even though if — currently strictly speaking — it might not be
necessary in some cases.

sd-bus used to support a kdbus backend once upon a time. On kdbus
message data could be placed in memfds in some cases, and the message
API would hide that internally, maintaining even a cache of reusable
memfd objects per bus, to make this somewhat cheap. Maintaining a
per-connection cache like that always requires proper locking. The
kdbus thing is now in the past, but I am very sure we should keep our
options open there.

> > Note that DBus puts an emphasis on message ordering, which means
> > worker-thread-pool based dispatching is kinda incompatible with what
> > it is built for (as thread pool stuff usually means out-of-order
> > dispatching). Hence, whether doing threaded sd-bus is a good idea is a
> > different question to ask.
>
> The only specific thing in our worker-thread approach here is that when a
> D-Bus object receives a call of its method, it doesn't guarantee that it
> sends the reply before accepting another call of that method. If it
> receives calls c1 and c2 that order, it might e.,g, reply to c2 first, then
> to c1. On the caller side, the caller abstracts away from this and it still
> normally waits for the reply to its issued call. Is that against D-Bus
> message ordering rules?

I am not sure I follow fully, but if I understand correctly you are
suggesting that clients always use sd_bus_call() for doing method
calls, i.e. waiting synchronously for the reply? Many programs don't
work that way: they enqueue a bunch of method calls at once,
asynchronously, expecting a callback when they are done. if these are
now executed out of order you have a problem.

DBus guarantees global ordering across the bus. And this leaks into
client behaviour. Only in very specific cases (i.e. where each method
call is basically "pure", "stateless") it is safe to do out of order
execution.

> > sd-bus itself tries to push people towards a different model btw: that
> > you have a separate bus connection for each thread
> > (i.e. sd_bus_default() and friends tells you the default connection
> > for your specific thread), and that within that thread you have perfect
> > ordering.
>
> I see. However, each connection then must have a different bus name. They
> cannot share the same bus name...

That's correct.

> > It's fine to pass the sd_bus_message to worker threads, as long as you
> > make sure you always have a reference to it. While passing the
> > reference you don't have to lock the message/bus, but when accessing
> > the message you have to take the lock however.
>
> Taking the lock on our side is not sufficient, still. Because sd-bus
> accesses the message, too -- it unrefs it after method callback, without
> taking any lock. So we still have race condition.

As mentioned above, the lock needs to be held as long as you are in
any sd_bus_xyz call, and the lock needs to cover both the sd_bus
connection object and all its bus message objects. in that case what
you are describing is not possible, as the lock will be held whenever
sd-bus code is executed. It won't "unref it after method callback
without taking any lock", because it wouldn't be running without
the lock being held, because you did so before entering sd-bus code.

(BTW, it should be safe to unlock the mutex from callback code, as
long as you take it again when befor returning from callback
code.)

>
> But, on the other hand, just changing the message ref_count++ and
> ref_count-- to atomic operations is truly a simple thing that doesn't
> involve broader change of paradigm. Sd-bus already does it this way -- for
> sd_bus ref count handling.

Coincidentally, we actually dropped the last bits of atomic ref
counting from sd-bus a few days ago:

https://github.com/systemd/systemd/pull/11888

Lennart

--
Lennart Poettering, Red Hat