[PATCH] vhost: rcu annotation fixup

[PATCH] vhost: rcu annotation fixup

[Michael S. Tsirkin]

When built with rcu checks enabled, vhost triggers
bogus warnings as vhost features are read without
dev->mutex sometimes.
Fixing it properly is not trivial as vhost.h does not
know which lockdep classes it will be used under.
Disable the warning by stubbing out the check for now.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/vhost/vhost.h |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 2af44b7..2d03a31 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -173,9 +173,7 @@ static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
 {
 	unsigned acked_features;
 
-	acked_features =
-		rcu_dereference_index_check(dev->acked_features,
-					    lockdep_is_held(&dev->mutex));
+	acked_features = rcu_dereference_index_check(dev->acked_features, 1);
 	return acked_features & (1 << bit);
 }
 
-- 
1.7.3.2.91.g446ac

Re: [PATCH] vhost: rcu annotation fixup

[Paul E. McKenney]

On Tue, Jan 18, 2011 at 01:08:45PM +0200, Michael S. Tsirkin wrote:
> When built with rcu checks enabled, vhost triggers
> bogus warnings as vhost features are read without
> dev->mutex sometimes.
> Fixing it properly is not trivial as vhost.h does not
> know which lockdep classes it will be used under.
> Disable the warning by stubbing out the check for now.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/vhost/vhost.h |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 2af44b7..2d03a31 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -173,9 +173,7 @@ static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
>  {
>  	unsigned acked_features;
> 
> -	acked_features =
> -		rcu_dereference_index_check(dev->acked_features,
> -					    lockdep_is_held(&dev->mutex));
> +	acked_features = rcu_dereference_index_check(dev->acked_features, 1);

Ouch!!!

Could you please at least add a comment?

Alternatively, pass in the lock that is held and check for that?  Given
that this is a static inline, the compiler should be able to optimize
the argument away when !PROVE_RCU, correct?

							Thanx, Paul

>  	return acked_features & (1 << bit);
>  }
> 
> -- 
> 1.7.3.2.91.g446ac
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] vhost: rcu annotation fixup

[Michael S. Tsirkin]

On Tue, Jan 18, 2011 at 09:48:34AM -0800, Paul E. McKenney wrote:
> On Tue, Jan 18, 2011 at 01:08:45PM +0200, Michael S. Tsirkin wrote:
> > When built with rcu checks enabled, vhost triggers
> > bogus warnings as vhost features are read without
> > dev->mutex sometimes.
> > Fixing it properly is not trivial as vhost.h does not
> > know which lockdep classes it will be used under.
> > Disable the warning by stubbing out the check for now.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  drivers/vhost/vhost.h |    4 +---
> >  1 files changed, 1 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index 2af44b7..2d03a31 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -173,9 +173,7 @@ static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
> >  {
> >  	unsigned acked_features;
> > 
> > -	acked_features =
> > -		rcu_dereference_index_check(dev->acked_features,
> > -					    lockdep_is_held(&dev->mutex));
> > +	acked_features = rcu_dereference_index_check(dev->acked_features, 1);
> 
> Ouch!!!
> 
> Could you please at least add a comment?

Yes, OK.

> Alternatively, pass in the lock that is held and check for that?  Given
> that this is a static inline, the compiler should be able to optimize
> the argument away when !PROVE_RCU, correct?
> 
> 							Thanx, Paul

Hopefully, yes. We don't always have a lock: the idea was
to create a lockdep for these cases. But we can't pass
the pointer to that ...

> >  	return acked_features & (1 << bit);
> >  }
> > 
> > -- 
> > 1.7.3.2.91.g446ac
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] vhost: rcu annotation fixup

[Paul E. McKenney]

On Tue, Jan 18, 2011 at 07:55:00PM +0200, Michael S. Tsirkin wrote:
> On Tue, Jan 18, 2011 at 09:48:34AM -0800, Paul E. McKenney wrote:
> > On Tue, Jan 18, 2011 at 01:08:45PM +0200, Michael S. Tsirkin wrote:
> > > When built with rcu checks enabled, vhost triggers
> > > bogus warnings as vhost features are read without
> > > dev->mutex sometimes.
> > > Fixing it properly is not trivial as vhost.h does not
> > > know which lockdep classes it will be used under.
> > > Disable the warning by stubbing out the check for now.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  drivers/vhost/vhost.h |    4 +---
> > >  1 files changed, 1 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > > index 2af44b7..2d03a31 100644
> > > --- a/drivers/vhost/vhost.h
> > > +++ b/drivers/vhost/vhost.h
> > > @@ -173,9 +173,7 @@ static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
> > >  {
> > >  	unsigned acked_features;
> > > 
> > > -	acked_features =
> > > -		rcu_dereference_index_check(dev->acked_features,
> > > -					    lockdep_is_held(&dev->mutex));
> > > +	acked_features = rcu_dereference_index_check(dev->acked_features, 1);
> > 
> > Ouch!!!
> > 
> > Could you please at least add a comment?
> 
> Yes, OK.
> 
> > Alternatively, pass in the lock that is held and check for that?  Given
> > that this is a static inline, the compiler should be able to optimize
> > the argument away when !PROVE_RCU, correct?
> > 
> > 							Thanx, Paul
> 
> Hopefully, yes. We don't always have a lock: the idea was
> to create a lockdep for these cases. But we can't pass
> the pointer to that ...

I suppose you could pass a pointer to the lockdep map structure.
Not sure if this makes sense, but it would handle the situation.

Alternatively, create a helper function that checks the possibilities
and screams if none of them are in effect.

							Thanx, Paul

> > >  	return acked_features & (1 << bit);
> > >  }
> > > 
> > > -- 
> > > 1.7.3.2.91.g446ac
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] vhost: rcu annotation fixup

[Michael S. Tsirkin]

On Tue, Jan 18, 2011 at 11:02:33AM -0800, Paul E. McKenney wrote:
> On Tue, Jan 18, 2011 at 07:55:00PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Jan 18, 2011 at 09:48:34AM -0800, Paul E. McKenney wrote:
> > > On Tue, Jan 18, 2011 at 01:08:45PM +0200, Michael S. Tsirkin wrote:
> > > > When built with rcu checks enabled, vhost triggers
> > > > bogus warnings as vhost features are read without
> > > > dev->mutex sometimes.
> > > > Fixing it properly is not trivial as vhost.h does not
> > > > know which lockdep classes it will be used under.
> > > > Disable the warning by stubbing out the check for now.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  drivers/vhost/vhost.h |    4 +---
> > > >  1 files changed, 1 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > > > index 2af44b7..2d03a31 100644
> > > > --- a/drivers/vhost/vhost.h
> > > > +++ b/drivers/vhost/vhost.h
> > > > @@ -173,9 +173,7 @@ static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
> > > >  {
> > > >  	unsigned acked_features;
> > > > 
> > > > -	acked_features =
> > > > -		rcu_dereference_index_check(dev->acked_features,
> > > > -					    lockdep_is_held(&dev->mutex));
> > > > +	acked_features = rcu_dereference_index_check(dev->acked_features, 1);
> > > 
> > > Ouch!!!
> > > 
> > > Could you please at least add a comment?
> > 
> > Yes, OK.
> > 
> > > Alternatively, pass in the lock that is held and check for that?  Given
> > > that this is a static inline, the compiler should be able to optimize
> > > the argument away when !PROVE_RCU, correct?
> > > 
> > > 							Thanx, Paul
> > 
> > Hopefully, yes. We don't always have a lock: the idea was
> > to create a lockdep for these cases. But we can't pass
> > the pointer to that ...
> 
> I suppose you could pass a pointer to the lockdep map structure.
> Not sure if this makes sense, but it would handle the situation.

Will it compile with lockdep disabled too? What will the pointer be?

> Alternatively, create a helper function that checks the possibilities
> and screams if none of them are in effect.
> 
> 							Thanx, Paul

The problem here is the callee needs to know about all callers.

> > > >  	return acked_features & (1 << bit);
> > > >  }
> > > > 
> > > > -- 
> > > > 1.7.3.2.91.g446ac
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] vhost: rcu annotation fixup

[Paul E. McKenney]

On Tue, Jan 18, 2011 at 10:10:31PM +0200, Michael S. Tsirkin wrote:
> On Tue, Jan 18, 2011 at 11:02:33AM -0800, Paul E. McKenney wrote:
> > On Tue, Jan 18, 2011 at 07:55:00PM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Jan 18, 2011 at 09:48:34AM -0800, Paul E. McKenney wrote:
> > > > On Tue, Jan 18, 2011 at 01:08:45PM +0200, Michael S. Tsirkin wrote:
> > > > > When built with rcu checks enabled, vhost triggers
> > > > > bogus warnings as vhost features are read without
> > > > > dev->mutex sometimes.
> > > > > Fixing it properly is not trivial as vhost.h does not
> > > > > know which lockdep classes it will be used under.
> > > > > Disable the warning by stubbing out the check for now.
> > > > > 
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > >  drivers/vhost/vhost.h |    4 +---
> > > > >  1 files changed, 1 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > > > > index 2af44b7..2d03a31 100644
> > > > > --- a/drivers/vhost/vhost.h
> > > > > +++ b/drivers/vhost/vhost.h
> > > > > @@ -173,9 +173,7 @@ static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
> > > > >  {
> > > > >  	unsigned acked_features;
> > > > > 
> > > > > -	acked_features =
> > > > > -		rcu_dereference_index_check(dev->acked_features,
> > > > > -					    lockdep_is_held(&dev->mutex));
> > > > > +	acked_features = rcu_dereference_index_check(dev->acked_features, 1);
> > > > 
> > > > Ouch!!!
> > > > 
> > > > Could you please at least add a comment?
> > > 
> > > Yes, OK.
> > > 
> > > > Alternatively, pass in the lock that is held and check for that?  Given
> > > > that this is a static inline, the compiler should be able to optimize
> > > > the argument away when !PROVE_RCU, correct?
> > > > 
> > > > 							Thanx, Paul
> > > 
> > > Hopefully, yes. We don't always have a lock: the idea was
> > > to create a lockdep for these cases. But we can't pass
> > > the pointer to that ...
> > 
> > I suppose you could pass a pointer to the lockdep map structure.
> > Not sure if this makes sense, but it would handle the situation.
> 
> Will it compile with lockdep disabled too? What will the pointer be?

One (crude) approach would be to make the pointer void* if lockdep
is disabled.

> > Alternatively, create a helper function that checks the possibilities
> > and screams if none of them are in effect.
> > 
> > 							Thanx, Paul
> 
> The problem here is the callee needs to know about all callers.

As does the guy reading the code.  ;-)

							Thanx, Paul

> > > > >  	return acked_features & (1 << bit);
> > > > >  }
> > > > > 
> > > > > -- 
> > > > > 1.7.3.2.91.g446ac
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > > Please read the FAQ at  http://www.tux.org/lkml/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] vhost: rcu annotation fixup

[Mel Gorman]

On Tue, Jan 18, 2011 at 01:08:45PM +0200, Michael S. Tsirkin wrote:
> When built with rcu checks enabled, vhost triggers
> bogus warnings as vhost features are read without
> dev->mutex sometimes.
> Fixing it properly is not trivial as vhost.h does not
> know which lockdep classes it will be used under.
> Disable the warning by stubbing out the check for now.
> 

What is the harm in leaving the bogus warnings until the difficult fix
happens? RCU checks enabled does not seem like something that is enabled
in production. If this patch is applied, there is always the risk that
it'll be simply forgotten about.

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Re: [PATCH] vhost: rcu annotation fixup

[Michael S. Tsirkin]

On Wed, Jan 19, 2011 at 12:40:40AM +0000, Mel Gorman wrote:
> On Tue, Jan 18, 2011 at 01:08:45PM +0200, Michael S. Tsirkin wrote:
> > When built with rcu checks enabled, vhost triggers
> > bogus warnings as vhost features are read without
> > dev->mutex sometimes.
> > Fixing it properly is not trivial as vhost.h does not
> > know which lockdep classes it will be used under.
> > Disable the warning by stubbing out the check for now.
> > 
> 
> What is the harm in leaving the bogus warnings until the difficult fix
> happens? RCU checks enabled does not seem like something that is enabled
> in production.

I would like to run with rcu checks enabled sometimes to debug kvm,
which has an elaborate rcu strategy.  Bogus warnings in the log
make it easy to overlook the real ones. Further, the rcu macros
used are a form of documentation. If we have
-               rcu_dereference_index_check(dev->acked_features,
-                                           lockdep_is_held(&dev->mutex));
this means 'taken in rcu read side critical section or under mutex',

+       acked_features = rcu_dereference_index_check(dev->acked_features, 1);
means 'not checked'.

> If this patch is applied, there is always the risk that
> it'll be simply forgotten about.

Well, that's why I put in a TODO.
If there's a demand for that, I can add a Kconfig option to
trigger a warning at each unchecked rcu call in vhost-net
but I doubt it'll get a lof of use :)


> -- 
> Mel Gorman
> Part-time Phd Student                          Linux Technology Center
> University of Limerick                         IBM Dublin Software Lab