[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking
- To: Wei Liu <wei.liu@xxxxxxxxxx>
- From: Madhuparna Bhowmik <madhuparnabhowmik04@xxxxxxxxx>
- Date: Wed, 15 Jan 2020 19:36:38 +0530
- Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxx>, paul@xxxxxxx, netdev@xxxxxxxxxxxxxxx, Amol Grover <frextrite@xxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx, Joel Fernandes <joel@xxxxxxxxxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, linux-kernel-mentees@xxxxxxxxxxxxxxxxxxxxxxxxx, davem@xxxxxxxxxxxxx
- Delivery-date: Wed, 15 Jan 2020 14:06:58 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
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@xxxxxxxxx wrote:
> From: Madhuparna Bhowmik <madhuparnabhowmik04@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|