From: Madhuparna Bhowmik <[email protected]>
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 <[email protected]>
---
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
Fri, Feb 21, 2020 at 05:51:41PM CET, [email protected] wrote:
>From: Madhuparna Bhowmik <[email protected]>
>
>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 <[email protected]>
Reviewed-by: Jiri Pirko <[email protected]>
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
>
On Fri, Feb 21, 2020 at 06:20:08PM +0100, Jiri Pirko wrote:
> Fri, Feb 21, 2020 at 05:51:41PM CET, [email protected] wrote:
> >From: Madhuparna Bhowmik <[email protected]>
> >
> >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 <[email protected]>
>
> Reviewed-by: Jiri Pirko <[email protected]>
>
> 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
> >
Fri, Feb 21, 2020 at 06:35:34PM CET, [email protected] wrote:
>On Fri, Feb 21, 2020 at 06:20:08PM +0100, Jiri Pirko wrote:
>> Fri, Feb 21, 2020 at 05:51:41PM CET, [email protected] wrote:
>> >From: Madhuparna Bhowmik <[email protected]>
>> >
>> >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 <[email protected]>
>>
>> Reviewed-by: Jiri Pirko <[email protected]>
>>
>> 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()
On Fri, Feb 21, 2020 at 06:54:36PM +0100, Jiri Pirko wrote:
> Fri, Feb 21, 2020 at 06:35:34PM CET, [email protected] wrote:
> >On Fri, Feb 21, 2020 at 06:20:08PM +0100, Jiri Pirko wrote:
> >> Fri, Feb 21, 2020 at 05:51:41PM CET, [email protected] wrote:
> >> >From: Madhuparna Bhowmik <[email protected]>
> >> >
> >> >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 <[email protected]>
> >>
> >> Reviewed-by: Jiri Pirko <[email protected]>
> >>
> >> 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
On Fri, Feb 21, 2020 at 06:20:08PM +0100, Jiri Pirko wrote:
> Fri, Feb 21, 2020 at 05:51:41PM CET, [email protected] wrote:
> >From: Madhuparna Bhowmik <[email protected]>
> >
> >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 <[email protected]>
>
> Reviewed-by: Jiri Pirko <[email protected]>
>
> 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
> >
Sun, Feb 23, 2020 at 12:03:42PM CET, [email protected] wrote:
>On Fri, Feb 21, 2020 at 06:20:08PM +0100, Jiri Pirko wrote:
>> Fri, Feb 21, 2020 at 05:51:41PM CET, [email protected] wrote:
>> >From: Madhuparna Bhowmik <[email protected]>
>> >
>> >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 <[email protected]>
>>
>> Reviewed-by: Jiri Pirko <[email protected]>
>>
>> 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
>> >
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 <[email protected]>
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/[email protected]