| Message ID | 1366247154-3136-3-git-send-email-asias@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 18, 2013 at 09:05:53AM +0800, Asias He wrote: > Signed-off-by: Asias He <asias@redhat.com> > --- > 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. > static int tcm_vhost_check_true(struct se_portal_group *se_tpg) > { > return 1; > -- > 1.8.1.4 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <asias@redhat.com> > > > --- > > > 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <asias@redhat.com> > > --- > > 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); if (vs_tpg) return ture else return false; Note, tcm_vhost_check_endpoint() is called in multiple places. Having a helper is more convenient. > > static int tcm_vhost_check_true(struct se_portal_group *se_tpg) > > { > > return 1; > > -- > > 1.8.1.4
On Thu, Apr 18, 2013 at 10:38:23AM +0300, Michael S. Tsirkin wrote: > 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 <asias@redhat.com> > > > > --- > > > > 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. So you want the helper to be this? static bool tcm_vhost_check_endpoint() { struct tcm_vhost_tpg **vs_tpg; vs_tpg = rcu_dereference_check(vq->private_data, 1); if (vs_tpg) return true; else return false } And what difference code the compiler will generate with this: static bool tcm_vhost_check_endpoint() { if (rcu_dereference_check(vq->private_data, 1)) return true else return false } > > > 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. All the caller are in the vhost thread. Why need vq mutex to be taken? > > > > static int tcm_vhost_check_true(struct se_portal_group *se_tpg) > > > > { > > > > return 1; > > > > -- > > > > 1.8.1.4 > > > > -- > > Asias
On Mon, Apr 22, 2013 at 04:53:27PM +0800, Asias He wrote: > On Thu, Apr 18, 2013 at 10:38:23AM +0300, Michael S. Tsirkin wrote: > > 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 <asias@redhat.com> > > > > > --- > > > > > 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. > > So you want the helper to be this? > > static bool tcm_vhost_check_endpoint() > { > struct tcm_vhost_tpg **vs_tpg; > > vs_tpg = rcu_dereference_check(vq->private_data, 1); > if (vs_tpg) > return true; > else > return false > } > > And what difference code the compiler will generate with this: > > static bool tcm_vhost_check_endpoint() > { > > if (rcu_dereference_check(vq->private_data, 1)) > return true > else > return false > } > No, what I want is that private data is either dereferenced (not tested against NULL) or used under vq mutex. In this case the second option is the easiest and the cleanest. So move it under mutex and then test it safely without rcu. > > > > > 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. > > All the caller are in the vhost thread. Why need vq mutex to be taken? vhost thread is an implementation detail, people are experimenting with various threading strategies. In particular, code must keep working when we move it to use a workqueue. > > > > > static int tcm_vhost_check_true(struct se_portal_group *se_tpg) > > > > > { > > > > > return 1; > > > > > -- > > > > > 1.8.1.4 > > > > > > -- > > > Asias > > -- > Asias -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 22, 2013 at 04:28:07PM +0300, Michael S. Tsirkin wrote: > On Mon, Apr 22, 2013 at 04:53:27PM +0800, Asias He wrote: > > On Thu, Apr 18, 2013 at 10:38:23AM +0300, Michael S. Tsirkin wrote: > > > 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 <asias@redhat.com> > > > > > > --- > > > > > > 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. > > > > So you want the helper to be this? > > > > static bool tcm_vhost_check_endpoint() > > { > > struct tcm_vhost_tpg **vs_tpg; > > > > vs_tpg = rcu_dereference_check(vq->private_data, 1); > > if (vs_tpg) > > return true; > > else > > return false > > } > > > > And what difference code the compiler will generate with this: > > > > static bool tcm_vhost_check_endpoint() > > { > > > > if (rcu_dereference_check(vq->private_data, 1)) > > return true > > else > > return false > > } > > > > No, what I want is that private data is > either dereferenced (not tested against NULL) > or used under vq mutex. > In this case the second option is the easiest > and the cleanest. So move it under mutex > and then test it safely without rcu. Why you use it in -net? The test against NULL is under in the rcu read critical section. What is wrong with this really? > > > > > > > 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. > > > > All the caller are in the vhost thread. Why need vq mutex to be taken? > > vhost thread is an implementation detail, people are experimenting with > various threading strategies. In particular, code must > keep working when we move it to use a workqueue. Well, you should remove your vhost RCU thing first and use real RCU explicitly. Does -net keep working after vhost changes to workqueue? Since when this becomes a requirement? > > > > > > static int tcm_vhost_check_true(struct se_portal_group *se_tpg) > > > > > > { > > > > > > return 1; > > > > > > -- > > > > > > 1.8.1.4 > > > > > > > > -- > > > > Asias > > > > -- > > Asias
On Mon, Apr 22, 2013 at 11:00:19PM +0800, Asias He wrote: > On Mon, Apr 22, 2013 at 04:28:07PM +0300, Michael S. Tsirkin wrote: > > On Mon, Apr 22, 2013 at 04:53:27PM +0800, Asias He wrote: > > > On Thu, Apr 18, 2013 at 10:38:23AM +0300, Michael S. Tsirkin wrote: > > > > 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 <asias@redhat.com> > > > > > > > --- > > > > > > > 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. > > > > > > So you want the helper to be this? > > > > > > static bool tcm_vhost_check_endpoint() > > > { > > > struct tcm_vhost_tpg **vs_tpg; > > > > > > vs_tpg = rcu_dereference_check(vq->private_data, 1); > > > if (vs_tpg) > > > return true; > > > else > > > return false > > > } > > > > > > And what difference code the compiler will generate with this: > > > > > > static bool tcm_vhost_check_endpoint() > > > { > > > > > > if (rcu_dereference_check(vq->private_data, 1)) > > > return true > > > else > > > return false > > > } > > > > > > > No, what I want is that private data is > > either dereferenced (not tested against NULL) > > or used under vq mutex. > > In this case the second option is the easiest > > and the cleanest. So move it under mutex > > and then test it safely without rcu. > > Why you use it in -net? The test against NULL is under in the rcu > read critical section. What is wrong with this really? Look at the code and see that barriers in rcu_dereference assume that pointer really is dereferenced. You don't dereference it, so rcu_dereference_XXX is wrong. > > > > > > > > > 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. > > > > > > All the caller are in the vhost thread. Why need vq mutex to be taken? > > > > vhost thread is an implementation detail, people are experimenting with > > various threading strategies. In particular, code must > > keep working when we move it to use a workqueue. > > Well, you should remove your vhost RCU thing first and use real RCU > explicitly. I don't think it's useful or possible. > Does -net keep working after vhost changes to workqueue? I think so, yes. > Since when this becomes a requirement? It's the requirement to use the abstractions that are in place. Devices should use the interfaces and not assume vhost implementation, it happens to be the case but it's not guaranteed. > > > > > > > static int tcm_vhost_check_true(struct se_portal_group *se_tpg) > > > > > > > { > > > > > > > return 1; > > > > > > > -- > > > > > > > 1.8.1.4 > > > > > > > > > > -- > > > > > Asias > > > > > > -- > > > Asias > > -- > Asias -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; +} + static int tcm_vhost_check_true(struct se_portal_group *se_tpg) { return 1;
Signed-off-by: Asias He <asias@redhat.com> --- drivers/vhost/tcm_vhost.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)