* [Linux-kernel-mentees] [PATCH v2] drivers: char: ipmi: ipmi_msghandler: Pass lockdep expression to RCU lists @ 2020-01-10 16:47 Amol Grover 2020-01-12 5:25 ` kbuild test robot 0 siblings, 1 reply; 9+ messages in thread From: Amol Grover @ 2020-01-10 16:47 UTC (permalink / raw) To: Corey Minyard, Arnd Bergmann, Greg Kroah-Hartman Cc: Paul E . McKenney, linux-kernel, Joel Fernandes, openipmi-developer, linux-kernel-mentees intf->cmd_rcvrs is traversed with list_for_each_entry_rcu outside an RCU read-side critical section but under the protection of intf->cmd_rcvrs_mutex. ipmi_interfaces is traversed using list_for_each_entry_rcu outside an RCU read-side critical section but under the protection of ipmi_interfaces_mutex. Hence, add the corresponding lockdep expression to the list traversal primitive to silence false-positive lockdep warnings, and harden RCU lists. Add macro for the corresponding lockdep expression to make the code clean and concise. Signed-off-by: Amol Grover <frextrite@gmail.com> --- v2: - Fix sparse error CHECK: Alignment should match open parenthesis drivers/char/ipmi/ipmi_msghandler.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index cad9563f8f48..1cbeacb1e8b9 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -35,6 +35,8 @@ #include <linux/nospec.h> #define IPMI_DRIVER_VERSION "39.2" +#define cmd_rcvrs_mutex_held() \ + lockdep_is_held(&intf->cmd_rcvrs_mutex) static struct ipmi_recv_msg *ipmi_alloc_recv_msg(void); static int ipmi_init_msghandler(void); @@ -618,6 +620,8 @@ static DEFINE_MUTEX(ipmidriver_mutex); static LIST_HEAD(ipmi_interfaces); static DEFINE_MUTEX(ipmi_interfaces_mutex); +#define ipmi_interfaces_mutex_held() \ + lockdep_is_held(&ipmi_interfaces_mutex) static struct srcu_struct ipmi_interfaces_srcu; /* @@ -1321,7 +1325,8 @@ static void _ipmi_destroy_user(struct ipmi_user *user) * synchronize_srcu()) then free everything in that list. */ mutex_lock(&intf->cmd_rcvrs_mutex); - list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link) { + list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, + cmd_rcvrs_mutex_held()) { if (rcvr->user == user) { list_del_rcu(&rcvr->link); rcvr->next = rcvrs; @@ -1599,7 +1604,8 @@ static struct cmd_rcvr *find_cmd_rcvr(struct ipmi_smi *intf, { struct cmd_rcvr *rcvr; - list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link) { + list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, + rcu_read_lock_held() || cmd_rcvrs_mutex_held()) { if ((rcvr->netfn == netfn) && (rcvr->cmd == cmd) && (rcvr->chans & (1 << chan))) return rcvr; @@ -1614,7 +1620,8 @@ static int is_cmd_rcvr_exclusive(struct ipmi_smi *intf, { struct cmd_rcvr *rcvr; - list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link) { + list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, + cmd_rcvrs_mutex_held()) { if ((rcvr->netfn == netfn) && (rcvr->cmd == cmd) && (rcvr->chans & chans)) return 0; @@ -3450,7 +3457,8 @@ int ipmi_add_smi(struct module *owner, /* Look for a hole in the numbers. */ i = 0; link = &ipmi_interfaces; - list_for_each_entry_rcu(tintf, &ipmi_interfaces, link) { + list_for_each_entry_rcu(tintf, &ipmi_interfaces, link, + ipmi_interfaces_mutex_held()) { if (tintf->intf_num != i) { link = &tintf->link; break; -- 2.24.1 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2] drivers: char: ipmi: ipmi_msghandler: Pass lockdep expression to RCU lists 2020-01-10 16:47 [Linux-kernel-mentees] [PATCH v2] drivers: char: ipmi: ipmi_msghandler: Pass lockdep expression to RCU lists Amol Grover @ 2020-01-12 5:25 ` kbuild test robot 2020-01-14 3:00 ` Amol Grover 0 siblings, 1 reply; 9+ messages in thread From: kbuild test robot @ 2020-01-12 5:25 UTC (permalink / raw) To: Amol Grover Cc: kbuild-all, Corey Minyard, Arnd Bergmann, Paul E . McKenney, linux-kernel, Joel Fernandes, openipmi-developer, linux-kernel-mentees [-- Attachment #1: Type: text/plain, Size: 6208 bytes --] Hi Amol, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on char-misc/char-misc-testing] [also build test WARNING on ipmi/for-next arm-soc/for-next v5.5-rc5] [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/Amol-Grover/drivers-char-ipmi-ipmi_msghandler-Pass-lockdep-expression-to-RCU-lists/20200111-081002 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 16bb7abc4a6b9defffa294e4dc28383e62a1dbcf config: x86_64-randconfig-a003-20200109 (attached as .config) compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> All warnings (new ones prefixed by >>): In file included from include/linux/export.h:43:0, from include/linux/linkage.h:7, from include/linux/kernel.h:8, from include/linux/list.h:9, from include/linux/module.h:12, from drivers/char/ipmi/ipmi_msghandler.c:17: drivers/char/ipmi/ipmi_msghandler.c: In function 'find_cmd_rcvr': include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ ^ include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var' #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) ^ >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ ^ >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ ^ include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' for (__list_check_rcu(dummy, ## cond, 0), \ ^ drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, ^ include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ ^ include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var' #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) ^ >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ ^ >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ ^ include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' for (__list_check_rcu(dummy, ## cond, 0), \ ^ drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, ^ include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ ^ include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value' (cond) ? \ ^ include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var' #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) ^ >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ ^ >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ ^ include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' for (__list_check_rcu(dummy, ## cond, 0), \ ^ drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, ^ vim +/if +263 include/linux/rcupdate.h 632ee200130899 Paul E. McKenney 2010-02-22 254 f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 255 /** f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 256 * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 257 * @c: condition to check f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 258 * @s: informative message f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 259 */ f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 260 #define RCU_LOCKDEP_WARN(c, s) \ f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 261 do { \ f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 262 static bool __section(.data.unlikely) __warned; \ f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 @263 if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 264 __warned = true; \ f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 265 lockdep_rcu_suspicious(__FILE__, __LINE__, s); \ f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 266 } \ f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 267 } while (0) f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 268 :::::: The code at line 263 was first introduced by commit :::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN() :::::: TO: Paul E. McKenney <paulmck@linux.vnet.ibm.com> :::::: CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 34460 bytes --] [-- Attachment #3: Type: text/plain, Size: 201 bytes --] _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2] drivers: char: ipmi: ipmi_msghandler: Pass lockdep expression to RCU lists 2020-01-12 5:25 ` kbuild test robot @ 2020-01-14 3:00 ` Amol Grover 2020-01-14 17:58 ` Paul E. McKenney 0 siblings, 1 reply; 9+ messages in thread From: Amol Grover @ 2020-01-14 3:00 UTC (permalink / raw) To: kbuild test robot Cc: kbuild-all, Corey Minyard, Arnd Bergmann, Paul E . McKenney, linux-kernel, Joel Fernandes, openipmi-developer, linux-kernel-mentees On Sun, Jan 12, 2020 at 01:25:58PM +0800, kbuild test robot wrote: > Hi Amol, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on char-misc/char-misc-testing] > [also build test WARNING on ipmi/for-next arm-soc/for-next v5.5-rc5] > [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/Amol-Grover/drivers-char-ipmi-ipmi_msghandler-Pass-lockdep-expression-to-RCU-lists/20200111-081002 > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 16bb7abc4a6b9defffa294e4dc28383e62a1dbcf > config: x86_64-randconfig-a003-20200109 (attached as .config) > compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010 > reproduce: > # save the attached .config to linux build tree > make ARCH=x86_64 > > If you fix the issue, kindly add following tag > Reported-by: kbuild test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>): > > In file included from include/linux/export.h:43:0, > from include/linux/linkage.h:7, > from include/linux/kernel.h:8, > from include/linux/list.h:9, > from include/linux/module.h:12, > from drivers/char/ipmi/ipmi_msghandler.c:17: > drivers/char/ipmi/ipmi_msghandler.c: In function 'find_cmd_rcvr': > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > ^ As mentioned above, RCU_LOCKDEP_WARN macro is called from __list_check_rcu with 2 parameters 1. !cond && !rcu_read_lock_any_held() 2. The message to display incase there is a lockdep warning. However, if I pass the lockdep checking condition as: list_for_each_entry_rcu(ptr, list, head, lockdep_is_held(&some_lock) || rcu_read_lock_held()) this trickles down to __list_check_rcu and then finally to RCU_LOCKDEP_WARN as (here cond is `lockdep_is_held(&some_lock) || rcu_read_lock_held()`): RCU_LOCKDEP_WARN(!lockdep_is_held(&some_lock) || rcu_read_lock_held() && !rcu_read_lock_any_held()) which according to operator precedence (I hopefully got them right) would always evaluate to true if we are in an RCU read-side critical section (without a lock), and hence, result in a false-positive lockdep warning. This could be easily solved by putting `cond` inside brackets as it is correctly done in RCU_LOCKDEP_WARN macro but not in __list_check_rcu macro. Is that so, or did I miss something? Secondly, since there is already a condition that checks for RCU read-side critical section, the extra `rcu_read_lock_held()` we supply is sort of redundant and can be skipped right? Thanks Amol > include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var' > #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > ^ > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > ^ > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > ^ > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > for (__list_check_rcu(dummy, ## cond, 0), \ > ^ > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > ^ > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > ^ > include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var' > #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > ^ > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > ^ > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > ^ > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > for (__list_check_rcu(dummy, ## cond, 0), \ > ^ > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > ^ > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > ^ > include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value' > (cond) ? \ > ^ > include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var' > #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) > ^ > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > ^ > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > ^ > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > for (__list_check_rcu(dummy, ## cond, 0), \ > ^ > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > ^ > > vim +/if +263 include/linux/rcupdate.h > > 632ee200130899 Paul E. McKenney 2010-02-22 254 > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 255 /** > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 256 * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 257 * @c: condition to check > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 258 * @s: informative message > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 259 */ > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 260 #define RCU_LOCKDEP_WARN(c, s) \ > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 261 do { \ > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 262 static bool __section(.data.unlikely) __warned; \ > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 @263 if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 264 __warned = true; \ > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 265 lockdep_rcu_suspicious(__FILE__, __LINE__, s); \ > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 266 } \ > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 267 } while (0) > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 268 > > :::::: The code at line 263 was first introduced by commit > :::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN() > > :::::: TO: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > :::::: CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2] drivers: char: ipmi: ipmi_msghandler: Pass lockdep expression to RCU lists 2020-01-14 3:00 ` Amol Grover @ 2020-01-14 17:58 ` Paul E. McKenney 2020-01-15 12:36 ` Amol Grover 0 siblings, 1 reply; 9+ messages in thread From: Paul E. McKenney @ 2020-01-14 17:58 UTC (permalink / raw) To: Amol Grover Cc: Corey Minyard, kbuild-all, kbuild test robot, Arnd Bergmann, linux-kernel, Joel Fernandes, openipmi-developer, linux-kernel-mentees On Tue, Jan 14, 2020 at 08:30:30AM +0530, Amol Grover wrote: > On Sun, Jan 12, 2020 at 01:25:58PM +0800, kbuild test robot wrote: > > Hi Amol, > > > > Thank you for the patch! Perhaps something to improve: > > > > [auto build test WARNING on char-misc/char-misc-testing] > > [also build test WARNING on ipmi/for-next arm-soc/for-next v5.5-rc5] > > [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/Amol-Grover/drivers-char-ipmi-ipmi_msghandler-Pass-lockdep-expression-to-RCU-lists/20200111-081002 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 16bb7abc4a6b9defffa294e4dc28383e62a1dbcf > > config: x86_64-randconfig-a003-20200109 (attached as .config) > > compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010 > > reproduce: > > # save the attached .config to linux build tree > > make ARCH=x86_64 > > > > If you fix the issue, kindly add following tag > > Reported-by: kbuild test robot <lkp@intel.com> > > > > All warnings (new ones prefixed by >>): > > > > In file included from include/linux/export.h:43:0, > > from include/linux/linkage.h:7, > > from include/linux/kernel.h:8, > > from include/linux/list.h:9, > > from include/linux/module.h:12, > > from drivers/char/ipmi/ipmi_msghandler.c:17: > > drivers/char/ipmi/ipmi_msghandler.c: In function 'find_cmd_rcvr': > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > ^ > > As mentioned above, RCU_LOCKDEP_WARN macro is called from > __list_check_rcu with 2 parameters > > 1. !cond && !rcu_read_lock_any_held() > 2. The message to display incase there is a lockdep warning. > > > However, if I pass the lockdep checking condition as: > > list_for_each_entry_rcu(ptr, list, head, lockdep_is_held(&some_lock) || rcu_read_lock_held()) Right, given the _rcu() suffix on the command, the rcu_read_lock_held() is implied. > this trickles down to __list_check_rcu and then finally to > RCU_LOCKDEP_WARN as (here cond is `lockdep_is_held(&some_lock) || rcu_read_lock_held()`): > > RCU_LOCKDEP_WARN(!lockdep_is_held(&some_lock) || rcu_read_lock_held() && !rcu_read_lock_any_held()) > > which according to operator precedence (I hopefully got them right) > would always evaluate to true if we are in an RCU read-side critical > section (without a lock), and hence, result in a false-positive lockdep > warning. It looks that way to me. But why not actually try it out? After all, only the running system knows for sure. And there might be some trick that we are both missing. > This could be easily solved by putting `cond` inside brackets as it is > correctly done in RCU_LOCKDEP_WARN macro but not in __list_check_rcu > macro. Is that so, or did I miss something? Again, that looks correct to me, but please check. > Secondly, since there is already a condition that checks for RCU > read-side critical section, the extra `rcu_read_lock_held()` we supply > is sort of redundant and can be skipped right? Yes, the general rule is that if the primitives ends with _rcu(), any lockdep condition will be in addition to rcu_read_lock_any_held(). So you should not need to pass RCU read-side lockdep expressions to primitives whose names end in _rcu.. Thanx, Paul > Thanks > Amol > > > include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var' > > #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > > ^ > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > ^ > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > ^ > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > for (__list_check_rcu(dummy, ## cond, 0), \ > > ^ > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > ^ > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > ^ > > include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var' > > #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > > ^ > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > ^ > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > ^ > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > for (__list_check_rcu(dummy, ## cond, 0), \ > > ^ > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > ^ > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > ^ > > include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value' > > (cond) ? \ > > ^ > > include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var' > > #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) > > ^ > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > ^ > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > ^ > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > for (__list_check_rcu(dummy, ## cond, 0), \ > > ^ > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > ^ > > > > vim +/if +263 include/linux/rcupdate.h > > > > 632ee200130899 Paul E. McKenney 2010-02-22 254 > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 255 /** > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 256 * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 257 * @c: condition to check > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 258 * @s: informative message > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 259 */ > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 260 #define RCU_LOCKDEP_WARN(c, s) \ > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 261 do { \ > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 262 static bool __section(.data.unlikely) __warned; \ > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 @263 if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 264 __warned = true; \ > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 265 lockdep_rcu_suspicious(__FILE__, __LINE__, s); \ > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 266 } \ > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 267 } while (0) > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 268 > > > > :::::: The code at line 263 was first introduced by commit > > :::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN() > > > > :::::: TO: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > :::::: CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > > --- > > 0-DAY kernel test infrastructure Open Source Technology Center > > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation > > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2] drivers: char: ipmi: ipmi_msghandler: Pass lockdep expression to RCU lists 2020-01-14 17:58 ` Paul E. McKenney @ 2020-01-15 12:36 ` Amol Grover 2020-01-15 19:32 ` Paul E. McKenney 0 siblings, 1 reply; 9+ messages in thread From: Amol Grover @ 2020-01-15 12:36 UTC (permalink / raw) To: Paul E. McKenney Cc: Corey Minyard, kbuild-all, kbuild test robot, Arnd Bergmann, linux-kernel, Joel Fernandes, openipmi-developer, linux-kernel-mentees On Tue, Jan 14, 2020 at 09:58:28AM -0800, Paul E. McKenney wrote: > On Tue, Jan 14, 2020 at 08:30:30AM +0530, Amol Grover wrote: > > On Sun, Jan 12, 2020 at 01:25:58PM +0800, kbuild test robot wrote: > > > Hi Amol, > > > > > > Thank you for the patch! Perhaps something to improve: > > > > > > [auto build test WARNING on char-misc/char-misc-testing] > > > [also build test WARNING on ipmi/for-next arm-soc/for-next v5.5-rc5] > > > [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/Amol-Grover/drivers-char-ipmi-ipmi_msghandler-Pass-lockdep-expression-to-RCU-lists/20200111-081002 > > > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 16bb7abc4a6b9defffa294e4dc28383e62a1dbcf > > > config: x86_64-randconfig-a003-20200109 (attached as .config) > > > compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010 > > > reproduce: > > > # save the attached .config to linux build tree > > > make ARCH=x86_64 > > > > > > If you fix the issue, kindly add following tag > > > Reported-by: kbuild test robot <lkp@intel.com> > > > > > > All warnings (new ones prefixed by >>): > > > > > > In file included from include/linux/export.h:43:0, > > > from include/linux/linkage.h:7, > > > from include/linux/kernel.h:8, > > > from include/linux/list.h:9, > > > from include/linux/module.h:12, > > > from drivers/char/ipmi/ipmi_msghandler.c:17: > > > drivers/char/ipmi/ipmi_msghandler.c: In function 'find_cmd_rcvr': > > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > ^ > > > > As mentioned above, RCU_LOCKDEP_WARN macro is called from > > __list_check_rcu with 2 parameters > > > > 1. !cond && !rcu_read_lock_any_held() > > 2. The message to display incase there is a lockdep warning. > > > > > > However, if I pass the lockdep checking condition as: > > > > list_for_each_entry_rcu(ptr, list, head, lockdep_is_held(&some_lock) || rcu_read_lock_held()) > > Right, given the _rcu() suffix on the command, the rcu_read_lock_held() > is implied. > > > this trickles down to __list_check_rcu and then finally to > > RCU_LOCKDEP_WARN as (here cond is `lockdep_is_held(&some_lock) || rcu_read_lock_held()`): > > > > RCU_LOCKDEP_WARN(!lockdep_is_held(&some_lock) || rcu_read_lock_held() && !rcu_read_lock_any_held()) > > > > which according to operator precedence (I hopefully got them right) > > would always evaluate to true if we are in an RCU read-side critical > > section (without a lock), and hence, result in a false-positive lockdep > > warning. > > It looks that way to me. But why not actually try it out? After all, > only the running system knows for sure. And there might be some trick > that we are both missing. > I just tested this, here are the results: Case 1: Using`lockdep_is_held() || rcu_read_lock_held()` lock RCU RSCS Splat? Actual Y Y N N Y N N N N Y Y N <= N N Y Y Similar for Case 2: Using `rcu_read_lock_held() || lockdep_is_held()` Case 3: Consider 2 locks (outside rcu_read_lock()) `lockdep_is_held(lock1) || lockdep_is_held(lock2)` lock1 lock2 Splat? Actual Y Y N* N Y N N N N Y Y N <= N N Y Y This too proves the hypothesis (I'd like to call that). *However, this shows an interesting result. When both lock1 and lock2 are held, according to the hypothesis, a splat should've occured, since the check condition (albeit faulty atm) would be: `!lockdep_is_held(lock1) || lockdep_is_held(lock2) && !rcu_read_lock_any_held()` => `!T || T && !F` => `F || T && T` => `F || T` => `T` However, there was no splat. Which led me to investigate further and I found out: 1. `rcu_read_lock_any_held()` always returns 1 even if it is outside RCU read-side CS. 2. `rcu_read_lock_held()` seems OK, returns 1 when inside and 0 when outside The kernel is compiled with PROVE_RCU=y PROVE_RCU_LIST=y Any thoughts on this? Is this intended? And should I send-in the patch for the first problem? Thanks Amol > > This could be easily solved by putting `cond` inside brackets as it is > > correctly done in RCU_LOCKDEP_WARN macro but not in __list_check_rcu > > macro. Is that so, or did I miss something? > > Again, that looks correct to me, but please check. > > > Secondly, since there is already a condition that checks for RCU > > read-side critical section, the extra `rcu_read_lock_held()` we supply > > is sort of redundant and can be skipped right? > > Yes, the general rule is that if the primitives ends with _rcu(), any > lockdep condition will be in addition to rcu_read_lock_any_held(). > So you should not need to pass RCU read-side lockdep expressions to > primitives whose names end in _rcu.. > > Thanx, Paul > > > Thanks > > Amol > > > > > include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var' > > > #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > > > ^ > > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > ^ > > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > ^ > > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > > for (__list_check_rcu(dummy, ## cond, 0), \ > > > ^ > > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > > ^ > > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > ^ > > > include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var' > > > #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > > > ^ > > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > ^ > > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > ^ > > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > > for (__list_check_rcu(dummy, ## cond, 0), \ > > > ^ > > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > > ^ > > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > ^ > > > include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value' > > > (cond) ? \ > > > ^ > > > include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var' > > > #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) > > > ^ > > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > ^ > > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > ^ > > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > > for (__list_check_rcu(dummy, ## cond, 0), \ > > > ^ > > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > > ^ > > > > > > vim +/if +263 include/linux/rcupdate.h > > > > > > 632ee200130899 Paul E. McKenney 2010-02-22 254 > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 255 /** > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 256 * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 257 * @c: condition to check > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 258 * @s: informative message > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 259 */ > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 260 #define RCU_LOCKDEP_WARN(c, s) \ > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 261 do { \ > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 262 static bool __section(.data.unlikely) __warned; \ > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 @263 if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 264 __warned = true; \ > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 265 lockdep_rcu_suspicious(__FILE__, __LINE__, s); \ > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 266 } \ > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 267 } while (0) > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 268 > > > > > > :::::: The code at line 263 was first introduced by commit > > > :::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN() > > > > > > :::::: TO: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > :::::: CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > > > > --- > > > 0-DAY kernel test infrastructure Open Source Technology Center > > > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation > > > > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2] drivers: char: ipmi: ipmi_msghandler: Pass lockdep expression to RCU lists 2020-01-15 12:36 ` Amol Grover @ 2020-01-15 19:32 ` Paul E. McKenney 2020-01-16 2:54 ` Amol Grover 0 siblings, 1 reply; 9+ messages in thread From: Paul E. McKenney @ 2020-01-15 19:32 UTC (permalink / raw) To: Amol Grover Cc: Corey Minyard, kbuild-all, kbuild test robot, Arnd Bergmann, linux-kernel, Joel Fernandes, openipmi-developer, linux-kernel-mentees On Wed, Jan 15, 2020 at 06:06:53PM +0530, Amol Grover wrote: > On Tue, Jan 14, 2020 at 09:58:28AM -0800, Paul E. McKenney wrote: > > On Tue, Jan 14, 2020 at 08:30:30AM +0530, Amol Grover wrote: > > > On Sun, Jan 12, 2020 at 01:25:58PM +0800, kbuild test robot wrote: > > > > Hi Amol, > > > > > > > > Thank you for the patch! Perhaps something to improve: > > > > > > > > [auto build test WARNING on char-misc/char-misc-testing] > > > > [also build test WARNING on ipmi/for-next arm-soc/for-next v5.5-rc5] > > > > [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/Amol-Grover/drivers-char-ipmi-ipmi_msghandler-Pass-lockdep-expression-to-RCU-lists/20200111-081002 > > > > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 16bb7abc4a6b9defffa294e4dc28383e62a1dbcf > > > > config: x86_64-randconfig-a003-20200109 (attached as .config) > > > > compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010 > > > > reproduce: > > > > # save the attached .config to linux build tree > > > > make ARCH=x86_64 > > > > > > > > If you fix the issue, kindly add following tag > > > > Reported-by: kbuild test robot <lkp@intel.com> > > > > > > > > All warnings (new ones prefixed by >>): > > > > > > > > In file included from include/linux/export.h:43:0, > > > > from include/linux/linkage.h:7, > > > > from include/linux/kernel.h:8, > > > > from include/linux/list.h:9, > > > > from include/linux/module.h:12, > > > > from drivers/char/ipmi/ipmi_msghandler.c:17: > > > > drivers/char/ipmi/ipmi_msghandler.c: In function 'find_cmd_rcvr': > > > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > ^ > > > > > > As mentioned above, RCU_LOCKDEP_WARN macro is called from > > > __list_check_rcu with 2 parameters > > > > > > 1. !cond && !rcu_read_lock_any_held() > > > 2. The message to display incase there is a lockdep warning. > > > > > > > > > However, if I pass the lockdep checking condition as: > > > > > > list_for_each_entry_rcu(ptr, list, head, lockdep_is_held(&some_lock) || rcu_read_lock_held()) > > > > Right, given the _rcu() suffix on the command, the rcu_read_lock_held() > > is implied. > > > > > this trickles down to __list_check_rcu and then finally to > > > RCU_LOCKDEP_WARN as (here cond is `lockdep_is_held(&some_lock) || rcu_read_lock_held()`): > > > > > > RCU_LOCKDEP_WARN(!lockdep_is_held(&some_lock) || rcu_read_lock_held() && !rcu_read_lock_any_held()) > > > > > > which according to operator precedence (I hopefully got them right) > > > would always evaluate to true if we are in an RCU read-side critical > > > section (without a lock), and hence, result in a false-positive lockdep > > > warning. > > > > It looks that way to me. But why not actually try it out? After all, > > only the running system knows for sure. And there might be some trick > > that we are both missing. > > > > I just tested this, here are the results: > > Case 1: Using`lockdep_is_held() || rcu_read_lock_held()` > > lock RCU RSCS Splat? Actual > Y Y N N > Y N N N > N Y Y N <= > N N Y Y > > Similar for > Case 2: Using `rcu_read_lock_held() || lockdep_is_held()` > > Case 3: Consider 2 locks (outside rcu_read_lock()) > `lockdep_is_held(lock1) || lockdep_is_held(lock2)` > > lock1 lock2 Splat? Actual > Y Y N* N > Y N N N > N Y Y N <= > N N Y Y > > This too proves the hypothesis (I'd like to call that). Very good! > *However, this shows an interesting result. When both lock1 and lock2 > are held, according to the hypothesis, a splat should've occured, since > the check condition (albeit faulty atm) would be: > > `!lockdep_is_held(lock1) || lockdep_is_held(lock2) && !rcu_read_lock_any_held()` > => `!T || T && !F` > => `F || T && T` > => `F || T` > => `T` > However, there was no splat. Which led me to investigate further and I > found out: > 1. `rcu_read_lock_any_held()` always returns 1 even if it is outside RCU > read-side CS. > 2. `rcu_read_lock_held()` seems OK, returns 1 when inside and 0 when > outside > > The kernel is compiled with > PROVE_RCU=y > PROVE_RCU_LIST=y Were you within a preempt-disable region, for example, was some other spinlock held? (A lockdep splat should give you a list of locks held.) Both of these act as generalized RCU read-side critical sections in recent kernels. > Any thoughts on this? Is this intended? And should I send-in the patch > for the first problem? Separate patches for the initial problem and fixing the macro argument, please, if that is what you are asking. Thanx, Paul > Thanks > Amol > > > > This could be easily solved by putting `cond` inside brackets as it is > > > correctly done in RCU_LOCKDEP_WARN macro but not in __list_check_rcu > > > macro. Is that so, or did I miss something? > > > > Again, that looks correct to me, but please check. > > > > > Secondly, since there is already a condition that checks for RCU > > > read-side critical section, the extra `rcu_read_lock_held()` we supply > > > is sort of redundant and can be skipped right? > > > > Yes, the general rule is that if the primitives ends with _rcu(), any > > lockdep condition will be in addition to rcu_read_lock_any_held(). > > So you should not need to pass RCU read-side lockdep expressions to > > primitives whose names end in _rcu.. > > > > Thanx, Paul > > > > > Thanks > > > Amol > > > > > > > include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var' > > > > #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > > > > ^ > > > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > > ^ > > > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > ^ > > > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > > > for (__list_check_rcu(dummy, ## cond, 0), \ > > > > ^ > > > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > > > ^ > > > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > ^ > > > > include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var' > > > > #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > > > > ^ > > > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > > ^ > > > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > ^ > > > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > > > for (__list_check_rcu(dummy, ## cond, 0), \ > > > > ^ > > > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > > > ^ > > > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > ^ > > > > include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value' > > > > (cond) ? \ > > > > ^ > > > > include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var' > > > > #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) > > > > ^ > > > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > > ^ > > > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > ^ > > > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > > > for (__list_check_rcu(dummy, ## cond, 0), \ > > > > ^ > > > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > > > ^ > > > > > > > > vim +/if +263 include/linux/rcupdate.h > > > > > > > > 632ee200130899 Paul E. McKenney 2010-02-22 254 > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 255 /** > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 256 * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 257 * @c: condition to check > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 258 * @s: informative message > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 259 */ > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 260 #define RCU_LOCKDEP_WARN(c, s) \ > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 261 do { \ > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 262 static bool __section(.data.unlikely) __warned; \ > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 @263 if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 264 __warned = true; \ > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 265 lockdep_rcu_suspicious(__FILE__, __LINE__, s); \ > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 266 } \ > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 267 } while (0) > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 268 > > > > > > > > :::::: The code at line 263 was first introduced by commit > > > > :::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN() > > > > > > > > :::::: TO: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > > :::::: CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > > > > > > --- > > > > 0-DAY kernel test infrastructure Open Source Technology Center > > > > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation > > > > > > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2] drivers: char: ipmi: ipmi_msghandler: Pass lockdep expression to RCU lists 2020-01-15 19:32 ` Paul E. McKenney @ 2020-01-16 2:54 ` Amol Grover 2020-01-16 3:21 ` Paul E. McKenney 0 siblings, 1 reply; 9+ messages in thread From: Amol Grover @ 2020-01-16 2:54 UTC (permalink / raw) To: Paul E. McKenney Cc: Corey Minyard, kbuild-all, kbuild test robot, Arnd Bergmann, linux-kernel, Joel Fernandes, openipmi-developer, linux-kernel-mentees On Wed, Jan 15, 2020 at 11:32:59AM -0800, Paul E. McKenney wrote: > On Wed, Jan 15, 2020 at 06:06:53PM +0530, Amol Grover wrote: > > On Tue, Jan 14, 2020 at 09:58:28AM -0800, Paul E. McKenney wrote: > > > On Tue, Jan 14, 2020 at 08:30:30AM +0530, Amol Grover wrote: > > > > On Sun, Jan 12, 2020 at 01:25:58PM +0800, kbuild test robot wrote: > > > > > Hi Amol, > > > > > > > > > > Thank you for the patch! Perhaps something to improve: > > > > > > > > > > [auto build test WARNING on char-misc/char-misc-testing] > > > > > [also build test WARNING on ipmi/for-next arm-soc/for-next v5.5-rc5] > > > > > [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/Amol-Grover/drivers-char-ipmi-ipmi_msghandler-Pass-lockdep-expression-to-RCU-lists/20200111-081002 > > > > > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 16bb7abc4a6b9defffa294e4dc28383e62a1dbcf > > > > > config: x86_64-randconfig-a003-20200109 (attached as .config) > > > > > compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010 > > > > > reproduce: > > > > > # save the attached .config to linux build tree > > > > > make ARCH=x86_64 > > > > > > > > > > If you fix the issue, kindly add following tag > > > > > Reported-by: kbuild test robot <lkp@intel.com> > > > > > > > > > > All warnings (new ones prefixed by >>): > > > > > > > > > > In file included from include/linux/export.h:43:0, > > > > > from include/linux/linkage.h:7, > > > > > from include/linux/kernel.h:8, > > > > > from include/linux/list.h:9, > > > > > from include/linux/module.h:12, > > > > > from drivers/char/ipmi/ipmi_msghandler.c:17: > > > > > drivers/char/ipmi/ipmi_msghandler.c: In function 'find_cmd_rcvr': > > > > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > ^ > > > > > > > > As mentioned above, RCU_LOCKDEP_WARN macro is called from > > > > __list_check_rcu with 2 parameters > > > > > > > > 1. !cond && !rcu_read_lock_any_held() > > > > 2. The message to display incase there is a lockdep warning. > > > > > > > > > > > > However, if I pass the lockdep checking condition as: > > > > > > > > list_for_each_entry_rcu(ptr, list, head, lockdep_is_held(&some_lock) || rcu_read_lock_held()) > > > > > > Right, given the _rcu() suffix on the command, the rcu_read_lock_held() > > > is implied. > > > > > > > this trickles down to __list_check_rcu and then finally to > > > > RCU_LOCKDEP_WARN as (here cond is `lockdep_is_held(&some_lock) || rcu_read_lock_held()`): > > > > > > > > RCU_LOCKDEP_WARN(!lockdep_is_held(&some_lock) || rcu_read_lock_held() && !rcu_read_lock_any_held()) > > > > > > > > which according to operator precedence (I hopefully got them right) > > > > would always evaluate to true if we are in an RCU read-side critical > > > > section (without a lock), and hence, result in a false-positive lockdep > > > > warning. > > > > > > It looks that way to me. But why not actually try it out? After all, > > > only the running system knows for sure. And there might be some trick > > > that we are both missing. > > > > > > > I just tested this, here are the results: > > > > Case 1: Using`lockdep_is_held() || rcu_read_lock_held()` > > > > lock RCU RSCS Splat? Actual > > Y Y N N > > Y N N N > > N Y Y N <= > > N N Y Y > > > > Similar for > > Case 2: Using `rcu_read_lock_held() || lockdep_is_held()` > > > > Case 3: Consider 2 locks (outside rcu_read_lock()) > > `lockdep_is_held(lock1) || lockdep_is_held(lock2)` > > > > lock1 lock2 Splat? Actual > > Y Y N* N > > Y N N N > > N Y Y N <= > > N N Y Y > > > > This too proves the hypothesis (I'd like to call that). > > Very good! > > > *However, this shows an interesting result. When both lock1 and lock2 > > are held, according to the hypothesis, a splat should've occured, since > > the check condition (albeit faulty atm) would be: > > > > `!lockdep_is_held(lock1) || lockdep_is_held(lock2) && !rcu_read_lock_any_held()` > > => `!T || T && !F` > > => `F || T && T` > > => `F || T` > > => `T` > > However, there was no splat. Which led me to investigate further and I > > found out: > > 1. `rcu_read_lock_any_held()` always returns 1 even if it is outside RCU > > read-side CS. > > 2. `rcu_read_lock_held()` seems OK, returns 1 when inside and 0 when > > outside > > > > The kernel is compiled with > > PROVE_RCU=y > > PROVE_RCU_LIST=y > > Were you within a preempt-disable region, for example, was some other > spinlock held? (A lockdep splat should give you a list of locks held.) > Both of these act as generalized RCU read-side critical sections in > recent kernels. > I'm not sure if I was inside a preempt-disable region. I just created a thread via kthread_run and ran rcu_read_lock_any_held() inside the thread, I'll investigate further and get back to you. And no-other locks were held. Here is the splat: [ 2724.551419] ============================= [ 2724.551420] WARNING: suspicious RCU usage [ 2724.551421] 5.5.0-rc5-next #15 Tainted: G OE [ 2724.551422] ----------------------------- [ 2724.551423] /home/amolg/git/lockdep_check/main.c:28 RCU-list traversed in non-reader section!! [ 2724.551424] other info that might help us debug this: [ 2724.551425] rcu_scheduler_active = 2, debug_locks = 1 [ 2724.551426] no locks held by my_thread/4275. [ 2724.551426] stack backtrace: [ 2724.551428] CPU: 2 PID: 4275 Comm: my_thread Tainted: G OE 5.5.0-rc5-next #15 [ 2724.551429] Hardware name: Gigabyte Technology Co., Ltd. Z170-D3H/Z170-D3H-CF, BIOS F21 03/06/2017 [ 2724.551430] Call Trace: [ 2724.551437] dump_stack+0x8f/0xd0 [ 2724.551440] threadfn+0x100/0x105 [main] [ 2724.551443] ? __kthread_parkme+0x48/0x60 [ 2724.551445] kthread+0xf9/0x130 [ 2724.551447] ? 0xffffffffc00d7000 [ 2724.551448] ? kthread_park+0x90/0x90 [ 2724.551451] ret_from_fork+0x3a/0x50 > > Any thoughts on this? Is this intended? And should I send-in the patch > > for the first problem? > > Separate patches for the initial problem and fixing the macro argument, > please, if that is what you are asking. > Sorry if I didn't make myself clear earlier. By `first problem` I was referring to the macro argument problem itself. I think by `initial problem` you mean this patch (drivers/char/ipmi), right? I'll make sure to send in a separate patch for the macro argument problem. Thanks Amol > Thanx, Paul > > > Thanks > > Amol > > > > > > This could be easily solved by putting `cond` inside brackets as it is > > > > correctly done in RCU_LOCKDEP_WARN macro but not in __list_check_rcu > > > > macro. Is that so, or did I miss something? > > > > > > Again, that looks correct to me, but please check. > > > > > > > Secondly, since there is already a condition that checks for RCU > > > > read-side critical section, the extra `rcu_read_lock_held()` we supply > > > > is sort of redundant and can be skipped right? > > > > > > Yes, the general rule is that if the primitives ends with _rcu(), any > > > lockdep condition will be in addition to rcu_read_lock_any_held(). > > > So you should not need to pass RCU read-side lockdep expressions to > > > primitives whose names end in _rcu.. > > > > > > Thanx, Paul > > > > > > > Thanks > > > > Amol > > > > > > > > > include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var' > > > > > #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > > > > > ^ > > > > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > > > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > > > ^ > > > > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > ^ > > > > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > > > > for (__list_check_rcu(dummy, ## cond, 0), \ > > > > > ^ > > > > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > > > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > > > > ^ > > > > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > ^ > > > > > include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var' > > > > > #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > > > > > ^ > > > > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > > > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > > > ^ > > > > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > ^ > > > > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > > > > for (__list_check_rcu(dummy, ## cond, 0), \ > > > > > ^ > > > > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > > > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > > > > ^ > > > > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > ^ > > > > > include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value' > > > > > (cond) ? \ > > > > > ^ > > > > > include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var' > > > > > #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) > > > > > ^ > > > > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > > > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > > > ^ > > > > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > ^ > > > > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > > > > for (__list_check_rcu(dummy, ## cond, 0), \ > > > > > ^ > > > > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > > > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > > > > ^ > > > > > > > > > > vim +/if +263 include/linux/rcupdate.h > > > > > > > > > > 632ee200130899 Paul E. McKenney 2010-02-22 254 > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 255 /** > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 256 * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 257 * @c: condition to check > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 258 * @s: informative message > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 259 */ > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 260 #define RCU_LOCKDEP_WARN(c, s) \ > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 261 do { \ > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 262 static bool __section(.data.unlikely) __warned; \ > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 @263 if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 264 __warned = true; \ > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 265 lockdep_rcu_suspicious(__FILE__, __LINE__, s); \ > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 266 } \ > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 267 } while (0) > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 268 > > > > > > > > > > :::::: The code at line 263 was first introduced by commit > > > > > :::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN() > > > > > > > > > > :::::: TO: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > > > :::::: CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > > > > > > > > --- > > > > > 0-DAY kernel test infrastructure Open Source Technology Center > > > > > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation > > > > > > > > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2] drivers: char: ipmi: ipmi_msghandler: Pass lockdep expression to RCU lists 2020-01-16 2:54 ` Amol Grover @ 2020-01-16 3:21 ` Paul E. McKenney 2020-01-16 16:27 ` Amol Grover 0 siblings, 1 reply; 9+ messages in thread From: Paul E. McKenney @ 2020-01-16 3:21 UTC (permalink / raw) To: Amol Grover Cc: Corey Minyard, kbuild-all, kbuild test robot, Arnd Bergmann, linux-kernel, Joel Fernandes, openipmi-developer, linux-kernel-mentees On Thu, Jan 16, 2020 at 08:24:25AM +0530, Amol Grover wrote: > On Wed, Jan 15, 2020 at 11:32:59AM -0800, Paul E. McKenney wrote: > > On Wed, Jan 15, 2020 at 06:06:53PM +0530, Amol Grover wrote: > > > On Tue, Jan 14, 2020 at 09:58:28AM -0800, Paul E. McKenney wrote: > > > > On Tue, Jan 14, 2020 at 08:30:30AM +0530, Amol Grover wrote: > > > > > On Sun, Jan 12, 2020 at 01:25:58PM +0800, kbuild test robot wrote: > > > > > > Hi Amol, > > > > > > > > > > > > Thank you for the patch! Perhaps something to improve: > > > > > > > > > > > > [auto build test WARNING on char-misc/char-misc-testing] > > > > > > [also build test WARNING on ipmi/for-next arm-soc/for-next v5.5-rc5] > > > > > > [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/Amol-Grover/drivers-char-ipmi-ipmi_msghandler-Pass-lockdep-expression-to-RCU-lists/20200111-081002 > > > > > > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 16bb7abc4a6b9defffa294e4dc28383e62a1dbcf > > > > > > config: x86_64-randconfig-a003-20200109 (attached as .config) > > > > > > compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010 > > > > > > reproduce: > > > > > > # save the attached .config to linux build tree > > > > > > make ARCH=x86_64 > > > > > > > > > > > > If you fix the issue, kindly add following tag > > > > > > Reported-by: kbuild test robot <lkp@intel.com> > > > > > > > > > > > > All warnings (new ones prefixed by >>): > > > > > > > > > > > > In file included from include/linux/export.h:43:0, > > > > > > from include/linux/linkage.h:7, > > > > > > from include/linux/kernel.h:8, > > > > > > from include/linux/list.h:9, > > > > > > from include/linux/module.h:12, > > > > > > from drivers/char/ipmi/ipmi_msghandler.c:17: > > > > > > drivers/char/ipmi/ipmi_msghandler.c: In function 'find_cmd_rcvr': > > > > > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > > ^ > > > > > > > > > > As mentioned above, RCU_LOCKDEP_WARN macro is called from > > > > > __list_check_rcu with 2 parameters > > > > > > > > > > 1. !cond && !rcu_read_lock_any_held() > > > > > 2. The message to display incase there is a lockdep warning. > > > > > > > > > > > > > > > However, if I pass the lockdep checking condition as: > > > > > > > > > > list_for_each_entry_rcu(ptr, list, head, lockdep_is_held(&some_lock) || rcu_read_lock_held()) > > > > > > > > Right, given the _rcu() suffix on the command, the rcu_read_lock_held() > > > > is implied. > > > > > > > > > this trickles down to __list_check_rcu and then finally to > > > > > RCU_LOCKDEP_WARN as (here cond is `lockdep_is_held(&some_lock) || rcu_read_lock_held()`): > > > > > > > > > > RCU_LOCKDEP_WARN(!lockdep_is_held(&some_lock) || rcu_read_lock_held() && !rcu_read_lock_any_held()) > > > > > > > > > > which according to operator precedence (I hopefully got them right) > > > > > would always evaluate to true if we are in an RCU read-side critical > > > > > section (without a lock), and hence, result in a false-positive lockdep > > > > > warning. > > > > > > > > It looks that way to me. But why not actually try it out? After all, > > > > only the running system knows for sure. And there might be some trick > > > > that we are both missing. > > > > > > > > > > I just tested this, here are the results: > > > > > > Case 1: Using`lockdep_is_held() || rcu_read_lock_held()` > > > > > > lock RCU RSCS Splat? Actual > > > Y Y N N > > > Y N N N > > > N Y Y N <= > > > N N Y Y > > > > > > Similar for > > > Case 2: Using `rcu_read_lock_held() || lockdep_is_held()` > > > > > > Case 3: Consider 2 locks (outside rcu_read_lock()) > > > `lockdep_is_held(lock1) || lockdep_is_held(lock2)` > > > > > > lock1 lock2 Splat? Actual > > > Y Y N* N > > > Y N N N > > > N Y Y N <= > > > N N Y Y > > > > > > This too proves the hypothesis (I'd like to call that). > > > > Very good! > > > > > *However, this shows an interesting result. When both lock1 and lock2 > > > are held, according to the hypothesis, a splat should've occured, since > > > the check condition (albeit faulty atm) would be: > > > > > > `!lockdep_is_held(lock1) || lockdep_is_held(lock2) && !rcu_read_lock_any_held()` > > > => `!T || T && !F` > > > => `F || T && T` > > > => `F || T` > > > => `T` > > > However, there was no splat. Which led me to investigate further and I > > > found out: > > > 1. `rcu_read_lock_any_held()` always returns 1 even if it is outside RCU > > > read-side CS. > > > 2. `rcu_read_lock_held()` seems OK, returns 1 when inside and 0 when > > > outside > > > > > > The kernel is compiled with > > > PROVE_RCU=y > > > PROVE_RCU_LIST=y > > > > Were you within a preempt-disable region, for example, was some other > > spinlock held? (A lockdep splat should give you a list of locks held.) > > Both of these act as generalized RCU read-side critical sections in > > recent kernels. > > I'm not sure if I was inside a preempt-disable region. I just created a > thread via kthread_run and ran rcu_read_lock_any_held() inside the > thread, I'll investigate further and get back to you. And no-other locks > were held. Here is the splat: > > [ 2724.551419] ============================= > [ 2724.551420] WARNING: suspicious RCU usage > [ 2724.551421] 5.5.0-rc5-next #15 Tainted: G OE > [ 2724.551422] ----------------------------- > [ 2724.551423] /home/amolg/git/lockdep_check/main.c:28 RCU-list > traversed in non-reader section!! > [ 2724.551424] > other info that might help us debug this: > > [ 2724.551425] > rcu_scheduler_active = 2, debug_locks = 1 > [ 2724.551426] no locks held by my_thread/4275. > [ 2724.551426] > stack backtrace: > [ 2724.551428] CPU: 2 PID: 4275 Comm: my_thread Tainted: G OE > 5.5.0-rc5-next #15 > [ 2724.551429] Hardware name: Gigabyte Technology Co., Ltd. > Z170-D3H/Z170-D3H-CF, BIOS F21 03/06/2017 > [ 2724.551430] Call Trace: > [ 2724.551437] dump_stack+0x8f/0xd0 > [ 2724.551440] threadfn+0x100/0x105 [main] > [ 2724.551443] ? __kthread_parkme+0x48/0x60 > [ 2724.551445] kthread+0xf9/0x130 > [ 2724.551447] ? 0xffffffffc00d7000 > [ 2724.551448] ? kthread_park+0x90/0x90 > [ 2724.551451] ret_from_fork+0x3a/0x50 Does your .config file by chance contain "CONFIG_PREEMPT=n"? In that case, pretty much any code not containing a call to schedule() or a return to userspace is an implicit RCU read-side critical section, which would mean that this is expected behavior. > > > Any thoughts on this? Is this intended? And should I send-in the patch > > > for the first problem? > > > > Separate patches for the initial problem and fixing the macro argument, > > please, if that is what you are asking. > > Sorry if I didn't make myself clear earlier. By `first problem` I was > referring to the macro argument problem itself. > > I think by `initial problem` you mean this patch (drivers/char/ipmi), > right? I'll make sure to send in a separate patch for the macro argument > problem. I did indeed mean that, thank you! Thanx, Paul > Thanks > Amol > > > Thanx, Paul > > > > > Thanks > > > Amol > > > > > > > > This could be easily solved by putting `cond` inside brackets as it is > > > > > correctly done in RCU_LOCKDEP_WARN macro but not in __list_check_rcu > > > > > macro. Is that so, or did I miss something? > > > > > > > > Again, that looks correct to me, but please check. > > > > > > > > > Secondly, since there is already a condition that checks for RCU > > > > > read-side critical section, the extra `rcu_read_lock_held()` we supply > > > > > is sort of redundant and can be skipped right? > > > > > > > > Yes, the general rule is that if the primitives ends with _rcu(), any > > > > lockdep condition will be in addition to rcu_read_lock_any_held(). > > > > So you should not need to pass RCU read-side lockdep expressions to > > > > primitives whose names end in _rcu.. > > > > > > > > Thanx, Paul > > > > > > > > > Thanks > > > > > Amol > > > > > > > > > > > include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var' > > > > > > #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > > > > > > ^ > > > > > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > > > > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > > > > ^ > > > > > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > > ^ > > > > > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > > > > > for (__list_check_rcu(dummy, ## cond, 0), \ > > > > > > ^ > > > > > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > > > > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > > > > > ^ > > > > > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > > ^ > > > > > > include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var' > > > > > > #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > > > > > > ^ > > > > > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > > > > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > > > > ^ > > > > > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > > ^ > > > > > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > > > > > for (__list_check_rcu(dummy, ## cond, 0), \ > > > > > > ^ > > > > > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > > > > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > > > > > ^ > > > > > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > > ^ > > > > > > include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value' > > > > > > (cond) ? \ > > > > > > ^ > > > > > > include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var' > > > > > > #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) > > > > > > ^ > > > > > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > > > > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > > > > ^ > > > > > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > > ^ > > > > > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > > > > > for (__list_check_rcu(dummy, ## cond, 0), \ > > > > > > ^ > > > > > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > > > > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > > > > > ^ > > > > > > > > > > > > vim +/if +263 include/linux/rcupdate.h > > > > > > > > > > > > 632ee200130899 Paul E. McKenney 2010-02-22 254 > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 255 /** > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 256 * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 257 * @c: condition to check > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 258 * @s: informative message > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 259 */ > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 260 #define RCU_LOCKDEP_WARN(c, s) \ > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 261 do { \ > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 262 static bool __section(.data.unlikely) __warned; \ > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 @263 if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 264 __warned = true; \ > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 265 lockdep_rcu_suspicious(__FILE__, __LINE__, s); \ > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 266 } \ > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 267 } while (0) > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 268 > > > > > > > > > > > > :::::: The code at line 263 was first introduced by commit > > > > > > :::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN() > > > > > > > > > > > > :::::: TO: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > > > > :::::: CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > > > > > > > > > > --- > > > > > > 0-DAY kernel test infrastructure Open Source Technology Center > > > > > > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation > > > > > > > > > > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2] drivers: char: ipmi: ipmi_msghandler: Pass lockdep expression to RCU lists 2020-01-16 3:21 ` Paul E. McKenney @ 2020-01-16 16:27 ` Amol Grover 0 siblings, 0 replies; 9+ messages in thread From: Amol Grover @ 2020-01-16 16:27 UTC (permalink / raw) To: Paul E. McKenney Cc: Corey Minyard, kbuild-all, kbuild test robot, Arnd Bergmann, linux-kernel, Joel Fernandes, openipmi-developer, linux-kernel-mentees On Wed, Jan 15, 2020 at 07:21:32PM -0800, Paul E. McKenney wrote: > On Thu, Jan 16, 2020 at 08:24:25AM +0530, Amol Grover wrote: > > On Wed, Jan 15, 2020 at 11:32:59AM -0800, Paul E. McKenney wrote: > > > On Wed, Jan 15, 2020 at 06:06:53PM +0530, Amol Grover wrote: > > > > On Tue, Jan 14, 2020 at 09:58:28AM -0800, Paul E. McKenney wrote: > > > > > On Tue, Jan 14, 2020 at 08:30:30AM +0530, Amol Grover wrote: > > > > > > On Sun, Jan 12, 2020 at 01:25:58PM +0800, kbuild test robot wrote: > > > > > > > Hi Amol, > > > > > > > > > > > > > > Thank you for the patch! Perhaps something to improve: > > > > > > > > > > > > > > [auto build test WARNING on char-misc/char-misc-testing] > > > > > > > [also build test WARNING on ipmi/for-next arm-soc/for-next v5.5-rc5] > > > > > > > [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/Amol-Grover/drivers-char-ipmi-ipmi_msghandler-Pass-lockdep-expression-to-RCU-lists/20200111-081002 > > > > > > > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 16bb7abc4a6b9defffa294e4dc28383e62a1dbcf > > > > > > > config: x86_64-randconfig-a003-20200109 (attached as .config) > > > > > > > compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010 > > > > > > > reproduce: > > > > > > > # save the attached .config to linux build tree > > > > > > > make ARCH=x86_64 > > > > > > > > > > > > > > If you fix the issue, kindly add following tag > > > > > > > Reported-by: kbuild test robot <lkp@intel.com> > > > > > > > > > > > > > > All warnings (new ones prefixed by >>): > > > > > > > > > > > > > > In file included from include/linux/export.h:43:0, > > > > > > > from include/linux/linkage.h:7, > > > > > > > from include/linux/kernel.h:8, > > > > > > > from include/linux/list.h:9, > > > > > > > from include/linux/module.h:12, > > > > > > > from drivers/char/ipmi/ipmi_msghandler.c:17: > > > > > > > drivers/char/ipmi/ipmi_msghandler.c: In function 'find_cmd_rcvr': > > > > > > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > > > ^ > > > > > > > > > > > > As mentioned above, RCU_LOCKDEP_WARN macro is called from > > > > > > __list_check_rcu with 2 parameters > > > > > > > > > > > > 1. !cond && !rcu_read_lock_any_held() > > > > > > 2. The message to display incase there is a lockdep warning. > > > > > > > > > > > > > > > > > > However, if I pass the lockdep checking condition as: > > > > > > > > > > > > list_for_each_entry_rcu(ptr, list, head, lockdep_is_held(&some_lock) || rcu_read_lock_held()) > > > > > > > > > > Right, given the _rcu() suffix on the command, the rcu_read_lock_held() > > > > > is implied. > > > > > > > > > > > this trickles down to __list_check_rcu and then finally to > > > > > > RCU_LOCKDEP_WARN as (here cond is `lockdep_is_held(&some_lock) || rcu_read_lock_held()`): > > > > > > > > > > > > RCU_LOCKDEP_WARN(!lockdep_is_held(&some_lock) || rcu_read_lock_held() && !rcu_read_lock_any_held()) > > > > > > > > > > > > which according to operator precedence (I hopefully got them right) > > > > > > would always evaluate to true if we are in an RCU read-side critical > > > > > > section (without a lock), and hence, result in a false-positive lockdep > > > > > > warning. > > > > > > > > > > It looks that way to me. But why not actually try it out? After all, > > > > > only the running system knows for sure. And there might be some trick > > > > > that we are both missing. > > > > > > > > > > > > > I just tested this, here are the results: > > > > > > > > Case 1: Using`lockdep_is_held() || rcu_read_lock_held()` > > > > > > > > lock RCU RSCS Splat? Actual > > > > Y Y N N > > > > Y N N N > > > > N Y Y N <= > > > > N N Y Y > > > > > > > > Similar for > > > > Case 2: Using `rcu_read_lock_held() || lockdep_is_held()` > > > > > > > > Case 3: Consider 2 locks (outside rcu_read_lock()) > > > > `lockdep_is_held(lock1) || lockdep_is_held(lock2)` > > > > > > > > lock1 lock2 Splat? Actual > > > > Y Y N* N > > > > Y N N N > > > > N Y Y N <= > > > > N N Y Y > > > > > > > > This too proves the hypothesis (I'd like to call that). > > > > > > Very good! > > > > > > > *However, this shows an interesting result. When both lock1 and lock2 > > > > are held, according to the hypothesis, a splat should've occured, since > > > > the check condition (albeit faulty atm) would be: > > > > > > > > `!lockdep_is_held(lock1) || lockdep_is_held(lock2) && !rcu_read_lock_any_held()` > > > > => `!T || T && !F` > > > > => `F || T && T` > > > > => `F || T` > > > > => `T` > > > > However, there was no splat. Which led me to investigate further and I > > > > found out: > > > > 1. `rcu_read_lock_any_held()` always returns 1 even if it is outside RCU > > > > read-side CS. > > > > 2. `rcu_read_lock_held()` seems OK, returns 1 when inside and 0 when > > > > outside > > > > > > > > The kernel is compiled with > > > > PROVE_RCU=y > > > > PROVE_RCU_LIST=y > > > > > > Were you within a preempt-disable region, for example, was some other > > > spinlock held? (A lockdep splat should give you a list of locks held.) > > > Both of these act as generalized RCU read-side critical sections in > > > recent kernels. > > > > I'm not sure if I was inside a preempt-disable region. I just created a > > thread via kthread_run and ran rcu_read_lock_any_held() inside the > > thread, I'll investigate further and get back to you. And no-other locks > > were held. Here is the splat: > > > > [ 2724.551419] ============================= > > [ 2724.551420] WARNING: suspicious RCU usage > > [ 2724.551421] 5.5.0-rc5-next #15 Tainted: G OE > > [ 2724.551422] ----------------------------- > > [ 2724.551423] /home/amolg/git/lockdep_check/main.c:28 RCU-list > > traversed in non-reader section!! > > [ 2724.551424] > > other info that might help us debug this: > > > > [ 2724.551425] > > rcu_scheduler_active = 2, debug_locks = 1 > > [ 2724.551426] no locks held by my_thread/4275. > > [ 2724.551426] > > stack backtrace: > > [ 2724.551428] CPU: 2 PID: 4275 Comm: my_thread Tainted: G OE > > 5.5.0-rc5-next #15 > > [ 2724.551429] Hardware name: Gigabyte Technology Co., Ltd. > > Z170-D3H/Z170-D3H-CF, BIOS F21 03/06/2017 > > [ 2724.551430] Call Trace: > > [ 2724.551437] dump_stack+0x8f/0xd0 > > [ 2724.551440] threadfn+0x100/0x105 [main] > > [ 2724.551443] ? __kthread_parkme+0x48/0x60 > > [ 2724.551445] kthread+0xf9/0x130 > > [ 2724.551447] ? 0xffffffffc00d7000 > > [ 2724.551448] ? kthread_park+0x90/0x90 > > [ 2724.551451] ret_from_fork+0x3a/0x50 > > Does your .config file by chance contain "CONFIG_PREEMPT=n"? > Indeed, CONFIG_PREEMPT was not set in my config, hence, the different results. > In that case, pretty much any code not containing a call to schedule() or > a return to userspace is an implicit RCU read-side critical section, which > would mean that this is expected behavior. > Got it, thank you! Thanks Amol > > > > Any thoughts on this? Is this intended? And should I send-in the patch > > > > for the first problem? > > > > > > Separate patches for the initial problem and fixing the macro argument, > > > please, if that is what you are asking. > > > > Sorry if I didn't make myself clear earlier. By `first problem` I was > > referring to the macro argument problem itself. > > > > I think by `initial problem` you mean this patch (drivers/char/ipmi), > > right? I'll make sure to send in a separate patch for the macro argument > > problem. > > I did indeed mean that, thank you! > > Thanx, Paul > > > Thanks > > Amol > > > > > Thanx, Paul > > > > > > > Thanks > > > > Amol > > > > > > > > > > This could be easily solved by putting `cond` inside brackets as it is > > > > > > correctly done in RCU_LOCKDEP_WARN macro but not in __list_check_rcu > > > > > > macro. Is that so, or did I miss something? > > > > > > > > > > Again, that looks correct to me, but please check. > > > > > > > > > > > Secondly, since there is already a condition that checks for RCU > > > > > > read-side critical section, the extra `rcu_read_lock_held()` we supply > > > > > > is sort of redundant and can be skipped right? > > > > > > > > > > Yes, the general rule is that if the primitives ends with _rcu(), any > > > > > lockdep condition will be in addition to rcu_read_lock_any_held(). > > > > > So you should not need to pass RCU read-side lockdep expressions to > > > > > primitives whose names end in _rcu.. > > > > > > > > > > Thanx, Paul > > > > > > > > > > > Thanks > > > > > > Amol > > > > > > > > > > > > > include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var' > > > > > > > #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > > > > > > > ^ > > > > > > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > > > > > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > > > > > ^ > > > > > > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > > > ^ > > > > > > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > > > > > > for (__list_check_rcu(dummy, ## cond, 0), \ > > > > > > > ^ > > > > > > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > > > > > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > > > > > > ^ > > > > > > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > > > ^ > > > > > > > include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var' > > > > > > > #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > > > > > > > ^ > > > > > > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > > > > > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > > > > > ^ > > > > > > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > > > ^ > > > > > > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > > > > > > for (__list_check_rcu(dummy, ## cond, 0), \ > > > > > > > ^ > > > > > > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > > > > > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > > > > > > ^ > > > > > > > include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses] > > > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > > > ^ > > > > > > > include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value' > > > > > > > (cond) ? \ > > > > > > > ^ > > > > > > > include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var' > > > > > > > #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) > > > > > > > ^ > > > > > > > >> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if' > > > > > > > if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > > > > > ^ > > > > > > > >> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN' > > > > > > > RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ > > > > > > > ^ > > > > > > > include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu' > > > > > > > for (__list_check_rcu(dummy, ## cond, 0), \ > > > > > > > ^ > > > > > > > drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu' > > > > > > > list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link, > > > > > > > ^ > > > > > > > > > > > > > > vim +/if +263 include/linux/rcupdate.h > > > > > > > > > > > > > > 632ee200130899 Paul E. McKenney 2010-02-22 254 > > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 255 /** > > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 256 * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met > > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 257 * @c: condition to check > > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 258 * @s: informative message > > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 259 */ > > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 260 #define RCU_LOCKDEP_WARN(c, s) \ > > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 261 do { \ > > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 262 static bool __section(.data.unlikely) __warned; \ > > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 @263 if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \ > > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 264 __warned = true; \ > > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 265 lockdep_rcu_suspicious(__FILE__, __LINE__, s); \ > > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 266 } \ > > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 267 } while (0) > > > > > > > f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 268 > > > > > > > > > > > > > > :::::: The code at line 263 was first introduced by commit > > > > > > > :::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN() > > > > > > > > > > > > > > :::::: TO: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > > > > > :::::: CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > > > > > > > > > > > > --- > > > > > > > 0-DAY kernel test infrastructure Open Source Technology Center > > > > > > > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation > > > > > > > > > > > > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-01-16 16:27 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-10 16:47 [Linux-kernel-mentees] [PATCH v2] drivers: char: ipmi: ipmi_msghandler: Pass lockdep expression to RCU lists Amol Grover 2020-01-12 5:25 ` kbuild test robot 2020-01-14 3:00 ` Amol Grover 2020-01-14 17:58 ` Paul E. McKenney 2020-01-15 12:36 ` Amol Grover 2020-01-15 19:32 ` Paul E. McKenney 2020-01-16 2:54 ` Amol Grover 2020-01-16 3:21 ` Paul E. McKenney 2020-01-16 16:27 ` Amol Grover
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).