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

[Michal Hocko <mhocko@suse.com>]

On Thu 25-07-24 13:38:50, Barry Song wrote:
> On Thu, Jul 25, 2024 at 12:17 AM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Wed 24-07-24 20:55:44, 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.
> >
> > Ohh, so you have done the migration. Please squash those two patches.
> > Also if we want to preserve clean __GFP_NOFAIL for internal MM use then it
> > should be moved away from include/linux/gfp_types.h. But is there any
> > real use for that?
> 
> yes. currently i got two,
> 
> lib/rhashtable.c
> 
> static struct bucket_table *bucket_table_alloc(struct rhashtable *ht,
>                                                size_t nbuckets,
>                                                gfp_t gfp)
> {
>         struct bucket_table *tbl = NULL;
>         size_t size;
>         int i;
>         static struct lock_class_key __key;
> 
>         tbl = alloc_hooks_tag(ht->alloc_tag,
>                         kvmalloc_node_noprof(struct_size(tbl, buckets,
> nbuckets),
>                                              gfp|__GFP_ZERO, NUMA_NO_NODE));
> 
>         size = nbuckets;
> 
>         if (tbl == NULL && (gfp & ~__GFP_NOFAIL) != GFP_KERNEL) {
>                 tbl = nested_bucket_table_alloc(ht, nbuckets, gfp);
>                 nbuckets = 0;
>         }
> 
>         ...
> 
>         return tbl;
> }

Ugh. OK this is a weird allocation fallback strategy 2d22ecf6db1c
("lib/rhashtable: guarantee initial hashtable allocation"). Maybe the
code should be just simplified and GFP_NOFAIL used from the begining?
Davidlohr WDYT? For your context Barry tries to drop all the
__GFP_NOFAIL use and replace it by GFP_NOFAIL which enforces
__GFP_DIRECT_RECLAIM so that people cannot request atomic NOFAIL.

> and tools/perf/builtin-kmem.c:
> 
> static const struct {
>         const char *original;
>         const char *compact;
> } gfp_compact_table[] = {
>         { "GFP_TRANSHUGE",              "THP" },
>         { "GFP_TRANSHUGE_LIGHT",        "THL" },
>         { "GFP_HIGHUSER_MOVABLE",       "HUM" },
>         { "GFP_HIGHUSER",               "HU" },
>         ...
>         { "__GFP_NOFAIL",               "NF" },
>        ...
> };

This is a prrintk formatting stuff. This counts as low level
functionality.

-- 
Michal Hocko
SUSE Labs