* Re: [lkp] [rhashtable] f9f51b8070: INFO: suspicious RCU usage. ] [not found] <874mfgedrp.fsf@yhuang-dev.intel.com> @ 2015-12-18 5:34 ` Herbert Xu 2015-12-18 6:24 ` rhashtable: Kill harmless RCU warning in rhashtable_walk_init Herbert Xu 0 siblings, 1 reply; 8+ messages in thread From: Herbert Xu @ 2015-12-18 5:34 UTC (permalink / raw) To: kernel test robot; +Cc: lkp, LKML, Colin Ian King, 0day robot, netdev On Fri, Dec 18, 2015 at 09:39:22AM +0800, kernel test robot wrote: > FYI, we noticed the below changes on > > https://github.com/0day-ci/linux Herbert-Xu/rhashtable-Fix-walker-list-corruption/20151216-164833 > commit f9f51b8070be3e829100614a7372b219723b864f ("rhashtable: Fix walker list corruption") > > > [ 8.933376] =============================== > [ 8.933376] =============================== > [ 8.934629] [ INFO: suspicious RCU usage. ] > [ 8.934629] [ INFO: suspicious RCU usage. ] > [ 8.935941] 4.4.0-rc3-00995-gf9f51b8 #2 Not tainted > [ 8.935941] 4.4.0-rc3-00995-gf9f51b8 #2 Not tainted > [ 8.937494] ------------------------------- > [ 8.937494] ------------------------------- > [ 8.938818] lib/rhashtable.c:504 suspicious rcu_dereference_protected() usage! > [ 8.938818] lib/rhashtable.c:504 suspicious rcu_dereference_protected() usage! This is actually a false positive because the new spin lock that we hold prevents ht->tbl from disappearing under us. So here is a patch to kill the warning with a comment. ---8<--- The commit f9f51b8070be3e829100614a7372b219723b864f ("rhashtable: Fix walker list corruption") causes a suspicious RCU usage warning because we no longer hold ht->mutex when we dereference ht->tbl. However, this is a false positive because we now hold ht->lock which also guarantees that ht->tbl won't disppear from under us. This patch kills the warning by using rcu_dereference_raw and adding a comment. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> diff --git a/lib/rhashtable.c b/lib/rhashtable.c index eb9240c..3404b06 100644 --- a/lib/rhashtable.c +++ b/lib/rhashtable.c @@ -519,7 +519,11 @@ int rhashtable_walk_init(struct rhashtable *ht, struct rhashtable_iter *iter) return -ENOMEM; spin_lock(&ht->lock); - iter->walker->tbl = rht_dereference(ht->tbl, ht); + /* We do not need RCU protection because we hold ht->lock + * which guarantees that if we see ht->tbl then it won't + * die on us. + */ + iter->walker->tbl = rcu_dereference_raw(ht->tbl); list_add(&iter->walker->list, &iter->walker->tbl->walkers); spin_unlock(&ht->lock); -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply related [flat|nested] 8+ messages in thread
* rhashtable: Kill harmless RCU warning in rhashtable_walk_init 2015-12-18 5:34 ` [lkp] [rhashtable] f9f51b8070: INFO: suspicious RCU usage. ] Herbert Xu @ 2015-12-18 6:24 ` Herbert Xu 2015-12-18 12:54 ` Eric Dumazet 0 siblings, 1 reply; 8+ messages in thread From: Herbert Xu @ 2015-12-18 6:24 UTC (permalink / raw) To: kernel test robot; +Cc: lkp, LKML, Colin Ian King, 0day robot, netdev On Fri, Dec 18, 2015 at 01:34:16PM +0800, Herbert Xu wrote: > On Fri, Dec 18, 2015 at 09:39:22AM +0800, kernel test robot wrote: > > FYI, we noticed the below changes on > > > > https://github.com/0day-ci/linux Herbert-Xu/rhashtable-Fix-walker-list-corruption/20151216-164833 > > commit f9f51b8070be3e829100614a7372b219723b864f ("rhashtable: Fix walker list corruption") > > > > [ 8.933376] =============================== > > [ 8.933376] =============================== > > [ 8.934629] [ INFO: suspicious RCU usage. ] > > [ 8.934629] [ INFO: suspicious RCU usage. ] > > [ 8.935941] 4.4.0-rc3-00995-gf9f51b8 #2 Not tainted > > [ 8.935941] 4.4.0-rc3-00995-gf9f51b8 #2 Not tainted > > [ 8.937494] ------------------------------- > > [ 8.937494] ------------------------------- > > [ 8.938818] lib/rhashtable.c:504 suspicious rcu_dereference_protected() usage! > > [ 8.938818] lib/rhashtable.c:504 suspicious rcu_dereference_protected() usage! > > This is actually a false positive because the new spin lock that > we hold prevents ht->tbl from disappearing under us. So here is > a patch to kill the warning with a comment. Resent with a proper patch subject and reported-by. ---8<--- The commit f9f51b8070be3e829100614a7372b219723b864f ("rhashtable: Fix walker list corruption") causes a suspicious RCU usage warning because we no longer hold ht->mutex when we dereference ht->tbl. However, this is a false positive because we now hold ht->lock which also guarantees that ht->tbl won't disppear from under us. This patch kills the warning by using rcu_dereference_raw and adding a comment. Reported-by: kernel test robot <ying.huang@linux.intel.com> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> diff --git a/lib/rhashtable.c b/lib/rhashtable.c index eb9240c..3404b06 100644 --- a/lib/rhashtable.c +++ b/lib/rhashtable.c @@ -519,7 +519,11 @@ int rhashtable_walk_init(struct rhashtable *ht, struct rhashtable_iter *iter) return -ENOMEM; spin_lock(&ht->lock); - iter->walker->tbl = rht_dereference(ht->tbl, ht); + /* We do not need RCU protection because we hold ht->lock + * which guarantees that if we see ht->tbl then it won't + * die on us. + */ + iter->walker->tbl = rcu_dereference_raw(ht->tbl); list_add(&iter->walker->list, &iter->walker->tbl->walkers); spin_unlock(&ht->lock); -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: rhashtable: Kill harmless RCU warning in rhashtable_walk_init 2015-12-18 6:24 ` rhashtable: Kill harmless RCU warning in rhashtable_walk_init Herbert Xu @ 2015-12-18 12:54 ` Eric Dumazet 2015-12-18 13:14 ` Herbert Xu 0 siblings, 1 reply; 8+ messages in thread From: Eric Dumazet @ 2015-12-18 12:54 UTC (permalink / raw) To: Herbert Xu Cc: kernel test robot, lkp, LKML, Colin Ian King, 0day robot, netdev On Fri, 2015-12-18 at 14:24 +0800, Herbert Xu wrote: > On Fri, Dec 18, 2015 at 01:34:16PM +0800, Herbert Xu wrote: > > On Fri, Dec 18, 2015 at 09:39:22AM +0800, kernel test robot wrote: > > > FYI, we noticed the below changes on > > > > > > https://github.com/0day-ci/linux Herbert-Xu/rhashtable-Fix-walker-list-corruption/20151216-164833 > > > commit f9f51b8070be3e829100614a7372b219723b864f ("rhashtable: Fix walker list corruption") > > > > > > [ 8.933376] =============================== > > > [ 8.933376] =============================== > > > [ 8.934629] [ INFO: suspicious RCU usage. ] > > > [ 8.934629] [ INFO: suspicious RCU usage. ] > > > [ 8.935941] 4.4.0-rc3-00995-gf9f51b8 #2 Not tainted > > > [ 8.935941] 4.4.0-rc3-00995-gf9f51b8 #2 Not tainted > > > [ 8.937494] ------------------------------- > > > [ 8.937494] ------------------------------- > > > [ 8.938818] lib/rhashtable.c:504 suspicious rcu_dereference_protected() usage! > > > [ 8.938818] lib/rhashtable.c:504 suspicious rcu_dereference_protected() usage! > > > > This is actually a false positive because the new spin lock that > > we hold prevents ht->tbl from disappearing under us. So here is > > a patch to kill the warning with a comment. > > Resent with a proper patch subject and reported-by. > > ---8<--- > The commit f9f51b8070be3e829100614a7372b219723b864f ("rhashtable: > Fix walker list corruption") causes a suspicious RCU usage warning > because we no longer hold ht->mutex when we dereference ht->tbl. > > However, this is a false positive because we now hold ht->lock > which also guarantees that ht->tbl won't disppear from under us. > > This patch kills the warning by using rcu_dereference_raw and > adding a comment. > > Reported-by: kernel test robot <ying.huang@linux.intel.com> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > diff --git a/lib/rhashtable.c b/lib/rhashtable.c > index eb9240c..3404b06 100644 > --- a/lib/rhashtable.c > +++ b/lib/rhashtable.c > @@ -519,7 +519,11 @@ int rhashtable_walk_init(struct rhashtable *ht, struct rhashtable_iter *iter) > return -ENOMEM; > > spin_lock(&ht->lock); > - iter->walker->tbl = rht_dereference(ht->tbl, ht); > + /* We do not need RCU protection because we hold ht->lock > + * which guarantees that if we see ht->tbl then it won't > + * die on us. > + */ > + iter->walker->tbl = rcu_dereference_raw(ht->tbl); You can avoid the comment by using the self documented and lockdep enabled primitive iter->walker->tbl = rcu_dereference_protected(ht->tbl, lockdep_is_held(&ht->lock)); But, storing the ht->tbl and then releasing the lock immediately after escapes RCU protection. So why do we store ht->tbl in the first place ? What exactly prevents it from disappearing after lock is released ? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: rhashtable: Kill harmless RCU warning in rhashtable_walk_init 2015-12-18 12:54 ` Eric Dumazet @ 2015-12-18 13:14 ` Herbert Xu 2015-12-18 21:27 ` David Miller 0 siblings, 1 reply; 8+ messages in thread From: Herbert Xu @ 2015-12-18 13:14 UTC (permalink / raw) To: Eric Dumazet Cc: kernel test robot, lkp, LKML, Colin Ian King, 0day robot, netdev On Fri, Dec 18, 2015 at 04:54:14AM -0800, Eric Dumazet wrote: > > You can avoid the comment by using the self documented and lockdep > enabled primitive > > iter->walker->tbl = rcu_dereference_protected(ht->tbl, > lockdep_is_held(&ht->lock)); That is just gross. I think a comment is much better in this case. If we were to have more place where ht->lock is taken and we had to do the RCU dereference on ht->tbl then we could add a helper for it. For now it's just a single place and I think a comment is the best way to deal with it. > But, storing the ht->tbl and then releasing the lock immediately after > escapes RCU protection. > > So why do we store ht->tbl in the first place ? > > What exactly prevents it from disappearing after lock is released ? We add ourselves to the walker list before we release the lock. The only entity that can destroy ht->tbl will take care of all walkers before doing so. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: rhashtable: Kill harmless RCU warning in rhashtable_walk_init 2015-12-18 13:14 ` Herbert Xu @ 2015-12-18 21:27 ` David Miller 2015-12-19 2:45 ` [PATCH v2] " Herbert Xu 0 siblings, 1 reply; 8+ messages in thread From: David Miller @ 2015-12-18 21:27 UTC (permalink / raw) To: herbert Cc: eric.dumazet, ying.huang, lkp, linux-kernel, colin.king, fengguang.wu, netdev From: Herbert Xu <herbert@gondor.apana.org.au> Date: Fri, 18 Dec 2015 21:14:08 +0800 > On Fri, Dec 18, 2015 at 04:54:14AM -0800, Eric Dumazet wrote: >> >> You can avoid the comment by using the self documented and lockdep >> enabled primitive >> >> iter->walker->tbl = rcu_dereference_protected(ht->tbl, >> lockdep_is_held(&ht->lock)); > > That is just gross. I think a comment is much better in this case. Herbert, this macro was created exactly to handle this situation, and this is what we do everywhere else in the tree. Please use it. Thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] rhashtable: Kill harmless RCU warning in rhashtable_walk_init 2015-12-18 21:27 ` David Miller @ 2015-12-19 2:45 ` Herbert Xu 2015-12-19 4:42 ` David Miller 0 siblings, 1 reply; 8+ messages in thread From: Herbert Xu @ 2015-12-19 2:45 UTC (permalink / raw) To: David Miller Cc: eric.dumazet, ying.huang, lkp, linux-kernel, colin.king, fengguang.wu, netdev On Fri, Dec 18, 2015 at 04:27:31PM -0500, David Miller wrote: > From: Herbert Xu <herbert@gondor.apana.org.au> > Date: Fri, 18 Dec 2015 21:14:08 +0800 > > > On Fri, Dec 18, 2015 at 04:54:14AM -0800, Eric Dumazet wrote: > >> > >> You can avoid the comment by using the self documented and lockdep > >> enabled primitive > >> > >> iter->walker->tbl = rcu_dereference_protected(ht->tbl, > >> lockdep_is_held(&ht->lock)); > > > > That is just gross. I think a comment is much better in this case. > > Herbert, this macro was created exactly to handle this situation, > and this is what we do everywhere else in the tree. OK. ---8<--- The commit f9f51b8070be3e829100614a7372b219723b864f ("rhashtable: Fix walker list corruption") causes a suspicious RCU usage warning because we no longer hold ht->mutex when we dereference ht->tbl. However, this is a false positive because we now hold ht->lock which also guarantees that ht->tbl won't disppear from under us. This patch kills the warning by using rcu_dereference_protected. Reported-by: kernel test robot <ying.huang@linux.intel.com> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> diff --git a/lib/rhashtable.c b/lib/rhashtable.c index eb9240c..51282f5 100644 --- a/lib/rhashtable.c +++ b/lib/rhashtable.c @@ -519,7 +519,8 @@ int rhashtable_walk_init(struct rhashtable *ht, struct rhashtable_iter *iter) return -ENOMEM; spin_lock(&ht->lock); - iter->walker->tbl = rht_dereference(ht->tbl, ht); + iter->walker->tbl = + rcu_dereference_protected(ht->tbl, lockdep_is_held(&ht->lock)); list_add(&iter->walker->list, &iter->walker->tbl->walkers); spin_unlock(&ht->lock); -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] rhashtable: Kill harmless RCU warning in rhashtable_walk_init 2015-12-19 2:45 ` [PATCH v2] " Herbert Xu @ 2015-12-19 4:42 ` David Miller 2015-12-19 4:47 ` [LKP] " Fengguang Wu 0 siblings, 1 reply; 8+ messages in thread From: David Miller @ 2015-12-19 4:42 UTC (permalink / raw) To: herbert Cc: eric.dumazet, ying.huang, lkp, linux-kernel, colin.king, fengguang.wu, netdev From: Herbert Xu <herbert@gondor.apana.org.au> Date: Sat, 19 Dec 2015 10:45:28 +0800 > On Fri, Dec 18, 2015 at 04:27:31PM -0500, David Miller wrote: >> From: Herbert Xu <herbert@gondor.apana.org.au> >> Date: Fri, 18 Dec 2015 21:14:08 +0800 >> >> > On Fri, Dec 18, 2015 at 04:54:14AM -0800, Eric Dumazet wrote: >> >> >> >> You can avoid the comment by using the self documented and lockdep >> >> enabled primitive >> >> >> >> iter->walker->tbl = rcu_dereference_protected(ht->tbl, >> >> lockdep_is_held(&ht->lock)); >> > >> > That is just gross. I think a comment is much better in this case. >> >> Herbert, this macro was created exactly to handle this situation, >> and this is what we do everywhere else in the tree. > > OK. > > ---8<--- > The commit f9f51b8070be3e829100614a7372b219723b864f ("rhashtable: > Fix walker list corruption") causes a suspicious RCU usage warning > because we no longer hold ht->mutex when we dereference ht->tbl. > > However, this is a false positive because we now hold ht->lock > which also guarantees that ht->tbl won't disppear from under us. > > This patch kills the warning by using rcu_dereference_protected. > > Reported-by: kernel test robot <ying.huang@linux.intel.com> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> The correct commti SHA1 is c6ff5268293ef98e48a99597e765ffc417e39fa5. Or at least, when I run: git show f9f51b8070be3e829100614a7372b219723b864f I get: fatal: bad object f9f51b8070be3e829100614a7372b219723b864f :-) I fixed this up and applied this, thanks! ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [LKP] [PATCH v2] rhashtable: Kill harmless RCU warning in rhashtable_walk_init 2015-12-19 4:42 ` David Miller @ 2015-12-19 4:47 ` Fengguang Wu 0 siblings, 0 replies; 8+ messages in thread From: Fengguang Wu @ 2015-12-19 4:47 UTC (permalink / raw) To: David Miller Cc: herbert, eric.dumazet, ying.huang, linux-kernel, netdev, colin.king, lkp On Fri, Dec 18, 2015 at 11:42:59PM -0500, David Miller wrote: > From: Herbert Xu <herbert@gondor.apana.org.au> > Date: Sat, 19 Dec 2015 10:45:28 +0800 > > > On Fri, Dec 18, 2015 at 04:27:31PM -0500, David Miller wrote: > >> From: Herbert Xu <herbert@gondor.apana.org.au> > >> Date: Fri, 18 Dec 2015 21:14:08 +0800 > >> > >> > On Fri, Dec 18, 2015 at 04:54:14AM -0800, Eric Dumazet wrote: > >> >> > >> >> You can avoid the comment by using the self documented and lockdep > >> >> enabled primitive > >> >> > >> >> iter->walker->tbl = rcu_dereference_protected(ht->tbl, > >> >> lockdep_is_held(&ht->lock)); > >> > > >> > That is just gross. I think a comment is much better in this case. > >> > >> Herbert, this macro was created exactly to handle this situation, > >> and this is what we do everywhere else in the tree. > > > > OK. > > > > ---8<--- > > The commit f9f51b8070be3e829100614a7372b219723b864f ("rhashtable: > > Fix walker list corruption") causes a suspicious RCU usage warning > > because we no longer hold ht->mutex when we dereference ht->tbl. > > > > However, this is a false positive because we now hold ht->lock > > which also guarantees that ht->tbl won't disppear from under us. > > > > This patch kills the warning by using rcu_dereference_protected. > > > > Reported-by: kernel test robot <ying.huang@linux.intel.com> > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > The correct commti SHA1 is c6ff5268293ef98e48a99597e765ffc417e39fa5. > > Or at least, when I run: > > git show f9f51b8070be3e829100614a7372b219723b864f > > I get: > > fatal: bad object f9f51b8070be3e829100614a7372b219723b864f > > :-) Oops, that commit comes from 0day robot :-) > https://github.com/0day-ci/linux Herbert-Xu/rhashtable-Fix-walker-list-corruption/20151216-164833 > commit f9f51b8070be3e829100614a7372b219723b864f ("rhashtable: Fix walker list corruption") commit f9f51b8070be3e829100614a7372b219723b864f Author: Herbert Xu <herbert@gondor.apana.org.au> AuthorDate: Wed Dec 16 16:45:54 2015 +0800 Commit: 0day robot <fengguang.wu@intel.com> CommitDate: Wed Dec 16 16:48:36 2015 +0800 rhashtable: Fix walker list corruption Thanks, Fengguang ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-12-19 4:47 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <874mfgedrp.fsf@yhuang-dev.intel.com>
2015-12-18 5:34 ` [lkp] [rhashtable] f9f51b8070: INFO: suspicious RCU usage. ] Herbert Xu
2015-12-18 6:24 ` rhashtable: Kill harmless RCU warning in rhashtable_walk_init Herbert Xu
2015-12-18 12:54 ` Eric Dumazet
2015-12-18 13:14 ` Herbert Xu
2015-12-18 21:27 ` David Miller
2015-12-19 2:45 ` [PATCH v2] " Herbert Xu
2015-12-19 4:42 ` David Miller
2015-12-19 4:47 ` [LKP] " Fengguang Wu
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).