From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37710) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T104i-0007Ad-KG for qemu-devel@nongnu.org; Mon, 13 Aug 2012 15:13:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T104g-0001y7-Vw for qemu-devel@nongnu.org; Mon, 13 Aug 2012 15:13:16 -0400 Received: from e28smtp01.in.ibm.com ([122.248.162.1]:42770) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T104g-0001xf-C8 for qemu-devel@nongnu.org; Mon, 13 Aug 2012 15:13:14 -0400 Received: from /spool/local by e28smtp01.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 14 Aug 2012 00:43:05 +0530 Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q7DJD2DY37093610 for ; Tue, 14 Aug 2012 00:43:02 +0530 Received: from d28av01.in.ibm.com (loopback [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q7E0ga4X024407 for ; Tue, 14 Aug 2012 06:12:36 +0530 Message-ID: <502951BC.1090400@linux.vnet.ibm.com> Date: Tue, 14 Aug 2012 00:43:00 +0530 From: Harsh Bora MIME-Version: 1.0 References: <1344426944-7638-1-git-send-email-pbonzini@redhat.com> <1344426944-7638-3-git-send-email-pbonzini@redhat.com> In-Reply-To: <1344426944-7638-3-git-send-email-pbonzini@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] 9p-synth: use mutex on read-side List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, aneesh.kumar@linux.vnet.ibm.com 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 > --- > 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); >