* [Qemu-devel] [PATCH 0/2] 9p-synth: remove poor-man RCU @ 2012-08-08 11:55 Paolo Bonzini 2012-08-08 11:55 ` [Qemu-devel] [PATCH 1/2] 9p-synth: fix read-side critical sections Paolo Bonzini 2012-08-08 11:55 ` [Qemu-devel] [PATCH 2/2] 9p-synth: use mutex on read-side Paolo Bonzini 0 siblings, 2 replies; 6+ messages in thread From: Paolo Bonzini @ 2012-08-08 11:55 UTC (permalink / raw) To: qemu-devel; +Cc: harsh, aneesh.kumar 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 1/2] 9p-synth: fix read-side critical sections 2012-08-08 11:55 [Qemu-devel] [PATCH 0/2] 9p-synth: remove poor-man RCU Paolo Bonzini @ 2012-08-08 11:55 ` Paolo Bonzini 2012-08-13 18:47 ` Harsh Bora 2012-08-08 11:55 ` [Qemu-devel] [PATCH 2/2] 9p-synth: use mutex on read-side Paolo Bonzini 1 sibling, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2012-08-08 11:55 UTC (permalink / raw) To: qemu-devel; +Cc: harsh, aneesh.kumar 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] 9p-synth: fix read-side critical sections 2012-08-08 11:55 ` [Qemu-devel] [PATCH 1/2] 9p-synth: fix read-side critical sections Paolo Bonzini @ 2012-08-13 18:47 ` Harsh Bora 0 siblings, 0 replies; 6+ messages in thread From: Harsh Bora @ 2012-08-13 18:47 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, aneesh.kumar 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, > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 2/2] 9p-synth: use mutex on read-side 2012-08-08 11:55 [Qemu-devel] [PATCH 0/2] 9p-synth: remove poor-man RCU Paolo Bonzini 2012-08-08 11:55 ` [Qemu-devel] [PATCH 1/2] 9p-synth: fix read-side critical sections Paolo Bonzini @ 2012-08-08 11:55 ` Paolo Bonzini 2012-08-13 19:13 ` Harsh Bora 1 sibling, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2012-08-08 11:55 UTC (permalink / raw) To: qemu-devel; +Cc: harsh, aneesh.kumar 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] 9p-synth: use mutex on read-side 2012-08-08 11:55 ` [Qemu-devel] [PATCH 2/2] 9p-synth: use mutex on read-side Paolo Bonzini @ 2012-08-13 19:13 ` Harsh Bora 2012-08-18 18:46 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Harsh Bora @ 2012-08-13 19:13 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, aneesh.kumar 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); > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] 9p-synth: use mutex on read-side 2012-08-13 19:13 ` Harsh Bora @ 2012-08-18 18:46 ` Paolo Bonzini 0 siblings, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2012-08-18 18:46 UTC (permalink / raw) To: Harsh Bora; +Cc: qemu-devel, aneesh.kumar 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-08-18 18:46 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-08-08 11:55 [Qemu-devel] [PATCH 0/2] 9p-synth: remove poor-man RCU Paolo Bonzini 2012-08-08 11:55 ` [Qemu-devel] [PATCH 1/2] 9p-synth: fix read-side critical sections Paolo Bonzini 2012-08-13 18:47 ` Harsh Bora 2012-08-08 11:55 ` [Qemu-devel] [PATCH 2/2] 9p-synth: use mutex on read-side Paolo Bonzini 2012-08-13 19:13 ` Harsh Bora 2012-08-18 18:46 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).