Re: [Lsf-pc] [LSF/MM/BPF TOPIC] Reclamation interactions with RCU

Dan Carpenter
2024-03-22T06:13:36Z

On Fri, Mar 22, 2024 at 12:47:42PM +1100, NeilBrown wrote:
> On Thu, 21 Mar 2024, Dan Carpenter wrote:
> > On Mon, Mar 04, 2024 at 09:45:48AM +1100, NeilBrown wrote:
> > > I have in mind a more explicit statement of how much waiting is
> > > acceptable.
> > >
> > > GFP_NOFAIL - wait indefinitely
> >
> > Why not call it GFP_SMALL? It wouldn't fail. The size would have to be
> > less than some limit. If the size was too large, that would trigger a
> > WARN_ON_ONCE().
>
> I would be happy with GFP_SMALL. It would never return NULL but might
> block indefinitely. It would (as you say) WARN (maybe ONCE) if the size
> was considered "COSTLY" and would possibly BUG if the size exceeded
> KMALLOC_MAX_SIZE.

I'd like to keep GFP_SMALL much smaller than KMALLOC_MAX_SIZE. IIf
you're allocating larger than that, you'd still be able to GFP_NOFAIL.
I looked quickly an I think over 60% of allocations are just sizeof(*p)
and probably 90% are under 4k.

>
> >
> > I obviously understand that this duplicates the information in the size
> > parameter but the point is that GFP_SMALL allocations have been
> > reviewed, updated, and don't have error handling code.
>
> We are on the same page here.
>
> >
> > We'd keep GFP_KERNEL which would keep the existing behavior. (Which is
> > that it can sleep and it can fail). I think that maps to GFP_RETRY but
> > GFP_RETRY is an uglier name.
>
> Can it fail though? I know it is allowed to, but does it happen?
>

In some sense, I don't really care about this, I just want the rules
clear from a static analysis perspective. Btw, you're discussing making
the too small to fail rule official but other times we have discussed
getting rid of it all together. So I think maybe it's better to keep
the rules strict but allow the actual implentation to change later.

The GFP_SMALL stuff is nice for static analysis because it would warn
about anything larger than whatever the small limit is. So that means I
have fewer allocations to review for integer overflow bugs.

Btw, Jiri Pirko, was proposing a macro which would automatically
allocate the 60+% of allocations which are sizeof(*p).
https://lore.kernel.org/all/20240315132249.2515468-1-jiri@resnulli.us/
I had offered an alternative macro but the idea is the same:

#define __ALLOC(p) p __free(kfree) = kzalloc(sizeof(*p), GFP_KERNEL)
struct ice_aqc_get_phy_caps_data *__ALLOC(pcaps);

Combining no fail allocations with automatic cleanup changes the way you
write code.

And then on the success patch you have the no_free_ptr() which I haven't
used but I think will be so useful for static analysis. I'm so excited
about this.

regards,
dan carpenter