LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] net: core: devlink.c: Use built-in RCU list checking
@ 2020-02-24  9:30 madhuparnabhowmik10
  2020-02-24 10:52 ` Jiri Pirko
  0 siblings, 1 reply; 14+ messages in thread
From: madhuparnabhowmik10 @ 2020-02-24  9:30 UTC (permalink / raw)
  To: jiri, davem
  Cc: netdev, linux-kernel, joel, linux-kernel-mentees, frextrite,
	paulmck, Madhuparna Bhowmik

From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>

list_for_each_entry_rcu() has built-in RCU and lock checking.

Pass cond argument to list_for_each_entry_rcu() to silence
false lockdep warning when CONFIG_PROVE_RCU_LIST is enabled.

The devlink->lock is held when devlink_dpipe_table_find()
is called in non RCU read side section. Therefore, pass struct devlink
to devlink_dpipe_table_find() for lockdep checking.

Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
---
 net/core/devlink.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index e82750bdc496..dadf5fa79bb1 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2103,11 +2103,11 @@ static int devlink_dpipe_entry_put(struct sk_buff *skb,
 
 static struct devlink_dpipe_table *
 devlink_dpipe_table_find(struct list_head *dpipe_tables,
-			 const char *table_name)
+			 const char *table_name, struct devlink *devlink)
 {
 	struct devlink_dpipe_table *table;
-
-	list_for_each_entry_rcu(table, dpipe_tables, list) {
+	list_for_each_entry_rcu(table, dpipe_tables, list,
+				lockdep_is_held(&devlink->lock)) {
 		if (!strcmp(table->name, table_name))
 			return table;
 	}
@@ -2226,7 +2226,7 @@ static int devlink_nl_cmd_dpipe_entries_get(struct sk_buff *skb,
 
 	table_name = nla_data(info->attrs[DEVLINK_ATTR_DPIPE_TABLE_NAME]);
 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
-					 table_name);
+					 table_name, devlink);
 	if (!table)
 		return -EINVAL;
 
@@ -2382,7 +2382,7 @@ static int devlink_dpipe_table_counters_set(struct devlink *devlink,
 	struct devlink_dpipe_table *table;
 
 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
-					 table_name);
+					 table_name, devlink);
 	if (!table)
 		return -EINVAL;
 
@@ -6814,7 +6814,7 @@ bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
 
 	rcu_read_lock();
 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
-					 table_name);
+					 table_name, devlink);
 	enabled = false;
 	if (table)
 		enabled = table->counters_enabled;
@@ -6845,7 +6845,7 @@ int devlink_dpipe_table_register(struct devlink *devlink,
 
 	mutex_lock(&devlink->lock);
 
-	if (devlink_dpipe_table_find(&devlink->dpipe_table_list, table_name)) {
+	if (devlink_dpipe_table_find(&devlink->dpipe_table_list, table_name, devlink)) {
 		err = -EEXIST;
 		goto unlock;
 	}
@@ -6881,7 +6881,7 @@ void devlink_dpipe_table_unregister(struct devlink *devlink,
 
 	mutex_lock(&devlink->lock);
 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
-					 table_name);
+					 table_name, devlink);
 	if (!table)
 		goto unlock;
 	list_del_rcu(&table->list);
@@ -7038,7 +7038,7 @@ int devlink_dpipe_table_resource_set(struct devlink *devlink,
 
 	mutex_lock(&devlink->lock);
 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
-					 table_name);
+					 table_name, devlink);
 	if (!table) {
 		err = -EINVAL;
 		goto out;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking
  2020-02-24  9:30 [PATCH] net: core: devlink.c: Use built-in RCU list checking madhuparnabhowmik10
@ 2020-02-24 10:52 ` Jiri Pirko
  2020-02-25 12:09   ` Madhuparna Bhowmik
  0 siblings, 1 reply; 14+ messages in thread
From: Jiri Pirko @ 2020-02-24 10:52 UTC (permalink / raw)
  To: madhuparnabhowmik10
  Cc: jiri, davem, netdev, linux-kernel, joel, linux-kernel-mentees,
	frextrite, paulmck

Mon, Feb 24, 2020 at 10:30:13AM CET, madhuparnabhowmik10@gmail.com wrote:
>From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
>
>list_for_each_entry_rcu() has built-in RCU and lock checking.
>
>Pass cond argument to list_for_each_entry_rcu() to silence
>false lockdep warning when CONFIG_PROVE_RCU_LIST is enabled.
>
>The devlink->lock is held when devlink_dpipe_table_find()
>is called in non RCU read side section. Therefore, pass struct devlink
>to devlink_dpipe_table_find() for lockdep checking.
>
>Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
>---
> net/core/devlink.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index e82750bdc496..dadf5fa79bb1 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -2103,11 +2103,11 @@ static int devlink_dpipe_entry_put(struct sk_buff *skb,
> 
> static struct devlink_dpipe_table *
> devlink_dpipe_table_find(struct list_head *dpipe_tables,
>-			 const char *table_name)
>+			 const char *table_name, struct devlink *devlink)
> {
> 	struct devlink_dpipe_table *table;
>-
>-	list_for_each_entry_rcu(table, dpipe_tables, list) {
>+	list_for_each_entry_rcu(table, dpipe_tables, list,
>+				lockdep_is_held(&devlink->lock)) {
> 		if (!strcmp(table->name, table_name))
> 			return table;
> 	}
>@@ -2226,7 +2226,7 @@ static int devlink_nl_cmd_dpipe_entries_get(struct sk_buff *skb,
> 
> 	table_name = nla_data(info->attrs[DEVLINK_ATTR_DPIPE_TABLE_NAME]);
> 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
>-					 table_name);
>+					 table_name, devlink);
> 	if (!table)
> 		return -EINVAL;
> 
>@@ -2382,7 +2382,7 @@ static int devlink_dpipe_table_counters_set(struct devlink *devlink,
> 	struct devlink_dpipe_table *table;
> 
> 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
>-					 table_name);
>+					 table_name, devlink);
> 	if (!table)
> 		return -EINVAL;
> 
>@@ -6814,7 +6814,7 @@ bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
> 
> 	rcu_read_lock();
> 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
>-					 table_name);
>+					 table_name, devlink);
> 	enabled = false;
> 	if (table)
> 		enabled = table->counters_enabled;
>@@ -6845,7 +6845,7 @@ int devlink_dpipe_table_register(struct devlink *devlink,
> 
> 	mutex_lock(&devlink->lock);
> 
>-	if (devlink_dpipe_table_find(&devlink->dpipe_table_list, table_name)) {
>+	if (devlink_dpipe_table_find(&devlink->dpipe_table_list, table_name, devlink)) {

Run scripts/checkpatch.pl on your patch. You are breaking 80-cols limit
here.

Otherwise, the patch looks fine.

> 		err = -EEXIST;
> 		goto unlock;
> 	}
>@@ -6881,7 +6881,7 @@ void devlink_dpipe_table_unregister(struct devlink *devlink,
> 
> 	mutex_lock(&devlink->lock);
> 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
>-					 table_name);
>+					 table_name, devlink);
> 	if (!table)
> 		goto unlock;
> 	list_del_rcu(&table->list);
>@@ -7038,7 +7038,7 @@ int devlink_dpipe_table_resource_set(struct devlink *devlink,
> 
> 	mutex_lock(&devlink->lock);
> 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
>-					 table_name);
>+					 table_name, devlink);
> 	if (!table) {
> 		err = -EINVAL;
> 		goto out;
>-- 
>2.17.1
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking
  2020-02-24 10:52 ` Jiri Pirko
@ 2020-02-25 12:09   ` Madhuparna Bhowmik
  0 siblings, 0 replies; 14+ messages in thread
From: Madhuparna Bhowmik @ 2020-02-25 12:09 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: madhuparnabhowmik10, jiri, davem, netdev, linux-kernel, joel,
	linux-kernel-mentees, frextrite, paulmck

On Mon, Feb 24, 2020 at 11:52:09AM +0100, Jiri Pirko wrote:
> Mon, Feb 24, 2020 at 10:30:13AM CET, madhuparnabhowmik10@gmail.com wrote:
> >From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> >
> >list_for_each_entry_rcu() has built-in RCU and lock checking.
> >
> >Pass cond argument to list_for_each_entry_rcu() to silence
> >false lockdep warning when CONFIG_PROVE_RCU_LIST is enabled.
> >
> >The devlink->lock is held when devlink_dpipe_table_find()
> >is called in non RCU read side section. Therefore, pass struct devlink
> >to devlink_dpipe_table_find() for lockdep checking.
> >
> >Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> >---
> > net/core/devlink.c | 18 +++++++++---------
> > 1 file changed, 9 insertions(+), 9 deletions(-)
> >
> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >index e82750bdc496..dadf5fa79bb1 100644
> >--- a/net/core/devlink.c
> >+++ b/net/core/devlink.c
> >@@ -2103,11 +2103,11 @@ static int devlink_dpipe_entry_put(struct sk_buff *skb,
> > 
> > static struct devlink_dpipe_table *
> > devlink_dpipe_table_find(struct list_head *dpipe_tables,
> >-			 const char *table_name)
> >+			 const char *table_name, struct devlink *devlink)
> > {
> > 	struct devlink_dpipe_table *table;
> >-
> >-	list_for_each_entry_rcu(table, dpipe_tables, list) {
> >+	list_for_each_entry_rcu(table, dpipe_tables, list,
> >+				lockdep_is_held(&devlink->lock)) {
> > 		if (!strcmp(table->name, table_name))
> > 			return table;
> > 	}
> >@@ -2226,7 +2226,7 @@ static int devlink_nl_cmd_dpipe_entries_get(struct sk_buff *skb,
> > 
> > 	table_name = nla_data(info->attrs[DEVLINK_ATTR_DPIPE_TABLE_NAME]);
> > 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
> >-					 table_name);
> >+					 table_name, devlink);
> > 	if (!table)
> > 		return -EINVAL;
> > 
> >@@ -2382,7 +2382,7 @@ static int devlink_dpipe_table_counters_set(struct devlink *devlink,
> > 	struct devlink_dpipe_table *table;
> > 
> > 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
> >-					 table_name);
> >+					 table_name, devlink);
> > 	if (!table)
> > 		return -EINVAL;
> > 
> >@@ -6814,7 +6814,7 @@ bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
> > 
> > 	rcu_read_lock();
> > 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
> >-					 table_name);
> >+					 table_name, devlink);
> > 	enabled = false;
> > 	if (table)
> > 		enabled = table->counters_enabled;
> >@@ -6845,7 +6845,7 @@ int devlink_dpipe_table_register(struct devlink *devlink,
> > 
> > 	mutex_lock(&devlink->lock);
> > 
> >-	if (devlink_dpipe_table_find(&devlink->dpipe_table_list, table_name)) {
> >+	if (devlink_dpipe_table_find(&devlink->dpipe_table_list, table_name, devlink)) {
> 
> Run scripts/checkpatch.pl on your patch. You are breaking 80-cols limit
> here.
>
Sure, I will take care of this and send the updated patch soon.

Thank you,
Madhuparna
> Otherwise, the patch looks fine.
> 
> > 		err = -EEXIST;
> > 		goto unlock;
> > 	}
> >@@ -6881,7 +6881,7 @@ void devlink_dpipe_table_unregister(struct devlink *devlink,
> > 
> > 	mutex_lock(&devlink->lock);
> > 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
> >-					 table_name);
> >+					 table_name, devlink);
> > 	if (!table)
> > 		goto unlock;
> > 	list_del_rcu(&table->list);
> >@@ -7038,7 +7038,7 @@ int devlink_dpipe_table_resource_set(struct devlink *devlink,
> > 
> > 	mutex_lock(&devlink->lock);
> > 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
> >-					 table_name);
> >+					 table_name, devlink);
> > 	if (!table) {
> > 		err = -EINVAL;
> > 		goto out;
> >-- 
> >2.17.1
> >

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking
  2020-02-25 12:27 madhuparnabhowmik10
  2020-02-25 13:06 ` Jiri Pirko
@ 2020-02-27  0:59 ` David Miller
  1 sibling, 0 replies; 14+ messages in thread
From: David Miller @ 2020-02-27  0:59 UTC (permalink / raw)
  To: madhuparnabhowmik10
  Cc: jiri, netdev, linux-kernel, joel, linux-kernel-mentees,
	frextrite, paulmck

From: madhuparnabhowmik10@gmail.com
Date: Tue, 25 Feb 2020 17:57:45 +0530

> From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> 
> list_for_each_entry_rcu() has built-in RCU and lock checking.
> 
> Pass cond argument to list_for_each_entry_rcu() to silence
> false lockdep warning when CONFIG_PROVE_RCU_LIST is enabled.
> 
> The devlink->lock is held when devlink_dpipe_table_find()
> is called in non RCU read side section. Therefore, pass struct devlink
> to devlink_dpipe_table_find() for lockdep checking.
> 
> Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>

Applied, thank you.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking
  2020-02-25 12:27 madhuparnabhowmik10
@ 2020-02-25 13:06 ` Jiri Pirko
  2020-02-27  0:59 ` David Miller
  1 sibling, 0 replies; 14+ messages in thread
From: Jiri Pirko @ 2020-02-25 13:06 UTC (permalink / raw)
  To: madhuparnabhowmik10
  Cc: jiri, davem, netdev, linux-kernel, joel, linux-kernel-mentees,
	frextrite, paulmck

Tue, Feb 25, 2020 at 01:27:45PM CET, madhuparnabhowmik10@gmail.com wrote:
>From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
>
>list_for_each_entry_rcu() has built-in RCU and lock checking.
>
>Pass cond argument to list_for_each_entry_rcu() to silence
>false lockdep warning when CONFIG_PROVE_RCU_LIST is enabled.
>
>The devlink->lock is held when devlink_dpipe_table_find()
>is called in non RCU read side section. Therefore, pass struct devlink
>to devlink_dpipe_table_find() for lockdep checking.
>
>Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>

Reviewed-by: Jiri Pirko <jiri@mellanox.com>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH] net: core: devlink.c: Use built-in RCU list checking
@ 2020-02-25 12:27 madhuparnabhowmik10
  2020-02-25 13:06 ` Jiri Pirko
  2020-02-27  0:59 ` David Miller
  0 siblings, 2 replies; 14+ messages in thread
From: madhuparnabhowmik10 @ 2020-02-25 12:27 UTC (permalink / raw)
  To: jiri, davem
  Cc: netdev, linux-kernel, joel, linux-kernel-mentees, frextrite,
	paulmck, Madhuparna Bhowmik

From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>

list_for_each_entry_rcu() has built-in RCU and lock checking.

Pass cond argument to list_for_each_entry_rcu() to silence
false lockdep warning when CONFIG_PROVE_RCU_LIST is enabled.

The devlink->lock is held when devlink_dpipe_table_find()
is called in non RCU read side section. Therefore, pass struct devlink
to devlink_dpipe_table_find() for lockdep checking.

Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
---
 net/core/devlink.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index e82750bdc496..a92ab58304e5 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2103,11 +2103,11 @@ static int devlink_dpipe_entry_put(struct sk_buff *skb,
 
 static struct devlink_dpipe_table *
 devlink_dpipe_table_find(struct list_head *dpipe_tables,
-			 const char *table_name)
+			 const char *table_name, struct devlink *devlink)
 {
 	struct devlink_dpipe_table *table;
-
-	list_for_each_entry_rcu(table, dpipe_tables, list) {
+	list_for_each_entry_rcu(table, dpipe_tables, list,
+				lockdep_is_held(&devlink->lock)) {
 		if (!strcmp(table->name, table_name))
 			return table;
 	}
@@ -2226,7 +2226,7 @@ static int devlink_nl_cmd_dpipe_entries_get(struct sk_buff *skb,
 
 	table_name = nla_data(info->attrs[DEVLINK_ATTR_DPIPE_TABLE_NAME]);
 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
-					 table_name);
+					 table_name, devlink);
 	if (!table)
 		return -EINVAL;
 
@@ -2382,7 +2382,7 @@ static int devlink_dpipe_table_counters_set(struct devlink *devlink,
 	struct devlink_dpipe_table *table;
 
 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
-					 table_name);
+					 table_name, devlink);
 	if (!table)
 		return -EINVAL;
 
@@ -6814,7 +6814,7 @@ bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
 
 	rcu_read_lock();
 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
-					 table_name);
+					 table_name, devlink);
 	enabled = false;
 	if (table)
 		enabled = table->counters_enabled;
@@ -6845,7 +6845,8 @@ int devlink_dpipe_table_register(struct devlink *devlink,
 
 	mutex_lock(&devlink->lock);
 
-	if (devlink_dpipe_table_find(&devlink->dpipe_table_list, table_name)) {
+	if (devlink_dpipe_table_find(&devlink->dpipe_table_list, table_name,
+				     devlink)) {
 		err = -EEXIST;
 		goto unlock;
 	}
@@ -6881,7 +6882,7 @@ void devlink_dpipe_table_unregister(struct devlink *devlink,
 
 	mutex_lock(&devlink->lock);
 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
-					 table_name);
+					 table_name, devlink);
 	if (!table)
 		goto unlock;
 	list_del_rcu(&table->list);
@@ -7038,7 +7039,7 @@ int devlink_dpipe_table_resource_set(struct devlink *devlink,
 
 	mutex_lock(&devlink->lock);
 	table = devlink_dpipe_table_find(&devlink->dpipe_table_list,
-					 table_name);
+					 table_name, devlink);
 	if (!table) {
 		err = -EINVAL;
 		goto out;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking
  2020-02-21 16:51 madhuparnabhowmik10
  2020-02-21 17:20 ` Jiri Pirko
@ 2020-02-24  0:28 ` kbuild test robot
  1 sibling, 0 replies; 14+ messages in thread
From: kbuild test robot @ 2020-02-24  0:28 UTC (permalink / raw)
  To: madhuparnabhowmik10
  Cc: kbuild-all, clang-built-linux, jiri, davem, netdev, linux-kernel,
	joel, frextrite, linux-kernel-mentees, paulmck,
	Madhuparna Bhowmik

[-- Attachment #1: Type: text/plain, Size: 1848 bytes --]

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on net/master linus/master v5.6-rc2 next-20200221]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/madhuparnabhowmik10-gmail-com/net-core-devlink-c-Use-built-in-RCU-list-checking/20200223-035655
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 732a0dee501f9a693c9a711730838129f4587041
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (git://gitmirror/llvm_project 1df947ab403a9ec3bb1bf4cd83610a997dc4f3bc)
reproduce:
        # FIXME the reproduce steps for clang is not ready yet

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> net/core/devlink.c:2111:22: error: use of undeclared identifier 'devlink'
                                   lockdep_is_held(&devlink->lock)) {
                                                    ^
   1 error generated.

vim +/devlink +2111 net/core/devlink.c

  2103	
  2104	static struct devlink_dpipe_table *
  2105	devlink_dpipe_table_find(struct list_head *dpipe_tables,
  2106				 const char *table_name)
  2107	{
  2108		struct devlink_dpipe_table *table;
  2109	
  2110		list_for_each_entry_rcu(table, dpipe_tables, list,
> 2111					lockdep_is_held(&devlink->lock)) {
  2112			if (!strcmp(table->name, table_name))
  2113				return table;
  2114		}
  2115		return NULL;
  2116	}
  2117	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 72192 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking
  2020-02-23 11:03   ` Madhuparna Bhowmik
@ 2020-02-23 18:19     ` Jiri Pirko
  0 siblings, 0 replies; 14+ messages in thread
From: Jiri Pirko @ 2020-02-23 18:19 UTC (permalink / raw)
  To: Madhuparna Bhowmik
  Cc: jiri, davem, netdev, linux-kernel, joel, frextrite,
	linux-kernel-mentees, paulmck

Sun, Feb 23, 2020 at 12:03:42PM CET, madhuparnabhowmik10@gmail.com wrote:
>On Fri, Feb 21, 2020 at 06:20:08PM +0100, Jiri Pirko wrote:
>> Fri, Feb 21, 2020 at 05:51:41PM CET, madhuparnabhowmik10@gmail.com wrote:
>> >From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
>> >
>> >list_for_each_entry_rcu() has built-in RCU and lock checking.
>> >
>> >Pass cond argument to list_for_each_entry_rcu() to silence
>> >false lockdep warning when CONFIG_PROVE_RCU_LIST is enabled
>> >by default.
>> >
>> >Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
>> 
>> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
>> 
>> Thanks.
>> 
>> However, there is a callpath where not devlink lock neither rcu read is
>> taken:
>> devlink_dpipe_table_register()->devlink_dpipe_table_find()
>> I guess that was not the trace you were seeing, right?
>> 
>> 
>> >---
>> > net/core/devlink.c | 3 ++-
>> > 1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> >diff --git a/net/core/devlink.c b/net/core/devlink.c
>> >index 4c63c9a4c09e..3e8c94155d93 100644
>> >--- a/net/core/devlink.c
>> >+++ b/net/core/devlink.c
>> >@@ -2107,7 +2107,8 @@ devlink_dpipe_table_find(struct list_head *dpipe_tables,
>> > {
>> > 	struct devlink_dpipe_table *table;
>> > 
>> >-	list_for_each_entry_rcu(table, dpipe_tables, list) {
>> >+	list_for_each_entry_rcu(table, dpipe_tables, list,
>> >+				lockdep_is_held(&devlink->lock)) {
>
>Hi Jiri,
>
>I just noticed that this patch does not compile because devlink is
>not passed as an argument to devlink_dpipe_table_find() and it is not
>even global. I am not sure why I didn't encounter this error before
>sending the patch. Anyway, I am sorry about this.
>But it seems to be the right lock that should be held and checked for
>in devlink_dpipe_table_find(). 
>So will it be okay to pass devlink to devlink_dpipe_table_find()?

Sure.

>Anyway devlink_dpipe_table_find() is only called from functions
>within devlink.c.
>
>Let me know what you think about this.
>Thank you,
>Madhuparna
>
>
>> > 		if (!strcmp(table->name, table_name))
>> > 			return table;
>> > 	}
>> >-- 
>> >2.17.1
>> >

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking
  2020-02-21 17:20 ` Jiri Pirko
  2020-02-21 17:35   ` Madhuparna Bhowmik
@ 2020-02-23 11:03   ` Madhuparna Bhowmik
  2020-02-23 18:19     ` Jiri Pirko
  1 sibling, 1 reply; 14+ messages in thread
From: Madhuparna Bhowmik @ 2020-02-23 11:03 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: madhuparnabhowmik10, jiri, davem, netdev, linux-kernel, joel,
	frextrite, linux-kernel-mentees, paulmck

On Fri, Feb 21, 2020 at 06:20:08PM +0100, Jiri Pirko wrote:
> Fri, Feb 21, 2020 at 05:51:41PM CET, madhuparnabhowmik10@gmail.com wrote:
> >From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> >
> >list_for_each_entry_rcu() has built-in RCU and lock checking.
> >
> >Pass cond argument to list_for_each_entry_rcu() to silence
> >false lockdep warning when CONFIG_PROVE_RCU_LIST is enabled
> >by default.
> >
> >Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> 
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> 
> Thanks.
> 
> However, there is a callpath where not devlink lock neither rcu read is
> taken:
> devlink_dpipe_table_register()->devlink_dpipe_table_find()
> I guess that was not the trace you were seeing, right?
> 
> 
> >---
> > net/core/devlink.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >index 4c63c9a4c09e..3e8c94155d93 100644
> >--- a/net/core/devlink.c
> >+++ b/net/core/devlink.c
> >@@ -2107,7 +2107,8 @@ devlink_dpipe_table_find(struct list_head *dpipe_tables,
> > {
> > 	struct devlink_dpipe_table *table;
> > 
> >-	list_for_each_entry_rcu(table, dpipe_tables, list) {
> >+	list_for_each_entry_rcu(table, dpipe_tables, list,
> >+				lockdep_is_held(&devlink->lock)) {

Hi Jiri,

I just noticed that this patch does not compile because devlink is
not passed as an argument to devlink_dpipe_table_find() and it is not
even global. I am not sure why I didn't encounter this error before
sending the patch. Anyway, I am sorry about this.
But it seems to be the right lock that should be held and checked for
in devlink_dpipe_table_find(). 
So will it be okay to pass devlink to devlink_dpipe_table_find()?
Anyway devlink_dpipe_table_find() is only called from functions
within devlink.c.

Let me know what you think about this.
Thank you,
Madhuparna


> > 		if (!strcmp(table->name, table_name))
> > 			return table;
> > 	}
> >-- 
> >2.17.1
> >

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking
  2020-02-21 17:54     ` Jiri Pirko
@ 2020-02-21 18:00       ` Madhuparna Bhowmik
  0 siblings, 0 replies; 14+ messages in thread
From: Madhuparna Bhowmik @ 2020-02-21 18:00 UTC (permalink / raw)
  To: Jiri Pirko, y
  Cc: Madhuparna Bhowmik, jiri, davem, netdev, linux-kernel, joel,
	frextrite, linux-kernel-mentees, paulmck

On Fri, Feb 21, 2020 at 06:54:36PM +0100, Jiri Pirko wrote:
> Fri, Feb 21, 2020 at 06:35:34PM CET, madhuparnabhowmik10@gmail.com wrote:
> >On Fri, Feb 21, 2020 at 06:20:08PM +0100, Jiri Pirko wrote:
> >> Fri, Feb 21, 2020 at 05:51:41PM CET, madhuparnabhowmik10@gmail.com wrote:
> >> >From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> >> >
> >> >list_for_each_entry_rcu() has built-in RCU and lock checking.
> >> >
> >> >Pass cond argument to list_for_each_entry_rcu() to silence
> >> >false lockdep warning when CONFIG_PROVE_RCU_LIST is enabled
> >> >by default.
> >> >
> >> >Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> >> 
> >> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> >> 
> >> Thanks.
> >> 
> >> However, there is a callpath where not devlink lock neither rcu read is
> >> taken:
> >> devlink_dpipe_table_register()->devlink_dpipe_table_find()
> >>
> >Hi,
> >
> >Yes I had noticed this, but I was not sure if there is some other lock
> >which is being used.
> >
> >If yes, then can you please tell me which lock is held in this case,
> >and I can add that condition as well to list_for_each_entry_rcu() usage.
> >
> >And if no lock or rcu_read_lock is held then may be we should
> >use rcu_read_lock/unlock here.
> >
> >Let me know what you think about this.
> 
> devlink->lock should be held since the beginning of
> devlink_dpipe_table_register()
>
Alright, I will send a patch with this change soon.
Thank you for the help.

Regards,
Madhuparna

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking
  2020-02-21 17:35   ` Madhuparna Bhowmik
@ 2020-02-21 17:54     ` Jiri Pirko
  2020-02-21 18:00       ` Madhuparna Bhowmik
  0 siblings, 1 reply; 14+ messages in thread
From: Jiri Pirko @ 2020-02-21 17:54 UTC (permalink / raw)
  To: Madhuparna Bhowmik
  Cc: jiri, davem, netdev, linux-kernel, joel, frextrite,
	linux-kernel-mentees, paulmck

Fri, Feb 21, 2020 at 06:35:34PM CET, madhuparnabhowmik10@gmail.com wrote:
>On Fri, Feb 21, 2020 at 06:20:08PM +0100, Jiri Pirko wrote:
>> Fri, Feb 21, 2020 at 05:51:41PM CET, madhuparnabhowmik10@gmail.com wrote:
>> >From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
>> >
>> >list_for_each_entry_rcu() has built-in RCU and lock checking.
>> >
>> >Pass cond argument to list_for_each_entry_rcu() to silence
>> >false lockdep warning when CONFIG_PROVE_RCU_LIST is enabled
>> >by default.
>> >
>> >Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
>> 
>> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
>> 
>> Thanks.
>> 
>> However, there is a callpath where not devlink lock neither rcu read is
>> taken:
>> devlink_dpipe_table_register()->devlink_dpipe_table_find()
>>
>Hi,
>
>Yes I had noticed this, but I was not sure if there is some other lock
>which is being used.
>
>If yes, then can you please tell me which lock is held in this case,
>and I can add that condition as well to list_for_each_entry_rcu() usage.
>
>And if no lock or rcu_read_lock is held then may be we should
>use rcu_read_lock/unlock here.
>
>Let me know what you think about this.

devlink->lock should be held since the beginning of
devlink_dpipe_table_register()


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking
  2020-02-21 17:20 ` Jiri Pirko
@ 2020-02-21 17:35   ` Madhuparna Bhowmik
  2020-02-21 17:54     ` Jiri Pirko
  2020-02-23 11:03   ` Madhuparna Bhowmik
  1 sibling, 1 reply; 14+ messages in thread
From: Madhuparna Bhowmik @ 2020-02-21 17:35 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: madhuparnabhowmik10, jiri, davem, netdev, linux-kernel, joel,
	frextrite, linux-kernel-mentees, paulmck

On Fri, Feb 21, 2020 at 06:20:08PM +0100, Jiri Pirko wrote:
> Fri, Feb 21, 2020 at 05:51:41PM CET, madhuparnabhowmik10@gmail.com wrote:
> >From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> >
> >list_for_each_entry_rcu() has built-in RCU and lock checking.
> >
> >Pass cond argument to list_for_each_entry_rcu() to silence
> >false lockdep warning when CONFIG_PROVE_RCU_LIST is enabled
> >by default.
> >
> >Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> 
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> 
> Thanks.
> 
> However, there is a callpath where not devlink lock neither rcu read is
> taken:
> devlink_dpipe_table_register()->devlink_dpipe_table_find()
>
Hi,

Yes I had noticed this, but I was not sure if there is some other lock
which is being used.

If yes, then can you please tell me which lock is held in this case,
and I can add that condition as well to list_for_each_entry_rcu() usage.

And if no lock or rcu_read_lock is held then may be we should
use rcu_read_lock/unlock here.

Let me know what you think about this.

Thank you,
Madhuparna

> I guess that was not the trace you were seeing, right?
> 
> 
> >---
> > net/core/devlink.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >index 4c63c9a4c09e..3e8c94155d93 100644
> >--- a/net/core/devlink.c
> >+++ b/net/core/devlink.c
> >@@ -2107,7 +2107,8 @@ devlink_dpipe_table_find(struct list_head *dpipe_tables,
> > {
> > 	struct devlink_dpipe_table *table;
> > 
> >-	list_for_each_entry_rcu(table, dpipe_tables, list) {
> >+	list_for_each_entry_rcu(table, dpipe_tables, list,
> >+				lockdep_is_held(&devlink->lock)) {
> > 		if (!strcmp(table->name, table_name))
> > 			return table;
> > 	}
> >-- 
> >2.17.1
> >

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking
  2020-02-21 16:51 madhuparnabhowmik10
@ 2020-02-21 17:20 ` Jiri Pirko
  2020-02-21 17:35   ` Madhuparna Bhowmik
  2020-02-23 11:03   ` Madhuparna Bhowmik
  2020-02-24  0:28 ` kbuild test robot
  1 sibling, 2 replies; 14+ messages in thread
From: Jiri Pirko @ 2020-02-21 17:20 UTC (permalink / raw)
  To: madhuparnabhowmik10
  Cc: jiri, davem, netdev, linux-kernel, joel, frextrite,
	linux-kernel-mentees, paulmck

Fri, Feb 21, 2020 at 05:51:41PM CET, madhuparnabhowmik10@gmail.com wrote:
>From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
>
>list_for_each_entry_rcu() has built-in RCU and lock checking.
>
>Pass cond argument to list_for_each_entry_rcu() to silence
>false lockdep warning when CONFIG_PROVE_RCU_LIST is enabled
>by default.
>
>Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>

Reviewed-by: Jiri Pirko <jiri@mellanox.com>

Thanks.

However, there is a callpath where not devlink lock neither rcu read is
taken:
devlink_dpipe_table_register()->devlink_dpipe_table_find()

I guess that was not the trace you were seeing, right?


>---
> net/core/devlink.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 4c63c9a4c09e..3e8c94155d93 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -2107,7 +2107,8 @@ devlink_dpipe_table_find(struct list_head *dpipe_tables,
> {
> 	struct devlink_dpipe_table *table;
> 
>-	list_for_each_entry_rcu(table, dpipe_tables, list) {
>+	list_for_each_entry_rcu(table, dpipe_tables, list,
>+				lockdep_is_held(&devlink->lock)) {
> 		if (!strcmp(table->name, table_name))
> 			return table;
> 	}
>-- 
>2.17.1
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH] net: core: devlink.c: Use built-in RCU list checking
@ 2020-02-21 16:51 madhuparnabhowmik10
  2020-02-21 17:20 ` Jiri Pirko
  2020-02-24  0:28 ` kbuild test robot
  0 siblings, 2 replies; 14+ messages in thread
From: madhuparnabhowmik10 @ 2020-02-21 16:51 UTC (permalink / raw)
  To: jiri, davem
  Cc: netdev, linux-kernel, joel, frextrite, linux-kernel-mentees,
	paulmck, Madhuparna Bhowmik

From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>

list_for_each_entry_rcu() has built-in RCU and lock checking.

Pass cond argument to list_for_each_entry_rcu() to silence
false lockdep warning when CONFIG_PROVE_RCU_LIST is enabled
by default.

Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
---
 net/core/devlink.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 4c63c9a4c09e..3e8c94155d93 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2107,7 +2107,8 @@ devlink_dpipe_table_find(struct list_head *dpipe_tables,
 {
 	struct devlink_dpipe_table *table;
 
-	list_for_each_entry_rcu(table, dpipe_tables, list) {
+	list_for_each_entry_rcu(table, dpipe_tables, list,
+				lockdep_is_held(&devlink->lock)) {
 		if (!strcmp(table->name, table_name))
 			return table;
 	}
-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-02-27  0:59 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-24  9:30 [PATCH] net: core: devlink.c: Use built-in RCU list checking madhuparnabhowmik10
2020-02-24 10:52 ` Jiri Pirko
2020-02-25 12:09   ` Madhuparna Bhowmik
  -- strict thread matches above, loose matches on Subject: below --
2020-02-25 12:27 madhuparnabhowmik10
2020-02-25 13:06 ` Jiri Pirko
2020-02-27  0:59 ` David Miller
2020-02-21 16:51 madhuparnabhowmik10
2020-02-21 17:20 ` Jiri Pirko
2020-02-21 17:35   ` Madhuparna Bhowmik
2020-02-21 17:54     ` Jiri Pirko
2020-02-21 18:00       ` Madhuparna Bhowmik
2020-02-23 11:03   ` Madhuparna Bhowmik
2020-02-23 18:19     ` Jiri Pirko
2020-02-24  0:28 ` kbuild test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).