<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jan 15, 2020 at 7:26 PM Wei Liu <<a href="mailto:wei.liu@kernel.org">wei.liu@kernel.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Thanks for the patch.<br>
<br>
There is a typo in the subject line. It should say xen-netback, not<br>
xen-netbank.<br>
<br></blockquote><div>Hi,</div><div><br></div><div>I am sorry about this, I will send this patch again.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
On Wed, Jan 15, 2020 at 06:11:28PM +0530, <a href="mailto:madhuparnabhowmik04@gmail.com" target="_blank">madhuparnabhowmik04@gmail.com</a> wrote:<br>
> From: Madhuparna Bhowmik <<a href="mailto:madhuparnabhowmik04@gmail.com" target="_blank">madhuparnabhowmik04@gmail.com</a>><br>
> <br>
> list_for_each_entry_rcu has built-in RCU and lock checking.<br>
> Pass cond argument to list_for_each_entry_rcu.<br>
> <br>
> Signed-off-by: Madhuparna Bhowmik <<a href="mailto:madhuparnabhowmik04@gmail.com" target="_blank">madhuparnabhowmik04@gmail.com</a>><br>
> ---<br>
>  drivers/net/xen-netback/hash.c | 6 ++++--<br>
>  1 file changed, 4 insertions(+), 2 deletions(-)<br>
> <br>
> diff --git a/drivers/net/xen-netback/hash.c b/drivers/net/xen-netback/hash.c<br>
> index 10d580c3dea3..30709bc9d170 100644<br>
> --- a/drivers/net/xen-netback/hash.c<br>
> +++ b/drivers/net/xen-netback/hash.c<br>
> @@ -51,7 +51,8 @@ static void xenvif_add_hash(struct xenvif *vif, const u8 *tag,<br>
>  <br>
>       found = false;<br>
>       oldest = NULL;<br>
> -     list_for_each_entry_rcu(entry, &vif->hash.cache.list, link) {<br>
> +     list_for_each_entry_rcu(entry, &vif->hash.cache.list, link,<br>
> +                                                     lockdep_is_held(&vif->hash.cache.lock)) {<br>
<br>
There are probably too many tabs here. Indentation looks wrong.<br>
<br></blockquote><div>I will correct this when I resend this patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
The surrounding code makes it pretty clear that the lock is already held<br>
by the time list_for_each_entry_rcu is called, yet the checking involved<br>
in lockdep_is_held is not trivial, so I'm afraid I don't consider this a<br>
strict improvement over the existing code.<br>
<br></blockquote><div>Actually,  we want to make CONFIG_PROVE_LIST_RCU enabled by default.</div><div>And if the cond argument is not passed when the usage of list_for_each_entry_rcu()</div><div>is outside of rcu_read_lock(), it will lead to a false positive.</div><div>Therefore, I think this patch is required.</div><div>Let me know if you have any objections.</div><div><br></div><div>Thank you,</div><div>Madhuparna</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
If there is something I misunderstood, let me know.<br>
<br>
Wei.<br>
<br>
>               /* Make sure we don't add duplicate entries */<br>
>               if (entry->len == len &&<br>
>                   memcmp(entry->tag, tag, len) == 0)<br>
> @@ -102,7 +103,8 @@ static void xenvif_flush_hash(struct xenvif *vif)<br>
>  <br>
>       spin_lock_irqsave(&vif->hash.cache.lock, flags);<br>
>  <br>
> -     list_for_each_entry_rcu(entry, &vif->hash.cache.list, link) {<br>
> +     list_for_each_entry_rcu(entry, &vif->hash.cache.list, link,<br>
> +                                                     lockdep_is_held(&vif->hash.cache.lock)) {<br>
>               list_del_rcu(&entry->link);<br>
>               vif->hash.cache.count--;<br>
>               kfree_rcu(entry, rcu);<br>
> -- <br>
> 2.17.1<br>
> <br>
</blockquote></div></div>