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@xxxxxxxxxx > Signed-off-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx> 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 >
