[Qemu-devel] [PATCH v2 1/2] Introduce QLIST_INSERT_HEAD_RCU and dummy RCU wrappers.

[Qemu-devel] [PATCH v2 1/2] Introduce QLIST_INSERT_HEAD_RCU and dummy RCU wrappers.

[Harsh Prateek Bora]

SynthFS needs a QLIST_INSERT_HEAD_RCU to make sure list instructions are not
re-ordered and therefore avoiding a crash. There may be parallel readers which
should be allowed for lock-free access and this variant allows us to get rid 
of rwlocks used by readers.

SynthFS is a special case where we dont really need full RCU capabilities as
it doesnt allow list entry deletion but concurrent readers/writers and 
instruction re-ordering should not result in a crash.

Also, once the real rcu is available, dummy rcu macro definitions will go away
and the code will still work as expected.

This patchwork is based on inputs from Paolo Bonzini.

Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
---
 qemu-queue.h  |   12 ++++++++++++
 qemu-thread.h |    3 +++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/qemu-queue.h b/qemu-queue.h
index 1d07745..57234f8 100644
--- a/qemu-queue.h
+++ b/qemu-queue.h
@@ -76,6 +76,8 @@
  * For details on the use of these macros, see the queue(3) manual page.
  */
 
+#include "qemu-barrier.h" /* for smp_wmb() */
+
 /*
  * List definitions.
  */
@@ -122,6 +124,16 @@ struct {                                                                \
         (elm)->field.le_prev = &(head)->lh_first;                       \
 } while (/*CONSTCOND*/0)
 
+#define QLIST_INSERT_HEAD_RCU(head, elm, field) do {                    \
+        (elm)->field.le_prev = &(head)->lh_first;                       \
+        smp_wmb();                                                      \
+        if (((elm)->field.le_next = (head)->lh_first) != NULL)          \
+                (head)->lh_first->field.le_prev = &(elm)->field.le_next;\
+        smp_wmb();                                                      \
+        (head)->lh_first = (elm);                                       \
+        smp_wmb();                                                      \
+} while (/* CONSTCOND*/0)
+
 #define QLIST_REMOVE(elm, field) do {                                   \
         if ((elm)->field.le_next != NULL)                               \
                 (elm)->field.le_next->field.le_prev =                   \
diff --git a/qemu-thread.h b/qemu-thread.h
index 0a73d50..e008b60 100644
--- a/qemu-thread.h
+++ b/qemu-thread.h
@@ -19,6 +19,9 @@ 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.1.1

[Qemu-devel] [PATCH v2 2/2] Replace rwlocks with RCU variants of interfaces.

[Harsh Prateek Bora]

Use QLIST_INSERT_HEAD_RCU and rcu_read_lock/unlock instead of rwlocks.
Use v9fs_synth_mutex as a write-only mutex to handle concurrent writers.

Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p-synth.c |   26 ++++++++++++--------------
 1 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/hw/9pfs/virtio-9p-synth.c b/hw/9pfs/virtio-9p-synth.c
index cbf74e4..567611e 100644
--- a/hw/9pfs/virtio-9p-synth.c
+++ b/hw/9pfs/virtio-9p-synth.c
@@ -30,8 +30,7 @@ V9fsSynthNode v9fs_synth_root = {
     .attr = &v9fs_synth_root.actual_attr,
 };
 
-/*FIXME!! should be converted to qemu_rwlock_t */
-static pthread_rwlock_t v9fs_synth_mutex;
+static QemuMutex  v9fs_synth_mutex;
 static int v9fs_synth_node_count;
 /* set to 1 when the synth fs is ready */
 static int v9fs_synth_fs;
@@ -79,7 +78,7 @@ int qemu_v9fs_synth_mkdir(V9fsSynthNode *parent, int mode,
     if (!parent) {
         parent = &v9fs_synth_root;
     }
-    pthread_rwlock_wrlock(&v9fs_synth_mutex);
+    qemu_mutex_lock(&v9fs_synth_mutex);
     QLIST_FOREACH(tmp, &parent->child, sibling) {
         if (!strcmp(tmp->name, name)) {
             ret = EEXIST;
@@ -95,7 +94,7 @@ int qemu_v9fs_synth_mkdir(V9fsSynthNode *parent, int mode,
     *result = node;
     ret = 0;
 err_out:
-    pthread_rwlock_unlock(&v9fs_synth_mutex);
+    qemu_mutex_unlock(&v9fs_synth_mutex);
     return ret;
 }
 
@@ -116,7 +115,7 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int mode,
         parent = &v9fs_synth_root;
     }
 
-    pthread_rwlock_wrlock(&v9fs_synth_mutex);
+    qemu_mutex_lock(&v9fs_synth_mutex);
     QLIST_FOREACH(tmp, &parent->child, sibling) {
         if (!strcmp(tmp->name, name)) {
             ret = EEXIST;
@@ -134,10 +133,10 @@ 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(&parent->child, node, sibling);
+    QLIST_INSERT_HEAD_RCU(&parent->child, node, sibling);
     ret = 0;
 err_out:
-    pthread_rwlock_unlock(&v9fs_synth_mutex);
+    qemu_mutex_unlock(&v9fs_synth_mutex);
     return ret;
 }
 
@@ -230,7 +229,7 @@ static int v9fs_synth_get_dentry(V9fsSynthNode *dir, struct dirent *entry,
     int i = 0;
     V9fsSynthNode *node;
 
-    pthread_rwlock_rdlock(&v9fs_synth_mutex);
+    rcu_read_lock();
     QLIST_FOREACH(node, &dir->child, sibling) {
         /* This is the off child of the directory */
         if (i == off) {
@@ -238,7 +237,7 @@ static int v9fs_synth_get_dentry(V9fsSynthNode *dir, struct dirent *entry,
         }
         i++;
     }
-    pthread_rwlock_unlock(&v9fs_synth_mutex);
+    rcu_read_unlock();
     if (!node) {
         /* end of directory */
         *result = NULL;
@@ -483,13 +482,14 @@ static int v9fs_synth_name_to_path(FsContext *ctx, V9fsPath *dir_path,
         goto out;
     }
     /* search for the name in the childern */
-    pthread_rwlock_rdlock(&v9fs_synth_mutex);
+    rcu_read_lock();
     QLIST_FOREACH(node, &dir_node->child, sibling) {
         if (!strcmp(node->name, name)) {
             break;
         }
     }
-    pthread_rwlock_unlock(&v9fs_synth_mutex);
+    rcu_read_unlock();
+
     if (!node) {
         errno = ENOENT;
         return -1;
@@ -532,11 +532,9 @@ static ssize_t my_test_read(void *in_buf, int len, off_t offset, void *arg)
 static int v9fs_synth_init(FsContext *ctx)
 {
     V9fsSynthNode *node;
-    pthread_rwlockattr_t rwlockattr;
 
     QLIST_INIT(&v9fs_synth_root.child);
-    pthread_rwlockattr_init(&rwlockattr);
-    pthread_rwlock_init(&v9fs_synth_mutex, &rwlockattr);
+    qemu_mutex_init(&v9fs_synth_mutex);
 
     /* Add "." and ".." entries for root */
     v9fs_add_dir_node(&v9fs_synth_root, v9fs_synth_root.attr->mode,
-- 
1.7.1.1

Re: [Qemu-devel] [PATCH v2 2/2] Replace rwlocks with RCU variants of interfaces.

[Harsh Bora]

On 10/14/2011 02:05 AM, Harsh Prateek Bora wrote:
> Use QLIST_INSERT_HEAD_RCU and rcu_read_lock/unlock instead of rwlocks.
> Use v9fs_synth_mutex as a write-only mutex to handle concurrent writers.

This patch is based on top of:
http://repo.or.cz/w/qemu/v9fs.git/commitdiff/d647a2337e6e010c9f733d1ff68f7f97b27c5f80
>
> Signed-off-by: Harsh Prateek Bora<harsh@linux.vnet.ibm.com>
> ---
>   hw/9pfs/virtio-9p-synth.c |   26 ++++++++++++--------------
>   1 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p-synth.c b/hw/9pfs/virtio-9p-synth.c
> index cbf74e4..567611e 100644
> --- a/hw/9pfs/virtio-9p-synth.c
> +++ b/hw/9pfs/virtio-9p-synth.c
> @@ -30,8 +30,7 @@ V9fsSynthNode v9fs_synth_root = {
>       .attr =&v9fs_synth_root.actual_attr,
>   };
>
> -/*FIXME!! should be converted to qemu_rwlock_t */
> -static pthread_rwlock_t v9fs_synth_mutex;
> +static QemuMutex  v9fs_synth_mutex;
>   static int v9fs_synth_node_count;
>   /* set to 1 when the synth fs is ready */
>   static int v9fs_synth_fs;
> @@ -79,7 +78,7 @@ int qemu_v9fs_synth_mkdir(V9fsSynthNode *parent, int mode,
>       if (!parent) {
>           parent =&v9fs_synth_root;
>       }
> -    pthread_rwlock_wrlock(&v9fs_synth_mutex);
> +    qemu_mutex_lock(&v9fs_synth_mutex);
>       QLIST_FOREACH(tmp,&parent->child, sibling) {
>           if (!strcmp(tmp->name, name)) {
>               ret = EEXIST;
> @@ -95,7 +94,7 @@ int qemu_v9fs_synth_mkdir(V9fsSynthNode *parent, int mode,
>       *result = node;
>       ret = 0;
>   err_out:
> -    pthread_rwlock_unlock(&v9fs_synth_mutex);
> +    qemu_mutex_unlock(&v9fs_synth_mutex);
>       return ret;
>   }
>
> @@ -116,7 +115,7 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int mode,
>           parent =&v9fs_synth_root;
>       }
>
> -    pthread_rwlock_wrlock(&v9fs_synth_mutex);
> +    qemu_mutex_lock(&v9fs_synth_mutex);
>       QLIST_FOREACH(tmp,&parent->child, sibling) {
>           if (!strcmp(tmp->name, name)) {
>               ret = EEXIST;
> @@ -134,10 +133,10 @@ 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(&parent->child, node, sibling);
> +    QLIST_INSERT_HEAD_RCU(&parent->child, node, sibling);
>       ret = 0;
>   err_out:
> -    pthread_rwlock_unlock(&v9fs_synth_mutex);
> +    qemu_mutex_unlock(&v9fs_synth_mutex);
>       return ret;
>   }
>
> @@ -230,7 +229,7 @@ static int v9fs_synth_get_dentry(V9fsSynthNode *dir, struct dirent *entry,
>       int i = 0;
>       V9fsSynthNode *node;
>
> -    pthread_rwlock_rdlock(&v9fs_synth_mutex);
> +    rcu_read_lock();
>       QLIST_FOREACH(node,&dir->child, sibling) {
>           /* This is the off child of the directory */
>           if (i == off) {
> @@ -238,7 +237,7 @@ static int v9fs_synth_get_dentry(V9fsSynthNode *dir, struct dirent *entry,
>           }
>           i++;
>       }
> -    pthread_rwlock_unlock(&v9fs_synth_mutex);
> +    rcu_read_unlock();
>       if (!node) {
>           /* end of directory */
>           *result = NULL;
> @@ -483,13 +482,14 @@ static int v9fs_synth_name_to_path(FsContext *ctx, V9fsPath *dir_path,
>           goto out;
>       }
>       /* search for the name in the childern */
> -    pthread_rwlock_rdlock(&v9fs_synth_mutex);
> +    rcu_read_lock();
>       QLIST_FOREACH(node,&dir_node->child, sibling) {
>           if (!strcmp(node->name, name)) {
>               break;
>           }
>       }
> -    pthread_rwlock_unlock(&v9fs_synth_mutex);
> +    rcu_read_unlock();
> +
>       if (!node) {
>           errno = ENOENT;
>           return -1;
> @@ -532,11 +532,9 @@ static ssize_t my_test_read(void *in_buf, int len, off_t offset, void *arg)
>   static int v9fs_synth_init(FsContext *ctx)
>   {
>       V9fsSynthNode *node;
> -    pthread_rwlockattr_t rwlockattr;
>
>       QLIST_INIT(&v9fs_synth_root.child);
> -    pthread_rwlockattr_init(&rwlockattr);
> -    pthread_rwlock_init(&v9fs_synth_mutex,&rwlockattr);
> +    qemu_mutex_init(&v9fs_synth_mutex);
>
>       /* Add "." and ".." entries for root */
>       v9fs_add_dir_node(&v9fs_synth_root, v9fs_synth_root.attr->mode,

Re: [Qemu-devel] [PATCH v2 2/2] Replace rwlocks with RCU variants of interfaces.

[Aneesh Kumar K.V]

On Fri, 14 Oct 2011 02:05:02 +0530, Harsh Prateek Bora <harsh@linux.vnet.ibm.com> wrote:
> Use QLIST_INSERT_HEAD_RCU and rcu_read_lock/unlock instead of rwlocks.
> Use v9fs_synth_mutex as a write-only mutex to handle concurrent writers.
> 
> Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
> ---
>  hw/9pfs/virtio-9p-synth.c |   26 ++++++++++++--------------
>  1 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/9pfs/virtio-9p-synth.c b/hw/9pfs/virtio-9p-synth.c
> index cbf74e4..567611e 100644
> --- a/hw/9pfs/virtio-9p-synth.c
> +++ b/hw/9pfs/virtio-9p-synth.c
> @@ -30,8 +30,7 @@ V9fsSynthNode v9fs_synth_root = {
>      .attr = &v9fs_synth_root.actual_attr,
>  };
> 
> -/*FIXME!! should be converted to qemu_rwlock_t */
> -static pthread_rwlock_t v9fs_synth_mutex;
> +static QemuMutex  v9fs_synth_mutex;
>  static int v9fs_synth_node_count;
>  /* set to 1 when the synth fs is ready */
>  static int v9fs_synth_fs;
> @@ -79,7 +78,7 @@ int qemu_v9fs_synth_mkdir(V9fsSynthNode *parent, int mode,
>      if (!parent) {
>          parent = &v9fs_synth_root;
>      }
> -    pthread_rwlock_wrlock(&v9fs_synth_mutex);
> +    qemu_mutex_lock(&v9fs_synth_mutex);
>      QLIST_FOREACH(tmp, &parent->child, sibling) {
>          if (!strcmp(tmp->name, name)) {
>              ret = EEXIST;
> @@ -95,7 +94,7 @@ int qemu_v9fs_synth_mkdir(V9fsSynthNode *parent, int mode,
>      *result = node;
>      ret = 0;
>  err_out:
> -    pthread_rwlock_unlock(&v9fs_synth_mutex);
> +    qemu_mutex_unlock(&v9fs_synth_mutex);
>      return ret;


We sould be using QLIST_INSERT_HEAD_RCU even in v9fs_add_dir_node


>  }
> 
> @@ -116,7 +115,7 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int mode,
>          parent = &v9fs_synth_root;
>      }
> 

-aneesh

Re: [Qemu-devel] [PATCH v2 1/2] Introduce QLIST_INSERT_HEAD_RCU and dummy RCU wrappers.

[Paolo Bonzini]

On 10/13/2011 10:35 PM, Harsh Prateek Bora wrote:
>
> +#define QLIST_INSERT_HEAD_RCU(head, elm, field) do {                    \
> +        (elm)->field.le_prev =&(head)->lh_first;                       \
> +        smp_wmb();                                                      \
> +        if (((elm)->field.le_next = (head)->lh_first) != NULL)          \
> +                (head)->lh_first->field.le_prev =&(elm)->field.le_next;\
> +        smp_wmb();                                                      \
> +        (head)->lh_first = (elm);                                       \
> +        smp_wmb();                                                      \
> +} while (/* CONSTCOND*/0)

Actually, looking more at it it should be more like

        (elm)->field.le_prev =&(head)->lh_first;
        (elm)->field.le_next = (head)->lh_first;
        smb_wmb(); /* fill elm before linking it */
        if ((head)->lh_first != NULL)
           (head)->lh_first->field.le_prev =&(elm)->field.le_next;
        (head)->lh_first = (elm);
        smp_wmb();

... which even saves a memory barrier.

Paolo