From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> To: Wei Liu <wei.liu@kernel.org> Cc: "Paul E. McKenney" <paulmck@kernel.org>, paul@xen.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Joel Fernandes <joel@joelfernandes.org>, xen-devel@lists.xenproject.org, linux-kernel-mentees@lists.linuxfoundation.org, davem@davemloft.net Subject: Re: [Linux-kernel-mentees] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking Date: Wed, 15 Jan 2020 19:36:38 +0530 [thread overview] Message-ID: <CAF65HP0q_KcrUP_50JxZL1xNc47=detHvdOzjBmuiqUtB3AwfA@mail.gmail.com> (raw) In-Reply-To: <20200115135631.edr2nrfkycppxcku@debian> [-- 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
WARNING: multiple messages have this Message-ID (diff)
From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> To: Wei Liu <wei.liu@kernel.org> Cc: "Paul E. McKenney" <paulmck@kernel.org>, paul@xen.org, netdev@vger.kernel.org, Amol Grover <frextrite@gmail.com>, linux-kernel@vger.kernel.org, Joel Fernandes <joel@joelfernandes.org>, xen-devel@lists.xenproject.org, linux-kernel-mentees@lists.linuxfoundation.org, davem@davemloft.net Subject: Re: [Xen-devel] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking Date: Wed, 15 Jan 2020 19:36:38 +0530 [thread overview] Message-ID: <CAF65HP0q_KcrUP_50JxZL1xNc47=detHvdOzjBmuiqUtB3AwfA@mail.gmail.com> (raw) In-Reply-To: <20200115135631.edr2nrfkycppxcku@debian> [-- 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
next prev parent reply other threads:[~2020-01-15 14:07 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-01-15 12:41 [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking madhuparnabhowmik04 2020-01-15 12:41 ` [Xen-devel] " madhuparnabhowmik04 2020-01-15 12:41 ` [Linux-kernel-mentees] " madhuparnabhowmik04 2020-01-15 13:56 ` Wei Liu 2020-01-15 13:56 ` [Xen-devel] " Wei Liu 2020-01-15 13:56 ` [Linux-kernel-mentees] " Wei Liu 2020-01-15 14:06 ` Madhuparna Bhowmik [this message] 2020-01-15 14:06 ` [Xen-devel] " Madhuparna Bhowmik 2020-01-15 15:04 ` Wei Liu 2020-01-15 15:04 ` [Xen-devel] " Wei Liu 2020-01-15 15:04 ` [Linux-kernel-mentees] " Wei Liu 2020-01-15 15:46 ` Madhuparna Bhowmik 2020-01-15 15:46 ` [Xen-devel] " Madhuparna Bhowmik
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAF65HP0q_KcrUP_50JxZL1xNc47=detHvdOzjBmuiqUtB3AwfA@mail.gmail.com' \ --to=madhuparnabhowmik04@gmail.com \ --cc=davem@davemloft.net \ --cc=joel@joelfernandes.org \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=paul@xen.org \ --cc=paulmck@kernel.org \ --cc=wei.liu@kernel.org \ --cc=xen-devel@lists.xenproject.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.