Hi Lennart, Thanks a lot for your elaborate reply. See inline replies...
On Tue, Mar 5, 2019 at 11:27 AM Lennart Poettering <lenn...@poettering.net> wrote: > > On Mo, 04.03.19 21:56, Stanislav Angelovič (angelovi...@gmail.com) wrote: > > > sd-bus doesn't natively care for threads. However it's written in kind > of a threads-aware style, i.e. it keeps only local state, no global > variables. This means you can do locking around it yourself and thus > make it usable from multiple threads. Good design, IMO. > > 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. > 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? > > 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... > > > 1. In sd_bus_message_handler, create a deep copy of the method call > > sd_bus_message via sd_bus_message_copy(). And pass a pointer to this copy > > to a worker thread. This is very straight-forward and simple, it solves the > > race condition, but introduces a performance cost of copying (I don't know > > how big this cost is, perhaps it's rather negligible). > > 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. Creating a message copy (to which sd-bus no more keeps reference, just we) and then passing this copy to a different thread seems a solution to me. With one drawback -- making the copy. > > > 3. Solution on sd-bus side :-) which is to make sd_bus_message ref count > > handling thread-safe (atomic), just like one for sd_bus. This avoids the > > need for deep copy. What do you think? Anyway, we'd still have to come up > > with a solution now, based on already releases sd-bus versions. > > Given the fact that DBus as it stands now isn't really meant for > processing msgs in worker thread pools I don't think we want to add > more thread support to sd-bus. Doing worker thread pools for DBus can > work in special cases but not in the general case (example: if you > create an object in another service dbus gives you the guarantee that > the method reply and the signal announcing the object arrive in a > specific order, which is a property that cannot be held up if worker > thread pools are used), hence I am very conservative with supporting > this beyond how we already do (i.e that we maintain no global state, > and thus allow you to apply locking yourself) I understand your stance with throwing more threading support in sd-bus, and agree with it. 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. Having atomic message ref counting would open the doors for sd-bus users to "move" (not copy) messages from method callbacks to different threads, since, as mentioned above, throwing in locks on user's side is anyway not sufficient. > > Lennart > > -- > Lennart Poettering, Red Hat
_______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/systemd-devel
