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

[Davidlohr Bueso <dave@stgolabs.net>]

On Thu, 25 Jul 2024, Michal Hocko wrote:\n
>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.

Why is it so weird? Perhaps I'm missing your point, but the fallback
introduced in that commit attempts to avoid abusing nofail semantics
and only ask with a smaller size.

In any case, would the following be better (and also silences smatch)?
Disregarding the initial nofail request, rhashtable allocations are
always either regular GFP_KERNEL or GFP_ATOMIC (for the nested and
some insertion cases).

-----8<-----
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index dbbed19f8fff..c9f9cce4a3c1 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -184,12 +184,12 @@ static struct bucket_table *bucket_table_alloc(struct rhashtable *ht,
  	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));
+			kvmalloc_noprof(struct_size(tbl, buckets, nbuckets),
+					gfp|__GFP_ZERO));
  
  	size = nbuckets;
  
-	if (tbl == NULL && (gfp & ~__GFP_NOFAIL) != GFP_KERNEL) {
+	if (tbl == NULL && (gfp & GFP_ATOMIC)) {
  		tbl = nested_bucket_table_alloc(ht, nbuckets, gfp);
  		nbuckets = 0;
  	}