Re: [Xen-devel] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking
On Wed, Jan 15, 2020 at 8:34 PM Wei Liu 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. > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking
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
Re: [Xen-devel] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking
On Wed, Jan 15, 2020 at 7:26 PM Wei Liu 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, madhuparnabhowmi...@gmail.com
> wrote:
> > From: Madhuparna Bhowmik
> >
> > 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
> > ---
> > 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, >hash.cache.list, link) {
> > + list_for_each_entry_rcu(entry, >hash.cache.list, link,
> > +
> lockdep_is_held(>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(>hash.cache.lock, flags);
> >
> > - list_for_each_entry_rcu(entry, >hash.cache.list, link) {
> > + list_for_each_entry_rcu(entry, >hash.cache.list, link,
> > +
> lockdep_is_held(>hash.cache.lock)) {
> > list_del_rcu(>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
Re: [Xen-devel] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking
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, madhuparnabhowmi...@gmail.com wrote:
> From: Madhuparna Bhowmik
>
> 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
> ---
> 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, >hash.cache.list, link) {
> + list_for_each_entry_rcu(entry, >hash.cache.list, link,
> +
> lockdep_is_held(>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(>hash.cache.lock, flags);
>
> - list_for_each_entry_rcu(entry, >hash.cache.list, link) {
> + list_for_each_entry_rcu(entry, >hash.cache.list, link,
> +
> lockdep_is_held(>hash.cache.lock)) {
> list_del_rcu(>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
