Re: [PATCH RFC 5/5] non-mm: discourage the usage of __GFP_NOFAIL and encourage GFP_NOFAIL

Barry Song
2024-07-24T09:58:51Z

On Wed, Jul 24, 2024 at 9:53 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 7/24/24 10:55 AM, Barry Song wrote:
> > From: Barry Song <v-songbaohua@oppo.com>
> >
> > GFP_NOFAIL includes the meaning of block and direct reclamation, which
> > is essential for a true no-fail allocation. We are gradually starting
> > to enforce this block semantics to prevent the potential misuse of
> > __GFP_NOFAIL in atomic contexts in the future.
> >
> > A typical example of incorrect usage is in VDPA, where GFP_ATOMIC
> > and __GFP_NOFAIL are used together.
> >
> > [RFC]: This patch seems quite large; I don't mind splitting it into
> > multiple patches for different subsystems after patches 1 ~ 4 have
> > been applied.
> >
> > Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> >
> > diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
> > index fa01818c1972..29eaf8b84b52 100644
> > --- a/arch/powerpc/sysdev/xive/common.c
> > +++ b/arch/powerpc/sysdev/xive/common.c
> > @@ -1146,7 +1146,7 @@ static int __init xive_init_ipis(void)
> > if (!ipi_domain)
> > goto out_free_fwnode;
> >
> > - xive_ipis = kcalloc(nr_node_ids, sizeof(*xive_ipis), GFP_KERNEL | __GFP_NOFAIL);
> > + xive_ipis = kcalloc(nr_node_ids, sizeof(*xive_ipis), GFP_KERNEL | GFP_NOFAIL);
>
> This (and others) doesn't look great. Normally there's just one GFP_MAIN
> that combines several commonly used together flags internally, with possibly
> some | __GFP_EXTRA addition for less common modifications. Now you're
> combining two GFP_MAIN's and that's just confusing.

This is true, but I assume this won't incur overhead at runtime since the
compiler resolves GFP_KERNEL | GFP_NOFAIL at compile-time.
Only readers might find some bits are duplicated OR twice?

>
> So if we want to go this way, you'd need e.g.
>
> GFP_KERNEL_NOFAIL which is GFP_KERNEL | __GFP_NOFAIL

I actually considered this, but it doesn't always work because we have
many cases:

variable |= __GFP_NOFAIL.

>
> And probably also GFP_NOFS_NOFAIL and GFP_NOIO_NOFAIL (sigh).
>
> > if (!xive_ipis)
> > goto out_free_domain;
>