[Qemu-devel] [PATCH for-2.0 0/2] dataplane: fix internal IOThread name collision

[Qemu-devel] [PATCH for-2.0 0/2] dataplane: fix internal IOThread name collision

[Stefan Hajnoczi]

Christian Borntraeger <borntraeger@de.ibm.com> noticed that the naming of the
internal IOThread objects, which are created when the user does not specify an
IOThread explicitly, can result in collisions.

These patches make the IOThread object nameless so it cannot cause collisions.
This is important when multiple dataplane devices are used simultaneously or
the user happened to choose a colliding name.

Stefan Hajnoczi (2):
  iothread: make IOThread struct definition public
  dataplane: replace iothread object_add() with embedded instance

 hw/block/dataplane/virtio-blk.c | 31 ++++++++++++-------------------
 include/sysemu/iothread.h       | 12 +++++++++++-
 iothread.c                      | 11 -----------
 3 files changed, 23 insertions(+), 31 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH for-2.0 1/2] iothread: make IOThread struct definition public

[Stefan Hajnoczi]

Make the IOThread struct definition public so objects can be embedded in
parent structs.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/sysemu/iothread.h | 12 +++++++++++-
 iothread.c                | 11 -----------
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
index a32214a..7c01a61 100644
--- a/include/sysemu/iothread.h
+++ b/include/sysemu/iothread.h
@@ -15,10 +15,20 @@
 #define IOTHREAD_H
 
 #include "block/aio.h"
+#include "qemu/thread.h"
 
 #define TYPE_IOTHREAD "iothread"
 
-typedef struct IOThread IOThread;
+typedef struct {
+    Object parent_obj;
+
+    QemuThread thread;
+    AioContext *ctx;
+    QemuMutex init_done_lock;
+    QemuCond init_done_cond;    /* is thread initialization done? */
+    bool stopping;
+    int thread_id;
+} IOThread;
 
 #define IOTHREAD(obj) \
    OBJECT_CHECK(IOThread, obj, TYPE_IOTHREAD)
diff --git a/iothread.c b/iothread.c
index cb5986b..1fbf9f1 100644
--- a/iothread.c
+++ b/iothread.c
@@ -14,7 +14,6 @@
 #include "qom/object.h"
 #include "qom/object_interfaces.h"
 #include "qemu/module.h"
-#include "qemu/thread.h"
 #include "block/aio.h"
 #include "sysemu/iothread.h"
 #include "qmp-commands.h"
@@ -22,16 +21,6 @@
 #define IOTHREADS_PATH "/objects"
 
 typedef ObjectClass IOThreadClass;
-struct IOThread {
-    Object parent_obj;
-
-    QemuThread thread;
-    AioContext *ctx;
-    QemuMutex init_done_lock;
-    QemuCond init_done_cond;    /* is thread initialization done? */
-    bool stopping;
-    int thread_id;
-};
 
 #define IOTHREAD_GET_CLASS(obj) \
    OBJECT_GET_CLASS(IOThreadClass, obj, TYPE_IOTHREAD)
-- 
1.8.5.3

[Qemu-devel] [PATCH for-2.0 2/2] dataplane: replace iothread object_add() with embedded instance

[Stefan Hajnoczi]

Before IOThread was its own object, each virtio-blk device would create
its own internal thread.  We need to preserve this behavior for
backwards compatibility when users do not specify -device
virtio-blk-pci,iothread=<id>.

This patch changes how the internal IOThread object is created.
Previously we used the monitor object_add() function, which is really a
layering violation.  The problem is that this needs to assign a name but
we don't have a name for this internal object.

Generating names for internal objects is a pain but even worse is that
they may collide with user-defined names.

Paolo Bonzini <pbonzini@redhat.com> suggested that the internal IOThread
object should not be named.  This way the conflict cannot happen and we
no longer need object_add().

One gotcha is that internal IOThread objects will not be listed by the
query-iothreads command since they are not named.  This is okay though
because query-iothreads is new and the internal IOThread is just for
backwards compatibility.  New users should explicitly define IOThread
objects.

Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index f558b45..70b8a5a 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -23,7 +23,7 @@
 #include "virtio-blk.h"
 #include "block/aio.h"
 #include "hw/virtio/virtio-bus.h"
-#include "monitor/monitor.h" /* for object_add() */
+#include "qom/object_interfaces.h"
 
 enum {
     SEG_MAX = 126,                  /* maximum number of I/O segments */
@@ -59,7 +59,7 @@ struct VirtIOBlockDataPlane {
      * use it).
      */
     IOThread *iothread;
-    bool internal_iothread;
+    IOThread internal_iothread_obj;
     AioContext *ctx;
     EventNotifier io_notifier;      /* Linux AIO completion */
     EventNotifier host_notifier;    /* doorbell */
@@ -391,23 +391,19 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
     s->blk = blk;
 
     if (blk->iothread) {
-        s->internal_iothread = false;
         s->iothread = blk->iothread;
+        object_ref(OBJECT(s->iothread));
     } else {
-        /* Create per-device IOThread if none specified */
-        Error *local_err = NULL;
-
-        s->internal_iothread = true;
-        object_add(TYPE_IOTHREAD, vdev->name, NULL, NULL, &local_err);
-        if (error_is_set(&local_err)) {
-            error_propagate(errp, local_err);
-            g_free(s);
-            return;
-        }
-        s->iothread = iothread_find(vdev->name);
-        assert(s->iothread);
+        /* Create per-device IOThread if none specified.  This is for
+         * x-data-plane option compatibility.  If x-data-plane is removed we
+         * can drop this.
+         */
+        object_initialize(&s->internal_iothread_obj,
+                          sizeof(s->internal_iothread_obj),
+                          TYPE_IOTHREAD);
+        user_creatable_complete(OBJECT(&s->internal_iothread_obj), &error_abort);
+        s->iothread = &s->internal_iothread_obj;
     }
-    object_ref(OBJECT(s->iothread));
     s->ctx = iothread_get_aio_context(s->iothread);
 
     /* Prevent block operations that conflict with data plane thread */
@@ -426,9 +422,6 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
     virtio_blk_data_plane_stop(s);
     bdrv_set_in_use(s->blk->conf.bs, 0);
     object_unref(OBJECT(s->iothread));
-    if (s->internal_iothread) {
-        object_unparent(OBJECT(s->iothread));
-    }
     g_free(s);
 }
 
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH for-2.0 0/2] dataplane: fix internal IOThread name collision

[Paolo Bonzini]

Il 20/03/2014 15:06, Stefan Hajnoczi ha scritto:
> Christian Borntraeger <borntraeger@de.ibm.com> noticed that the naming of the
> internal IOThread objects, which are created when the user does not specify an
> IOThread explicitly, can result in collisions.
>
> These patches make the IOThread object nameless so it cannot cause collisions.
> This is important when multiple dataplane devices are used simultaneously or
> the user happened to choose a colliding name.
>
> Stefan Hajnoczi (2):
>   iothread: make IOThread struct definition public
>   dataplane: replace iothread object_add() with embedded instance
>
>  hw/block/dataplane/virtio-blk.c | 31 ++++++++++++-------------------
>  include/sysemu/iothread.h       | 12 +++++++++++-
>  iothread.c                      | 11 -----------
>  3 files changed, 23 insertions(+), 31 deletions(-)
>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH for-2.0 2/2] dataplane: replace iothread object_add() with embedded instance

[Paolo Bonzini]

Il 20/03/2014 15:06, Stefan Hajnoczi ha scritto:
> +        object_initialize(&s->internal_iothread_obj,
> +                          sizeof(s->internal_iothread_obj),
> +                          TYPE_IOTHREAD);
> +        user_creatable_complete(OBJECT(&s->internal_iothread_obj), &error_abort);

Perhaps you need an object_property_add_child here?  Otherwise the link 
points to an object without a canonical path.  Can do it as a follow-up 
though.

Paolo

Re: [Qemu-devel] [PATCH for-2.0 0/2] dataplane: fix internal IOThread name collision

[Christian Borntraeger]

On 20/03/14 15:06, Stefan Hajnoczi wrote:
> Christian Borntraeger <borntraeger@de.ibm.com> noticed that the naming of the
> internal IOThread objects, which are created when the user does not specify an
> IOThread explicitly, can result in collisions.
> 
> These patches make the IOThread object nameless so it cannot cause collisions.
> This is important when multiple dataplane devices are used simultaneously or
> the user happened to choose a colliding name.
> 
> Stefan Hajnoczi (2):
>   iothread: make IOThread struct definition public
>   dataplane: replace iothread object_add() with embedded instance
> 
>  hw/block/dataplane/virtio-blk.c | 31 ++++++++++++-------------------
>  include/sysemu/iothread.h       | 12 +++++++++++-
>  iothread.c                      | 11 -----------
>  3 files changed, 23 insertions(+), 31 deletions(-)
> 

This seem to make my setup functional again.

Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>

Thanks

Re: [Qemu-devel] [PATCH for-2.0 2/2] dataplane: replace iothread object_add() with embedded instance

[Stefan Hajnoczi]

On Thu, Mar 20, 2014 at 03:37:54PM +0100, Paolo Bonzini wrote:
> Il 20/03/2014 15:06, Stefan Hajnoczi ha scritto:
> >+        object_initialize(&s->internal_iothread_obj,
> >+                          sizeof(s->internal_iothread_obj),
> >+                          TYPE_IOTHREAD);
> >+        user_creatable_complete(OBJECT(&s->internal_iothread_obj), &error_abort);
> 
> Perhaps you need an object_property_add_child here?  Otherwise the
> link points to an object without a canonical path.  Can do it as a
> follow-up though.

The link doesn't exist yet since we're still using a qdev property.

I will add the child property when we switch to a link in QEMU 2.1.  In
order to do that, I posted an RFC series that shows how I propose to
clean up the virtio transport/device split which is currently broken.

Stefan