On Mon, Apr 3, 2023 at 9:06 PM Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
[..]
> > In other words, if init_srcu_struct_nodes() returns false, then passing along
> > the return value of init_srcu_struct_nodes() back to the caller of
> > init_srcu_struct_fields() depends on whether is_static = true or false. That
> > seems a bit wrong to me, init_srcu_struct_fields() should always return
> > -ENOMEM when init_srcu_struct_nodes() fails to allocate memory IMHO, whether
> > is_static is true or not.
> >
> > Sorry if I missed something subtle, and if the code is correct to begin with.
> > Also I feel the return paths could be made better to also fix the above issue
> > I mentioned. How about the following diff on top of the series, would it
> > work?
>
> Your restructuring looks like an excellent step forward, but given the late
> date, it might be best to avoid being in a rush.
>
> I -could- make the following small patch:
>
> 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 {
> WRITE_ONCE(ssp->srcu_sup->srcu_size_state, SRCU_SIZE_BIG);
> }
> }
>
> Except that this is a pre-existing bug that as far as we know no one
> is hitting, so the risk doesn't seem to stack up. After all, if you
> are hitting memory exhaustion at boot or on module load, this bug is
> probably the least of your problems. Even this fix looks to be v6.5
> material to me.
>
> So would you be willing to send a patch like the above that fixes the
> bug, and another like you have below to get a better error-path
> structure? No hurry, the end of this month is perfectly fine.
Yes, I am happy to send patches along these lines by the end of the
month. I will make a note to do so.
> And again, good catch!
Thanks!
- Joel
