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/[email protected]
Signed-off-by: Joel Fernandes (Google) <[email protected]>
---
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) {
- 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
--
2.41.0.487.g6d72f3e995-goog
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/[email protected]
> Signed-off-by: Joel Fernandes (Google) <[email protected]>
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
>
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
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
> On Jul 27, 2023, at 12:02 AM, Paul E. McKenney <[email protected]> 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