Linux Kernel Mentees Archive on lore.kernel.org
 help / color / Atom feed
From: "Paul E. McKenney" <paulmck@kernel.org>
To: Amol Grover <frextrite@gmail.com>
Cc: Corey Minyard <minyard@acm.org>,
	kbuild-all@lists.01.org, kbuild test robot <lkp@intel.com>,
	Arnd Bergmann <arnd@arndb.de>,
	linux-kernel@vger.kernel.org,
	Joel Fernandes <joel@joelfernandes.org>,
	openipmi-developer@lists.sourceforge.net,
	linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [Linux-kernel-mentees] [PATCH v2] drivers: char: ipmi: ipmi_msghandler: Pass lockdep expression to RCU lists
Date: Tue, 14 Jan 2020 09:58:28 -0800
Message-ID: <20200114175828.GR2935@paulmck-ThinkPad-P72> (raw)
In-Reply-To: <20200114030030.GB2559@workstation-portable>

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

  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-10 16:47 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 [this message]
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

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200114175828.GR2935@paulmck-ThinkPad-P72 \
    --to=paulmck@kernel.org \
    --cc=arnd@arndb.de \
    --cc=frextrite@gmail.com \
    --cc=joel@joelfernandes.org \
    --cc=kbuild-all@lists.01.org \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lkp@intel.com \
    --cc=minyard@acm.org \
    --cc=openipmi-developer@lists.sourceforge.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux Kernel Mentees Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kernel-mentees/0 linux-kernel-mentees/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kernel-mentees linux-kernel-mentees/ https://lore.kernel.org/linux-kernel-mentees \
		linux-kernel-mentees@lists.linuxfoundation.org
	public-inbox-index linux-kernel-mentees

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxfoundation.lists.linux-kernel-mentees


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git