[systemd-devel] [RFC][PATCH] sd-bus: split ref-counting of queues from ref-counting of the rest of the sd-bus object
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).
---
src/libsystemd/sd-bus/bus-internal.h | 10
src/libsystemd/sd-bus/bus-message.c | 6 +--
src/libsystemd/sd-bus/sd-bus.c | 93 +++-
3 files changed, 51 insertions(+), 58 deletions(-)
diff --git a/src/libsystemd/sd-bus/bus-internal.h
b/src/libsystemd/sd-bus/bus-internal.h
index 3dceb8a..c9b40af 100644
--- a/src/libsystemd/sd-bus/bus-internal.h
+++ b/src/libsystemd/sd-bus/bus-internal.h
@@ -145,6 +145,13 @@ struct sd_bus {
same time. */
RefCount n_ref;
+/* The {r,w}queue may only be accessed from the original
+ thread, so no need for atomic ref counting. The queues
+ are ref-counted separately from the sd_bus object to
+ avoid problems caused by mutual references between
+ busses and their queued messages */
+unsigned n_ref_queues;
+
enum bus_state state;
int input_fd, output_fd;
int message_version;
@@ -340,3 +347,6 @@ int bus_set_address_system(sd_bus *bus);
int bus_set_address_user(sd_bus *bus);
int bus_set_address_system_remote(sd_bus *b, const char *host);
int bus_set_address_system_container(sd_bus *b, const char *machine);
+
+sd_bus *bus_ref(sd_bus* bus);
+sd_bus *bus_unref(sd_bus *bus);
diff --git a/src/libsystemd/sd-bus/bus-message.c
b/src/libsystemd/sd-bus/bus-message.c
index 4fcc693..e33c08a 100644
--- a/src/libsystemd/sd-bus/bus-message.c
+++ b/src/libsystemd/sd-bus/bus-message.c
@@ -137,7 +137,7 @@ static void message_free(sd_bus_message *m) {
}
if (m-bus)
-sd_bus_unref(m-bus);
+bus_unref(m-bus);
if (m-free_fds) {
close_many(m-fds, m-n_fds);
@@ -427,7 +427,7 @@ int bus_message_from_header(
}
if (bus)
-m-bus = sd_bus_ref(bus);
+m-bus = bus_ref(bus);
*ret = m;
return 0;
@@ -502,7 +502,7 @@ static sd_bus_message *message_new(sd_bus *bus, uint8_t
type) {
m-root_container.need_offsets = BUS_MESSAGE_IS_GVARIANT(m);
if (bus)
-m-bus = sd_bus_ref(bus);
+m-bus = bus_ref(bus);
return m;
}
diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c
index bbe61a6..b09326d 100644
--- a/src/libsystemd/sd-bus/sd-bus.c
+++ b/src/libsystemd/sd-bus/sd-bus.c
@@ -111,12 +111,6 @@ static void bus_node_destroy(sd_bus *b, struct node *n) {
static void bus_reset_queues(sd_bus *b) {
assert(b);
-/* NOTE: We _must_ decrement b-Xqueue_size before calling
- * sd_bus_message_unref() for _each_ message. Otherwise the
- * self-reference checks in sd_bus_unref() will fire for each message.
- * We would thus recurse into sd_bus_message_unref() and trigger the
- * assert(m-n_ref 0) */
-
while (b-rqueue_size 0)
sd_bus_message_unref(b-rqueue[--b-rqueue_size]);
@@ -203,6 +197,7 @@ _public_ int sd_bus_new(sd_bus **ret) {
return -ENOMEM;
r-n_ref = REFCNT_INIT;
+r-n_ref_queues = 1;
r-input_fd = r-output_fd = -1;
r-message_version = 1;
r-creds_mask |=
SD_BUS_CREDS_WELL_KNOWN_NAMES|SD_BUS_CREDS_UNIQUE_NAME;
@@ -1380,72 +1375,60 @@ static void bus_enter_closing(sd_bus *bus) {
bus-state = BUS_CLOSING;
}
-_public_ sd_bus *sd_bus_ref(sd_bus *bus) {
-assert_return(bus, NULL);
+/* Unlike sd_bus_{un,}ref(), bus_{un,}ref() do not protect the
+ {r,w}queue. On the other hand, these methods are thread-safe.
+ */
+sd_bus *bus_ref(sd_bus* bus) {
+assert(bus);
assert_se(REFCNT_INC(bus-n_ref) = 2);
return bus;
}
-_public_ sd_bus *sd_bus_unref(sd_bus *bus) {
+sd_bus *bus_unref(sd_bus *bus) {
unsigned i;
if (!bus)
return NULL;
-/* TODO/FIXME: It's naive to think REFCNT_GET() is thread-safe in any
-
Re: [systemd-devel] [RFC][PATCH] sd-bus: split ref-counting of queues from ref-counting of the rest of the sd-bus object
Hi
On Sun, Mar 23, 2014 at 4:49 PM, Tom Gundersen t...@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.
With the current approach, m-bus becomes a pseudo reference as it's
not equivalent to real references. If we forced messages to be
referenced on the bus they're queued on, we could fix this more
easily, but we don't have this requirement. Therefore, I really wonder
whether keeping m-bus is making anyone happy?
Anyhow, I wanna hear first what Lennart had planned with m-bus..
maybe he can shed some light on this.
Thanks
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC][PATCH] sd-bus: split ref-counting of queues from ref-counting of the rest of the sd-bus object
On Sun, Mar 23, 2014 at 11:50 PM, David Herrmann dh.herrm...@gmail.com wrote:
On Sun, Mar 23, 2014 at 4:49 PM, Tom Gundersen t...@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
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
