* [Linux-kernel-mentees] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking @ 2020-01-15 12:41 madhuparnabhowmik04 2020-01-15 13:56 ` Wei Liu 0 siblings, 1 reply; 5+ messages in thread From: madhuparnabhowmik04 @ 2020-01-15 12:41 UTC (permalink / raw) To: wei.liu, paul, davem Cc: paulmck, netdev, linux-kernel, joel, xen-devel, linux-kernel-mentees From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> list_for_each_entry_rcu has built-in RCU and lock checking. Pass cond argument to list_for_each_entry_rcu. Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> --- drivers/net/xen-netback/hash.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/net/xen-netback/hash.c b/drivers/net/xen-netback/hash.c index 10d580c3dea3..30709bc9d170 100644 --- a/drivers/net/xen-netback/hash.c +++ b/drivers/net/xen-netback/hash.c @@ -51,7 +51,8 @@ static void xenvif_add_hash(struct xenvif *vif, const u8 *tag, found = false; oldest = NULL; - list_for_each_entry_rcu(entry, &vif->hash.cache.list, link) { + list_for_each_entry_rcu(entry, &vif->hash.cache.list, link, + lockdep_is_held(&vif->hash.cache.lock)) { /* Make sure we don't add duplicate entries */ if (entry->len == len && memcmp(entry->tag, tag, len) == 0) @@ -102,7 +103,8 @@ static void xenvif_flush_hash(struct xenvif *vif) spin_lock_irqsave(&vif->hash.cache.lock, flags); - list_for_each_entry_rcu(entry, &vif->hash.cache.list, link) { + list_for_each_entry_rcu(entry, &vif->hash.cache.list, link, + lockdep_is_held(&vif->hash.cache.lock)) { list_del_rcu(&entry->link); vif->hash.cache.count--; kfree_rcu(entry, rcu); -- 2.17.1 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking 2020-01-15 12:41 [Linux-kernel-mentees] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking madhuparnabhowmik04 @ 2020-01-15 13:56 ` Wei Liu 2020-01-15 14:06 ` Madhuparna Bhowmik 0 siblings, 1 reply; 5+ messages in thread From: Wei Liu @ 2020-01-15 13:56 UTC (permalink / raw) To: madhuparnabhowmik04 Cc: wei.liu, paulmck, paul, netdev, linux-kernel, joel, xen-devel, linux-kernel-mentees, davem Thanks for the patch. There is a typo in the subject line. It should say xen-netback, not xen-netbank. On Wed, Jan 15, 2020 at 06:11:28PM +0530, madhuparnabhowmik04@gmail.com wrote: > From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> > > list_for_each_entry_rcu has built-in RCU and lock checking. > Pass cond argument to list_for_each_entry_rcu. > > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> > --- > drivers/net/xen-netback/hash.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/xen-netback/hash.c b/drivers/net/xen-netback/hash.c > index 10d580c3dea3..30709bc9d170 100644 > --- a/drivers/net/xen-netback/hash.c > +++ b/drivers/net/xen-netback/hash.c > @@ -51,7 +51,8 @@ static void xenvif_add_hash(struct xenvif *vif, const u8 *tag, > > found = false; > oldest = NULL; > - list_for_each_entry_rcu(entry, &vif->hash.cache.list, link) { > + list_for_each_entry_rcu(entry, &vif->hash.cache.list, link, > + lockdep_is_held(&vif->hash.cache.lock)) { There are probably too many tabs here. Indentation looks wrong. The surrounding code makes it pretty clear that the lock is already held by the time list_for_each_entry_rcu is called, yet the checking involved in lockdep_is_held is not trivial, so I'm afraid I don't consider this a strict improvement over the existing code. If there is something I misunderstood, let me know. Wei. > /* Make sure we don't add duplicate entries */ > if (entry->len == len && > memcmp(entry->tag, tag, len) == 0) > @@ -102,7 +103,8 @@ static void xenvif_flush_hash(struct xenvif *vif) > > spin_lock_irqsave(&vif->hash.cache.lock, flags); > > - list_for_each_entry_rcu(entry, &vif->hash.cache.list, link) { > + list_for_each_entry_rcu(entry, &vif->hash.cache.list, link, > + lockdep_is_held(&vif->hash.cache.lock)) { > list_del_rcu(&entry->link); > vif->hash.cache.count--; > kfree_rcu(entry, rcu); > -- > 2.17.1 > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking 2020-01-15 13:56 ` Wei Liu @ 2020-01-15 14:06 ` Madhuparna Bhowmik 2020-01-15 15:04 ` Wei Liu 0 siblings, 1 reply; 5+ messages in thread From: Madhuparna Bhowmik @ 2020-01-15 14:06 UTC (permalink / raw) To: Wei Liu Cc: Paul E. McKenney, paul, netdev, linux-kernel, Joel Fernandes, xen-devel, linux-kernel-mentees, davem [-- Attachment #1.1: Type: text/plain, Size: 2673 bytes --] On Wed, Jan 15, 2020 at 7:26 PM Wei Liu <wei.liu@kernel.org> wrote: > Thanks for the patch. > > There is a typo in the subject line. It should say xen-netback, not > xen-netbank. > > Hi, I am sorry about this, I will send this patch again. > On Wed, Jan 15, 2020 at 06:11:28PM +0530, madhuparnabhowmik04@gmail.com > wrote: > > From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> > > > > list_for_each_entry_rcu has built-in RCU and lock checking. > > Pass cond argument to list_for_each_entry_rcu. > > > > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> > > --- > > drivers/net/xen-netback/hash.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/xen-netback/hash.c > b/drivers/net/xen-netback/hash.c > > index 10d580c3dea3..30709bc9d170 100644 > > --- a/drivers/net/xen-netback/hash.c > > +++ b/drivers/net/xen-netback/hash.c > > @@ -51,7 +51,8 @@ static void xenvif_add_hash(struct xenvif *vif, const > u8 *tag, > > > > found = false; > > oldest = NULL; > > - list_for_each_entry_rcu(entry, &vif->hash.cache.list, link) { > > + list_for_each_entry_rcu(entry, &vif->hash.cache.list, link, > > + > lockdep_is_held(&vif->hash.cache.lock)) { > > There are probably too many tabs here. Indentation looks wrong. > > I will correct this when I resend this patch. > The surrounding code makes it pretty clear that the lock is already held > by the time list_for_each_entry_rcu is called, yet the checking involved > in lockdep_is_held is not trivial, so I'm afraid I don't consider this a > strict improvement over the existing code. > > Actually, we want to make CONFIG_PROVE_LIST_RCU enabled by default. And if the cond argument is not passed when the usage of list_for_each_entry_rcu() is outside of rcu_read_lock(), it will lead to a false positive. Therefore, I think this patch is required. Let me know if you have any objections. Thank you, Madhuparna > If there is something I misunderstood, let me know. > > Wei. > > > /* Make sure we don't add duplicate entries */ > > if (entry->len == len && > > memcmp(entry->tag, tag, len) == 0) > > @@ -102,7 +103,8 @@ static void xenvif_flush_hash(struct xenvif *vif) > > > > spin_lock_irqsave(&vif->hash.cache.lock, flags); > > > > - list_for_each_entry_rcu(entry, &vif->hash.cache.list, link) { > > + list_for_each_entry_rcu(entry, &vif->hash.cache.list, link, > > + > lockdep_is_held(&vif->hash.cache.lock)) { > > list_del_rcu(&entry->link); > > vif->hash.cache.count--; > > kfree_rcu(entry, rcu); > > -- > > 2.17.1 > > > [-- Attachment #1.2: Type: text/html, Size: 4351 bytes --] [-- Attachment #2: Type: text/plain, Size: 201 bytes --] _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking 2020-01-15 14:06 ` Madhuparna Bhowmik @ 2020-01-15 15:04 ` Wei Liu 2020-01-15 15:46 ` Madhuparna Bhowmik 0 siblings, 1 reply; 5+ messages in thread From: Wei Liu @ 2020-01-15 15:04 UTC (permalink / raw) To: Madhuparna Bhowmik Cc: Wei Liu, Paul E. McKenney, paul, netdev, linux-kernel, Joel Fernandes, xen-devel, linux-kernel-mentees, davem On Wed, Jan 15, 2020 at 07:36:38PM +0530, Madhuparna Bhowmik wrote: [...] > > > The surrounding code makes it pretty clear that the lock is already held > > by the time list_for_each_entry_rcu is called, yet the checking involved > > in lockdep_is_held is not trivial, so I'm afraid I don't consider this a > > strict improvement over the existing code. > > > > Actually, we want to make CONFIG_PROVE_LIST_RCU enabled by default. I think you meant CONFIG_PROVE_RCU_LIST. > And if the cond argument is not passed when the usage of > list_for_each_entry_rcu() > is outside of rcu_read_lock(), it will lead to a false positive. > Therefore, I think this patch is required. Fair enough. Wei. _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking 2020-01-15 15:04 ` Wei Liu @ 2020-01-15 15:46 ` Madhuparna Bhowmik 0 siblings, 0 replies; 5+ messages in thread From: Madhuparna Bhowmik @ 2020-01-15 15:46 UTC (permalink / raw) To: Wei Liu Cc: Paul E. McKenney, paul, netdev, linux-kernel, Joel Fernandes, xen-devel, linux-kernel-mentees, davem [-- Attachment #1.1: Type: text/plain, Size: 893 bytes --] On Wed, Jan 15, 2020 at 8:34 PM Wei Liu <wei.liu@kernel.org> wrote: > On Wed, Jan 15, 2020 at 07:36:38PM +0530, Madhuparna Bhowmik wrote: > [...] > > > > > The surrounding code makes it pretty clear that the lock is already > held > > > by the time list_for_each_entry_rcu is called, yet the checking > involved > > > in lockdep_is_held is not trivial, so I'm afraid I don't consider this > a > > > strict improvement over the existing code. > > > > > > Actually, we want to make CONFIG_PROVE_LIST_RCU enabled by default. > > I think you meant CONFIG_PROVE_RCU_LIST. > > I am sorry about this. Yes, I meant CONFIG_PROVE_RCU_LIST. > And if the cond argument is not passed when the usage of > > list_for_each_entry_rcu() > > is outside of rcu_read_lock(), it will lead to a false positive. > > Therefore, I think this patch is required. > > Fair enough. > > Thank you, Madhuparna > Wei. > [-- Attachment #1.2: Type: text/html, Size: 1638 bytes --] [-- Attachment #2: Type: text/plain, Size: 201 bytes --] _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-15 15:46 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-15 12:41 [Linux-kernel-mentees] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking madhuparnabhowmik04 2020-01-15 13:56 ` Wei Liu 2020-01-15 14:06 ` Madhuparna Bhowmik 2020-01-15 15:04 ` Wei Liu 2020-01-15 15:46 ` Madhuparna Bhowmik
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).