Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 16 May 2023 05:17:57 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     Dan Carpenter <dan.carpenter@linaro.org>
Cc:     oe-kbuild@lists.linux.dev, lkp@intel.com,
        oe-kbuild-all@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: kernel/rcu/srcutree.c:1644 srcu_advance_state() warn:
 inconsistent returns '&ssp->srcu_sup->srcu_gp_mutex'.
Message-ID: <2ef061eb-e82d-4c16-a333-e499095a7162@paulmck-laptop>
Reply-To: paulmck@kernel.org
References: <c0937326-1cf1-429a-9883-30d9d79b01d0@kili.mountain>
 <c33b0348-7f86-47ce-913c-f1ebc6431f93@paulmck-laptop>
 <30f2e760-e2f3-4941-be9b-b9c5624fd861@kili.mountain>
 <4bdbbcd3-6620-4320-ada5-02b71a54106c@paulmck-laptop>
 <782b3fa6-576d-4c26-888e-5dc151feaaa8@kili.mountain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <782b3fa6-576d-4c26-888e-5dc151feaaa8@kili.mountain>
Precedence: bulk

On Tue, May 09, 2023 at 06:13:02PM +0300, Dan Carpenter wrote:
> On Tue, May 09, 2023 at 07:08:05AM -0700, Paul E. McKenney wrote:
> > On Tue, May 09, 2023 at 08:40:33AM +0300, Dan Carpenter wrote:
> > > On Sat, May 06, 2023 at 11:45:35AM -0700, Paul E. McKenney wrote:
> > > > On Sat, May 06, 2023 at 10:22:04AM +0300, Dan Carpenter wrote:
> > > > > aacb5d91ab1bfb Paul E. McKenney 2018-10-28  1632  	if (rcu_seq_state(READ_ONCE(ssp->srcu_gp_seq)) == SRCU_STATE_SCAN2) {
> > > > > 
> > > > > We don't mutex_unlock(&ssp->srcu_sup->srcu_gp_mutex) if this if
> > > > > statement is false.
> > > > 
> > > > Hmmm...
> > > > 
> > > > I could make the above line read something like the following:
> > > > 
> > > > 	if (!WARN_ON_ONCE(rcu_seq_state(READ_ONCE(ssp->srcu_sup->srcu_gp_seq)) != SRCU_STATE_SCAN2)) {
> > > 
> > > Smatch ignores WARN_ON().  WARNings are triggered all the time, so it's
> > > not like a BUG() which stops the code flow.
> > > 
> > > > 
> > > > The theory is that there are only three legal values for ->srcu_gp_seq.
> > > > Because we hold ->srcu_gp_mutex, no one else can change it.   The first
> > > > "if" statement either returns or sets that state to SRCU_STATE_SCAN1.
> > > > The second "if" statement also either returns or sets that state to
> > > > SRCU_STATE_SCAN2.  So that statement should not be false.
> > > 
> > > Smatch can't figure out that the statement is true.  The issue there is
> > > that ssp->srcu_sup->srcu_gp_seq stores a value in the low bits and a
> > > different value in the high bits.  This seems like something that might
> > > be worth handling correctly at some point, but that point is in the
> > > distant future...
> > > 
> > > Just ignore this one.
> > 
> > Fair enough!  Yeah, I could imagine that this would be non-trivial.
> > 
> > Is there a not-reached annotation that Smatch pays attention to?
> 
> Hm...  Yeah.  If you wanted you could do this.  I'm not sure it improves
> the readability.  Also for some reason my private Smatch build doesn't
> print a warning...  I need to investigate why that is...

There does seem to be a fair number of instances of unreachable() in
the kernel, so why not?

May I add your Signed-off-by?

							Thanx, Paul

> regards,
> dan carpenter
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 20d7a238d675..58e13d3c5a6a 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -1669,6 +1669,8 @@ static void srcu_advance_state(struct srcu_struct *ssp)
>  		}
>  		ssp->srcu_sup->srcu_n_exp_nodelay = 0;
>  		srcu_gp_end(ssp);  /* Releases ->srcu_gp_mutex. */
> +	} else {
> +		unreachable();
>  	}
>  }
>