| Message ID | 20200221165141.24630-1-madhuparnabhowmik10@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: core: devlink.c: Use built-in RCU list checking | expand |
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 >
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 > >
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()
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
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 > >
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 >> >
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
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; }