From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH v7 2/3] tcm_vhost: Add helper to check if endpoint is setup Date: Thu, 18 Apr 2013 10:38:23 +0300 Message-ID: <20130418073823.GG13787@redhat.com> References: <20130417131726.GA26157@redhat.com> <1366247154-3136-3-git-send-email-asias@redhat.com> <20130418070953.GD13787@redhat.com> <20130418083230.GC13554@hj.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20130418083230.GC13554@hj.localdomain> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Asias He Cc: kvm@vger.kernel.org, virtualization@lists.linux-foundation.org, target-devel@vger.kernel.org, Stefan Hajnoczi , Paolo Bonzini List-Id: virtualization@lists.linuxfoundation.org On Thu, Apr 18, 2013 at 04:32:30PM +0800, Asias He wrote: > On Thu, Apr 18, 2013 at 10:09:53AM +0300, Michael S. Tsirkin wrote: > > On Thu, Apr 18, 2013 at 09:05:53AM +0800, Asias He wrote: > > > Signed-off-by: Asias He > > > --- > > > drivers/vhost/tcm_vhost.c | 18 ++++++++++++++++++ > > > 1 file changed, 18 insertions(+) > > > > > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c > > > index 88ebb79..8f05528 100644 > > > --- a/drivers/vhost/tcm_vhost.c > > > +++ b/drivers/vhost/tcm_vhost.c > > > @@ -111,6 +111,24 @@ static bool tcm_vhost_check_feature(struct vhost_scsi *vs, int feature) > > > return ret; > > > } > > > > > > +static bool tcm_vhost_check_endpoint(struct vhost_virtqueue *vq) > > > +{ > > > + bool ret = false; > > > + > > > + /* > > > + * We can handle the vq only after the endpoint is setup by calling the > > > + * VHOST_SCSI_SET_ENDPOINT ioctl. > > > + * > > > + * TODO: Check that we are running from vhost_worker which acts > > > + * as read-side critical section for vhost kind of RCU. > > > + * See the comments in struct vhost_virtqueue in drivers/vhost/vhost.h > > > + */ > > > + if (rcu_dereference_check(vq->private_data, 1)) > > > + ret = true; > > > + > > > + return ret; > > > +} > > > + > > > > You can't use RCU in this way. You need to actually > > derefence. Instead, just move this within vq mutex > > and do rcu_dereference_protected. document that this function > > requires vq mutex. > > Any difference with: > > vs_tpg = rcu_dereference_check(vq->private_data, 1); Yes, it's not the same. rcu_dereference_protected says "this is generally RCU but here I use it under lock". rcu_dereference_check can only be used if you really dereference later, and you don't. > if (vs_tpg) > return ture > else > return false; > > Note, tcm_vhost_check_endpoint() is called in multiple places. Having a > helper is more convenient. I'm fine with the helper, but fix the implementation and fix all callers to have vq mutex when calling. > > > static int tcm_vhost_check_true(struct se_portal_group *se_tpg) > > > { > > > return 1; > > > -- > > > 1.8.1.4 > > -- > Asias