diff mbox series

[2/5] srcu: Fix error handling in init_srcu_struct_fields()

Message ID 20230725232913.2981357-3-joel@joelfernandes.org (mailing list archive)
State Superseded
Headers show
Series misc RCU fixes and cleanups | expand

Commit Message

Joel Fernandes July 25, 2023, 11:29 p.m. UTC 
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>
---
 kernel/rcu/srcutree.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

Comments

Paul E. McKenney July 26, 2023, 9:07 p.m. UTC | #1 
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
>
Joel Fernandes July 27, 2023, 3:04 a.m. UTC | #2 
On 7/26/23 17:07, Paul E. McKenney wrote:
>> -				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?
> 

Not much. Agreed we can get rid of "else" and reduce indent of the
WRITE_ONCE that follows.

Would you mind making this fixup in the patch for your apply, or would
you like me to refresh the patch? Let me know either way, thank you!

 - Joel
Paul E. McKenney July 27, 2023, 4:02 a.m. UTC | #3 
On Wed, Jul 26, 2023 at 11:04:04PM -0400, Joel Fernandes wrote:
> 
> 
> On 7/26/23 17:07, Paul E. McKenney wrote:
> >> -				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?
> > 
> 
> Not much. Agreed we can get rid of "else" and reduce indent of the
> WRITE_ONCE that follows.
> 
> Would you mind making this fixup in the patch for your apply, or would
> you like me to refresh the patch? Let me know either way, thank you!

Please include it with your next series, which has at least one other
change anyway.  ;-)

							Thanx, Paul
Joel Fernandes July 27, 2023, 4:24 a.m. UTC | #4 
> On Jul 27, 2023, at 12:02 AM, Paul E. McKenney <paulmck@kernel.org> wrote:
> 
> On Wed, Jul 26, 2023 at 11:04:04PM -0400, Joel Fernandes wrote:
>> 
>> 
>> On 7/26/23 17:07, Paul E. McKenney wrote:
>>>> -                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?
>>> 
>> 
>> Not much. Agreed we can get rid of "else" and reduce indent of the
>> WRITE_ONCE that follows.
>> 
>> Would you mind making this fixup in the patch for your apply, or would
>> you like me to refresh the patch? Let me know either way, thank you!
> 
> Please include it with your next series, which has at least one other
> change anyway.  ;-)

My pleasure ;-).

- Joel


> 
>                            Thanx, Paul
diff mbox series

Patch

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) {
-				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);
-		}
 	}
 	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