[PATCH] net: core: devlink.c: Use built-in RCU list checking

[PATCH] net: core: devlink.c: Use built-in RCU list checking

[madhuparnabhowmik10]

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

Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking

[Jiri Pirko]

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>

Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking

[David Miller]

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.

[PATCH] net: core: devlink.c: Use built-in RCU list checking

[madhuparnabhowmik10]

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

Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking

[Jiri Pirko]

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
>

Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking

[Madhuparna Bhowmik]

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
> >

[PATCH] net: core: devlink.c: Use built-in RCU list checking

[madhuparnabhowmik10]

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

Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking

[Jiri Pirko]

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
>

Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking

[Madhuparna Bhowmik]

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
> >

Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking

[Jiri Pirko]

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()

Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking

[Madhuparna Bhowmik]

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

Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking

[Madhuparna Bhowmik]

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
> >

Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking

[Jiri Pirko]

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
>> >

Re: [PATCH] net: core: devlink.c: Use built-in RCU list checking

[kbuild test robot]

[-- 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 --]