[Qemu-devel] [PATCH 0/2] 9p-synth: remove poor-man RCU

[Qemu-devel] [PATCH 0/2] 9p-synth: remove poor-man RCU

[Paolo Bonzini]

There are several mistakes in the pseudo-RCU usage of 9pfs.  This
patch series converts everything to a regular mutex.  Patch 1 applies
enough fixes so that a trivial mutex-based synchronization is correct.
Patch 2 uses the mutex for both read and write sides.

Paolo Bonzini (2):
  9p-synth: fix read-side critical sections
  9p-synth: use mutex on read-side

 hw/9pfs/virtio-9p-synth.c | 41 +++++++++++++++++++++++------------------
 qemu-thread.h             |  3 ---
 2 file modificati, 23 inserzioni(+), 21 rimozioni(-)

-- 
1.7.11.2

[Qemu-devel] [PATCH 1/2] 9p-synth: fix read-side critical sections

[Paolo Bonzini]

The read-side critical sections in 9p-synth currently only include the
navigation of the list.  This is incorrect; it works for two reasons,
first obviously because rcu_read_lock/unlock are still no-ops; second,
because elements of the list are never deleted from the list (only added).
In fact, only adding items is the reason why rcu_read_lock/unlock can
be left as no-ops.

If items were deleted, they could be reclaimed as soon as the read-side
critical section ends.  So, the read-side critical section must include
all _usage_ of the node we got from the list too.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/9pfs/virtio-9p-synth.c | 35 ++++++++++++++++++++---------------
 1 file modificato, 20 inserzioni(+), 15 rimozioni(-)

diff --git a/hw/9pfs/virtio-9p-synth.c b/hw/9pfs/virtio-9p-synth.c
index 92e0b09..a91ebe1 100644
--- a/hw/9pfs/virtio-9p-synth.c
+++ b/hw/9pfs/virtio-9p-synth.c
@@ -237,14 +237,15 @@ static int v9fs_synth_get_dentry(V9fsSynthNode *dir, struct dirent *entry,
         }
         i++;
     }
-    rcu_read_unlock();
     if (!node) {
         /* end of directory */
         *result = NULL;
-        return 0;
+        goto out;
     }
     v9fs_synth_direntry(node, entry, off);
     *result = entry;
+out:
+    rcu_read_unlock();
     return 0;
 }
 
@@ -466,6 +467,7 @@ static int v9fs_synth_name_to_path(FsContext *ctx, V9fsPath *dir_path,
 {
     V9fsSynthNode *node;
     V9fsSynthNode *dir_node;
+    int ret = 0;
 
     /* "." and ".." are not allowed */
     if (!strcmp(name, ".") || !strcmp(name, "..")) {
@@ -473,34 +475,37 @@ static int v9fs_synth_name_to_path(FsContext *ctx, V9fsPath *dir_path,
         return -1;
 
     }
+
+    rcu_read_lock();
     if (!dir_path) {
         dir_node = &v9fs_synth_root;
     } else {
         dir_node = *(V9fsSynthNode **)dir_path->data;
     }
-    if (!strcmp(name, "/")) {
-        node = dir_node;
-        goto out;
-    }
-    /* search for the name in the childern */
-    rcu_read_lock();
-    QLIST_FOREACH(node, &dir_node->child, sibling) {
-        if (!strcmp(node->name, name)) {
-            break;
+
+    node = dir_node;
+    if (strcmp(name, "/") != 0) {
+        /* search for the name in the childern */
+        QLIST_FOREACH(node, &dir_node->child, sibling) {
+            if (!strcmp(node->name, name)) {
+                break;
+            }
         }
     }
-    rcu_read_unlock();
 
     if (!node) {
         errno = ENOENT;
-        return -1;
+        ret = -1;
+        goto err_out;
     }
-out:
+
     /* Copy the node pointer to fid */
     target->data = g_malloc(sizeof(void *));
     memcpy(target->data, &node, sizeof(void *));
     target->size = sizeof(void *);
-    return 0;
+err_out:
+    rcu_read_unlock();
+    return ret;
 }
 
 static int v9fs_synth_renameat(FsContext *ctx, V9fsPath *olddir,
-- 
1.7.11.2

[Qemu-devel] [PATCH 2/2] 9p-synth: use mutex on read-side

[Paolo Bonzini]

Even with the fix in the previous patch, the lockless handling of paths
in 9p-synth is wrong.  Paths can outlive rcu_read_unlock arbitrarily
via the V9fsPath objects that 9p-synth creates.  This would require
a reference counting mechanism that is not there and is quite hard to
retrofit into V9fsPath.

It seems to me that this was a premature optimization, so replace
everything with a simple mutex.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/9pfs/virtio-9p-synth.c | 12 ++++++------
 qemu-thread.h             |  3 ---
 2 file modificati, 6 inserzioni(+), 9 rimozioni(-)

diff --git a/hw/9pfs/virtio-9p-synth.c b/hw/9pfs/virtio-9p-synth.c
index a91ebe1..426605e 100644
--- a/hw/9pfs/virtio-9p-synth.c
+++ b/hw/9pfs/virtio-9p-synth.c
@@ -59,7 +59,7 @@ static V9fsSynthNode *v9fs_add_dir_node(V9fsSynthNode *parent, int mode,
     }
     node->private = node;
     strncpy(node->name, name, sizeof(node->name));
-    QLIST_INSERT_HEAD_RCU(&parent->child, node, sibling);
+    QLIST_INSERT_HEAD(&parent->child, node, sibling);
     return node;
 }
 
@@ -133,7 +133,7 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int mode,
     node->attr->mode   = mode;
     node->private      = arg;
     strncpy(node->name, name, sizeof(node->name));
-    QLIST_INSERT_HEAD_RCU(&parent->child, node, sibling);
+    QLIST_INSERT_HEAD(&parent->child, node, sibling);
     ret = 0;
 err_out:
     qemu_mutex_unlock(&v9fs_synth_mutex);
@@ -229,7 +229,7 @@ static int v9fs_synth_get_dentry(V9fsSynthNode *dir, struct dirent *entry,
     int i = 0;
     V9fsSynthNode *node;
 
-    rcu_read_lock();
+    qemu_mutex_lock(&v9fs_synth_mutex);
     QLIST_FOREACH(node, &dir->child, sibling) {
         /* This is the off child of the directory */
         if (i == off) {
@@ -245,7 +245,7 @@ static int v9fs_synth_get_dentry(V9fsSynthNode *dir, struct dirent *entry,
     v9fs_synth_direntry(node, entry, off);
     *result = entry;
 out:
-    rcu_read_unlock();
+    qemu_mutex_unlock(&v9fs_synth_mutex);
     return 0;
 }
 
@@ -476,7 +476,7 @@ static int v9fs_synth_name_to_path(FsContext *ctx, V9fsPath *dir_path,
 
     }
 
-    rcu_read_lock();
+    qemu_mutex_lock(&v9fs_synth_mutex);
     if (!dir_path) {
         dir_node = &v9fs_synth_root;
     } else {
@@ -504,7 +504,7 @@ static int v9fs_synth_name_to_path(FsContext *ctx, V9fsPath *dir_path,
     memcpy(target->data, &node, sizeof(void *));
     target->size = sizeof(void *);
 err_out:
-    rcu_read_unlock();
+    qemu_mutex_unlock(&v9fs_synth_mutex);
     return ret;
 }
 
diff --git a/qemu-thread.h b/qemu-thread.h
index 05fdaaf..3c9715e 100644
--- a/qemu-thread.h
+++ b/qemu-thread.h
@@ -23,9 +23,6 @@ void qemu_mutex_lock(QemuMutex *mutex);
 int qemu_mutex_trylock(QemuMutex *mutex);
 void qemu_mutex_unlock(QemuMutex *mutex);
 
-#define rcu_read_lock() do { } while (0)
-#define rcu_read_unlock() do { } while (0)
-
 void qemu_cond_init(QemuCond *cond);
 void qemu_cond_destroy(QemuCond *cond);
 
-- 
1.7.11.2

Re: [Qemu-devel] [PATCH 1/2] 9p-synth: fix read-side critical sections

[Harsh Bora]

On 08/08/2012 05:25 PM, Paolo Bonzini wrote:
> The read-side critical sections in 9p-synth currently only include the
> navigation of the list.  This is incorrect; it works for two reasons,
> first obviously because rcu_read_lock/unlock are still no-ops; second,
> because elements of the list are never deleted from the list (only added).
> In fact, only adding items is the reason why rcu_read_lock/unlock can
> be left as no-ops.
>
> If items were deleted, they could be reclaimed as soon as the read-side
> critical section ends.  So, the read-side critical section must include
> all _usage_ of the node we got from the list too.

Acked-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>

>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/9pfs/virtio-9p-synth.c | 35 ++++++++++++++++++++---------------
>   1 file modificato, 20 inserzioni(+), 15 rimozioni(-)
>
> diff --git a/hw/9pfs/virtio-9p-synth.c b/hw/9pfs/virtio-9p-synth.c
> index 92e0b09..a91ebe1 100644
> --- a/hw/9pfs/virtio-9p-synth.c
> +++ b/hw/9pfs/virtio-9p-synth.c
> @@ -237,14 +237,15 @@ static int v9fs_synth_get_dentry(V9fsSynthNode *dir, struct dirent *entry,
>           }
>           i++;
>       }
> -    rcu_read_unlock();
>       if (!node) {
>           /* end of directory */
>           *result = NULL;
> -        return 0;
> +        goto out;
>       }
>       v9fs_synth_direntry(node, entry, off);
>       *result = entry;
> +out:
> +    rcu_read_unlock();
>       return 0;
>   }
>
> @@ -466,6 +467,7 @@ static int v9fs_synth_name_to_path(FsContext *ctx, V9fsPath *dir_path,
>   {
>       V9fsSynthNode *node;
>       V9fsSynthNode *dir_node;
> +    int ret = 0;
>
>       /* "." and ".." are not allowed */
>       if (!strcmp(name, ".") || !strcmp(name, "..")) {
> @@ -473,34 +475,37 @@ static int v9fs_synth_name_to_path(FsContext *ctx, V9fsPath *dir_path,
>           return -1;
>
>       }
> +
> +    rcu_read_lock();
>       if (!dir_path) {
>           dir_node = &v9fs_synth_root;
>       } else {
>           dir_node = *(V9fsSynthNode **)dir_path->data;
>       }
> -    if (!strcmp(name, "/")) {
> -        node = dir_node;
> -        goto out;
> -    }
> -    /* search for the name in the childern */
> -    rcu_read_lock();
> -    QLIST_FOREACH(node, &dir_node->child, sibling) {
> -        if (!strcmp(node->name, name)) {
> -            break;
> +
> +    node = dir_node;
> +    if (strcmp(name, "/") != 0) {
> +        /* search for the name in the childern */
> +        QLIST_FOREACH(node, &dir_node->child, sibling) {
> +            if (!strcmp(node->name, name)) {
> +                break;
> +            }
>           }
>       }
> -    rcu_read_unlock();
>
>       if (!node) {
>           errno = ENOENT;
> -        return -1;
> +        ret = -1;
> +        goto err_out;
>       }
> -out:
> +
>       /* Copy the node pointer to fid */
>       target->data = g_malloc(sizeof(void *));
>       memcpy(target->data, &node, sizeof(void *));
>       target->size = sizeof(void *);
> -    return 0;
> +err_out:
> +    rcu_read_unlock();
> +    return ret;
>   }
>
>   static int v9fs_synth_renameat(FsContext *ctx, V9fsPath *olddir,
>

Re: [Qemu-devel] [PATCH 2/2] 9p-synth: use mutex on read-side

[Harsh Bora]

On 08/08/2012 05:25 PM, Paolo Bonzini wrote:
> Even with the fix in the previous patch, the lockless handling of paths
> in 9p-synth is wrong.  Paths can outlive rcu_read_unlock arbitrarily
> via the V9fsPath objects that 9p-synth creates.  This would require
> a reference counting mechanism that is not there and is quite hard to
> retrofit into V9fsPath.
>
> It seems to me that this was a premature optimization, so replace
> everything with a simple mutex.

Hi Paolo,

The rcu_read_[un]lock() macros were added as no-ops (based on your 
inputs on #qemu) to replace reader-writer locks with RCU based locking 
as suggested while proposing QemuRWLock API for RW locks (See 
http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg00192.html).

v9fs_synth_mutex was actually a pthread_rwlock_t earlier.
I am not sure if reader lock would be better than having a plain mutex 
for readers as well.

Aneesh, inputs ?

Also, if we are going forward with this change, we may also want to 
remove definition of QLIST_INSERT_HEAD_RCU, since the code being remove 
below is the only consumer of that macro.

regards,
Harsh

>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/9pfs/virtio-9p-synth.c | 12 ++++++------
>   qemu-thread.h             |  3 ---
>   2 file modificati, 6 inserzioni(+), 9 rimozioni(-)
>
> diff --git a/hw/9pfs/virtio-9p-synth.c b/hw/9pfs/virtio-9p-synth.c
> index a91ebe1..426605e 100644
> --- a/hw/9pfs/virtio-9p-synth.c
> +++ b/hw/9pfs/virtio-9p-synth.c
> @@ -59,7 +59,7 @@ static V9fsSynthNode *v9fs_add_dir_node(V9fsSynthNode *parent, int mode,
>       }
>       node->private = node;
>       strncpy(node->name, name, sizeof(node->name));
> -    QLIST_INSERT_HEAD_RCU(&parent->child, node, sibling);
> +    QLIST_INSERT_HEAD(&parent->child, node, sibling);
>       return node;
>   }
>
> @@ -133,7 +133,7 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int mode,
>       node->attr->mode   = mode;
>       node->private      = arg;
>       strncpy(node->name, name, sizeof(node->name));
> -    QLIST_INSERT_HEAD_RCU(&parent->child, node, sibling);
> +    QLIST_INSERT_HEAD(&parent->child, node, sibling);
>       ret = 0;
>   err_out:
>       qemu_mutex_unlock(&v9fs_synth_mutex);
> @@ -229,7 +229,7 @@ static int v9fs_synth_get_dentry(V9fsSynthNode *dir, struct dirent *entry,
>       int i = 0;
>       V9fsSynthNode *node;
>
> -    rcu_read_lock();
> +    qemu_mutex_lock(&v9fs_synth_mutex);
>       QLIST_FOREACH(node, &dir->child, sibling) {
>           /* This is the off child of the directory */
>           if (i == off) {
> @@ -245,7 +245,7 @@ static int v9fs_synth_get_dentry(V9fsSynthNode *dir, struct dirent *entry,
>       v9fs_synth_direntry(node, entry, off);
>       *result = entry;
>   out:
> -    rcu_read_unlock();
> +    qemu_mutex_unlock(&v9fs_synth_mutex);
>       return 0;
>   }
>
> @@ -476,7 +476,7 @@ static int v9fs_synth_name_to_path(FsContext *ctx, V9fsPath *dir_path,
>
>       }
>
> -    rcu_read_lock();
> +    qemu_mutex_lock(&v9fs_synth_mutex);
>       if (!dir_path) {
>           dir_node = &v9fs_synth_root;
>       } else {
> @@ -504,7 +504,7 @@ static int v9fs_synth_name_to_path(FsContext *ctx, V9fsPath *dir_path,
>       memcpy(target->data, &node, sizeof(void *));
>       target->size = sizeof(void *);
>   err_out:
> -    rcu_read_unlock();
> +    qemu_mutex_unlock(&v9fs_synth_mutex);
>       return ret;
>   }
>
> diff --git a/qemu-thread.h b/qemu-thread.h
> index 05fdaaf..3c9715e 100644
> --- a/qemu-thread.h
> +++ b/qemu-thread.h
> @@ -23,9 +23,6 @@ void qemu_mutex_lock(QemuMutex *mutex);
>   int qemu_mutex_trylock(QemuMutex *mutex);
>   void qemu_mutex_unlock(QemuMutex *mutex);
>
> -#define rcu_read_lock() do { } while (0)
> -#define rcu_read_unlock() do { } while (0)
> -
>   void qemu_cond_init(QemuCond *cond);
>   void qemu_cond_destroy(QemuCond *cond);
>

Re: [Qemu-devel] [PATCH 2/2] 9p-synth: use mutex on read-side

[Paolo Bonzini]

Il 13/08/2012 21:13, Harsh Bora ha scritto:
>>
> 
> Hi Paolo,
> 
> The rcu_read_[un]lock() macros were added as no-ops (based on your
> inputs on #qemu) to replace reader-writer locks with RCU based locking
> as suggested while proposing QemuRWLock API for RW locks (See
> http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg00192.html).

The rwlock was also premature optimization IMHO.

> v9fs_synth_mutex was actually a pthread_rwlock_t earlier.
> I am not sure if reader lock would be better than having a plain mutex
> for readers as well.
> 
> Aneesh, inputs ?
> 
> Also, if we are going forward with this change, we may also want to
> remove definition of QLIST_INSERT_HEAD_RCU, since the code being remove
> below is the only consumer of that macro.

It's in a library file, so I don't mind leaving it in.  I think liu ping
fan's patches were using it.

Paolo