* Re: [Xen-devel] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking
@ 2020-01-15 13:56 ` Wei Liu
0 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2020-01-15 13:56 UTC (permalink / raw)
To: madhuparnabhowmik04
Cc: wei.liu, paulmck, paul, netdev, frextrite, 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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ 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
0 siblings, 0 replies; 13+ 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] 13+ messages in thread* Re: [Linux-kernel-mentees] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking
2020-01-15 13:56 ` [Linux-kernel-mentees] " Wei Liu
@ 2020-01-15 14:06 ` Madhuparna Bhowmik
-1 siblings, 0 replies; 13+ 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] 13+ messages in thread* Re: [Xen-devel] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking
@ 2020-01-15 14:06 ` Madhuparna Bhowmik
0 siblings, 0 replies; 13+ messages in thread
From: Madhuparna Bhowmik @ 2020-01-15 14:06 UTC (permalink / raw)
To: Wei Liu
Cc: Paul E. McKenney, paul, netdev, Amol Grover, 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: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking
2020-01-15 14:06 ` [Xen-devel] " Madhuparna Bhowmik
(?)
@ 2020-01-15 15:04 ` Wei Liu
-1 siblings, 0 replies; 13+ messages in thread
From: Wei Liu @ 2020-01-15 15:04 UTC (permalink / raw)
To: Madhuparna Bhowmik
Cc: Wei Liu, paul, davem, xen-devel, netdev, linux-kernel,
Paul E. McKenney, Joel Fernandes, Amol Grover,
linux-kernel-mentees
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Xen-devel] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking
@ 2020-01-15 15:04 ` Wei Liu
0 siblings, 0 replies; 13+ 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, Amol Grover,
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ 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
0 siblings, 0 replies; 13+ 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] 13+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking
2020-01-15 15:04 ` [Linux-kernel-mentees] " Wei Liu
@ 2020-01-15 15:46 ` Madhuparna Bhowmik
-1 siblings, 0 replies; 13+ 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] 13+ messages in thread
* Re: [Xen-devel] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking
@ 2020-01-15 15:46 ` Madhuparna Bhowmik
0 siblings, 0 replies; 13+ messages in thread
From: Madhuparna Bhowmik @ 2020-01-15 15:46 UTC (permalink / raw)
To: Wei Liu
Cc: Paul E. McKenney, paul, netdev, Amol Grover, 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: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread