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:   Wed, 26 Jul 2023 14:07:47 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     "Joel Fernandes (Google)" <joel@joelfernandes.org>
Cc:     linux-kernel@vger.kernel.org,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Josh Triplett <josh@joshtriplett.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        "open list:SLEEPABLE READ-COPY UPDATE (SRCU)" <rcu@vger.kernel.org>
Subject: Re: [PATCH 2/5] srcu: Fix error handling in init_srcu_struct_fields()
Message-ID: <1943e34c-3c8e-411c-a677-33f8c5b4b0e5@paulmck-laptop>
Reply-To: paulmck@kernel.org
References: <20230725232913.2981357-1-joel@joelfernandes.org>
 <20230725232913.2981357-3-joel@joelfernandes.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230725232913.2981357-3-joel@joelfernandes.org>
Precedence: bulk

On Tue, Jul 25, 2023 at 11:29:07PM +0000, Joel Fernandes (Google) wrote:
> The current error handling in init_srcu_struct_fields() is a bit
> inconsistent.  If init_srcu_struct_nodes() fails, the function either
> returns -ENOMEM or 0 depending on whether ssp->sda_is_static is true or
> false. This can make init_srcu_struct_fields() return 0 even if memory
> allocation failed!
> 
> Simplify the error handling by always returning -ENOMEM if either
> init_srcu_struct_nodes() or the per-CPU allocation fails. This makes the
> control flow easier to follow and avoids the inconsistent return values.
> 
> Add goto labels to avoid duplicating the error cleanup code.
> 
> Link: https://lore.kernel.org/r/20230404003508.GA254019@google.com
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Looks good, nice simplification!  One nit below.

							Thanx, Paul

> ---
>  kernel/rcu/srcutree.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 20d7a238d675..cbc37cbc1805 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -255,29 +255,32 @@ static int init_srcu_struct_fields(struct srcu_struct *ssp, bool is_static)
>  	ssp->srcu_sup->sda_is_static = is_static;
>  	if (!is_static)
>  		ssp->sda = alloc_percpu(struct srcu_data);
> -	if (!ssp->sda) {
> -		if (!is_static)
> -			kfree(ssp->srcu_sup);
> -		return -ENOMEM;
> -	}
> +	if (!ssp->sda)
> +		goto err_free_sup;
>  	init_srcu_struct_data(ssp);
>  	ssp->srcu_sup->srcu_gp_seq_needed_exp = 0;
>  	ssp->srcu_sup->srcu_last_gp_end = ktime_get_mono_fast_ns();
>  	if (READ_ONCE(ssp->srcu_sup->srcu_size_state) == SRCU_SIZE_SMALL && SRCU_SIZING_IS_INIT()) {
> -		if (!init_srcu_struct_nodes(ssp, GFP_ATOMIC)) {
> -			if (!ssp->srcu_sup->sda_is_static) {

I was going to complain about this ssp->srcu_sup->sda_is_static becoming
just is_static, but now I cannot see why I didn't just use is_static in
the first place.  ;-)

> -				free_percpu(ssp->sda);
> -				ssp->sda = NULL;
> -				kfree(ssp->srcu_sup);
> -				return -ENOMEM;
> -			}
> -		} else {
> +		if (!init_srcu_struct_nodes(ssp, GFP_ATOMIC))
> +			goto err_free_sda;
> +		else
>  			WRITE_ONCE(ssp->srcu_sup->srcu_size_state, SRCU_SIZE_BIG);

Given that the "then" clause is a goto, what is the "else" clause doing
for us?

> -		}
>  	}
>  	ssp->srcu_sup->srcu_ssp = ssp;
>  	smp_store_release(&ssp->srcu_sup->srcu_gp_seq_needed, 0); /* Init done. */
>  	return 0;
> +
> +err_free_sda:
> +	if (!is_static) {
> +		free_percpu(ssp->sda);
> +		ssp->sda = NULL;
> +	}
> +err_free_sup:
> +	if (!is_static) {
> +		kfree(ssp->srcu_sup);
> +		ssp->srcu_sup = NULL;
> +	}
> +	return -ENOMEM;
>  }
>  
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> -- 
> 2.41.0.487.g6d72f3e995-goog
>