| Message ID | 20200115124129.5684-1-madhuparnabhowmik04@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: xen-netbank: hash.c: Use built-in RCU list checking | expand |
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 >
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 > > >
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.
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. >
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);