Re: [PATCH slab v5 2/6] mm: Allow GFP_ACCOUNT to be used in alloc_pages_nolock().

[Shakeel Butt <shakeel.butt@linux.dev>]

On Fri, Sep 12, 2025 at 10:34:15AM -0700, Alexei Starovoitov wrote:
> On Fri, Sep 12, 2025 at 10:15 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Fri, Sep 12, 2025 at 10:11:26AM -0700, Shakeel Butt wrote:
> > > On Mon, Sep 08, 2025 at 06:00:03PM -0700, Alexei Starovoitov wrote:
> > > [...]
> > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > > index d1d037f97c5f..30ccff0283fd 100644
> > > > --- a/mm/page_alloc.c
> > > > +++ b/mm/page_alloc.c
> > > > @@ -7480,6 +7480,7 @@ static bool __free_unaccepted(struct page *page)
> > > >
> > > >  /**
> > > >   * alloc_pages_nolock - opportunistic reentrant allocation from any context
> > > > + * @gfp_flags: GFP flags. Only __GFP_ACCOUNT allowed.
> > >
> > > If only __GFP_ACCOUNT is allowed then why not use a 'bool account' in the
> > > parameter and add __GFP_ACCOUNT if account is true?
> >
> > It's clearer in the callers to call alloc_pages_nolock(__GFP_ACCOUNT)
> > than it is to call alloc_pages_nolock(true).
> >
> > I can immediately tell what the first one does.  I have no idea what
> > the polarity of 'true' might be (does it mean accounted or unaccounted?)
> > Is it rlated to accounting, GFP_COMP, highmem, whether it's OK to access
> > atomic reserves ... or literally anything else that you might want to
> > select when allocating memory.
> >
> > This use of unadorned booleans is an antipattern.  Nobody should be
> > advocating for such things.
> 
> +1.
> We strongly discourage bool in arguments in any function.
> It makes callsites unreadable.
> 
> We learned it the hard way though :(
> Some of the verifier code became a mess like:
>         err = check_load_mem(env, insn, true, false, false, "atomic_load");
> 
> it's on our todo to clean this up.

Sounds good.