On Sun, Aug 18, 2024 at 2:45 PM Barry Song [off-list ref] wrote:

On Sun, Aug 18, 2024 at 6:27 PM Barry Song [off-list ref] wrote:

quoted

On Sun, Aug 18, 2024 at 5:51 PM Yafang Shao [off-list ref] wrote:

quoted

On Sun, Aug 18, 2024 at 11:48 AM Barry Song [off-list ref] wrote:

quoted

On Sun, Aug 18, 2024 at 2:55 PM Yafang Shao [off-list ref] wrote:

quoted

On Sat, Aug 17, 2024 at 2:25 PM Barry Song [off-list ref] wrote:

quoted

From: Barry Song <redacted>

When users allocate memory with the __GFP_NOFAIL flag, they might
incorrectly use it alongside GFP_ATOMIC, GFP_NOWAIT, etc.  This kind of
non-blockable __GFP_NOFAIL is not supported and is pointless.  If we
attempt and still fail to allocate memory for these users, we have two
choices:

    1. We could busy-loop and hope that some other direct reclamation or
    kswapd rescues the current process. However, this is unreliable
    and could ultimately lead to hard or soft lockups,

That can occur even if we set both __GFP_NOFAIL and
__GFP_DIRECT_RECLAIM, right? So, I don't believe the issue is related
to setting __GFP_DIRECT_RECLAIM; rather, it stems from the flawed
design of __GFP_NOFAIL itself.

the point of GFP_NOFAIL is that it won't fail and its user won't check
the return value. without direct_reclamation, it is sometimes impossible.
but with direct reclamation, users constantly wait and finally they can

So, what exactly is the difference between 'constantly waiting' and
'busy looping'? Could you please clarify? Also, why can't we
'constantly wait' when __GFP_DIRECT_RECLAIM is not set?

I list two options in changelog
1: busy loop 2. bug_on. I am actually fine with either one. either one is
better than the existing code. but returning null in the current code
is definitely wrong.

1 somehow has the attempt to make __GFP_NOFAIL without direct_reclamation
legal. so it is a bit suspicious going in the wrong direction.

busy-loop is that you are not reclaiming memory you are not sleeping.
cpu is constantly working and busy, so it might result in a lockup, either
soft lockup or hard lockup.

Thanks for the clarification.
That can be avoided by a simple cond_resched() if the hard lockup or
softlockup is the main issue ;)

quoted

with direct_reclamation, wait is the case you can sleep. it is not holding
cpu, not a busy loop. in rare case, users might end in endless wait,
but it matches the doc of __GFP_NOFAIL, never return till memory
is gotten (the current code is implemented in this way unless users
incorrectly combine __GFP_NOFAIL with aotmic/nowait etc.)

and the essential difference between "w/ and w/o direct_reclaim": with
direct reclaim, the user is actively reclaiming memory to rescue itself
by all kinds of possible ways(compact, oom, reclamation), while without
direct reclamation, it can do nothing and just loop (busy-loop).

It can wake up kswapd, which can then reclaim memory. If kswapd can't
keep up, the system is likely under heavy memory pressure. In such a
case, it makes little difference whether __GFP_DIRECT_RECLAIM is set
or not. For reference, see the old issue:
https://lore.kernel.org/lkml/d9802b6a-949b-b327-c4a6-3dbca485ec20@gmx.com/ (local).

I believe the core issue persists, and the design of __GFP_NOFAIL
exacerbates it.

By the way, I believe we could trigger an asynchronous OOM kill in the
case without direct reclaim to avoid busy looping.

quoted

note, long-term we won't expose __GFP_NOFAIL any more. we
will only expose GFP_NOFAIL which enforces Blockable.  I am
quite busy on other issues, so this won't happen in a short time.

quoted

quoted

get memory. if you read the doc of __GFP_NOFAIL you will find it.
it is absolutely clearly documented.

quoted

quoted

which might not
    be well supported by some architectures.

    2. We could use BUG_ON to trigger a reliable system crash, avoiding
    exposing NULL dereference.

Neither option is ideal, but both are improvements over the existing code.
This patch selects the second option because, with the introduction of
scoped API and GFP_NOFAIL—capable of enforcing direct reclamation for
nofail users(which is in my plan), non-blockable nofail allocations will
no longer be possible.

Signed-off-by: Barry Song <redacted>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Uladzislau Rezki (Sony) <urezki@gmail.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Lorenzo Stoakes <redacted>
Cc: Christoph Lameter <redacted>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <redacted>
Cc: Vlastimil Babka <redacted>
Cc: Roman Gushchin <roman.gushchin@linux.dev>
Cc: Hyeonggon Yoo <redacted>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Kees Cook <kees@kernel.org>
Cc: "Eugenio Pérez" <eperezma@redhat.com>
Cc: Hailong.Liu <redacted>
Cc: Jason Wang <redacted>
Cc: Maxime Coquelin <redacted>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 mm/page_alloc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d2c37f8f8d09..fb5850ecd3ae 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c

@@ -4399,11 +4399,11 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
         */
        if (gfp_mask & __GFP_NOFAIL) {
                /*
-                * All existing users of the __GFP_NOFAIL are blockable, so warn
-                * of any new users that actually require GFP_NOWAIT
+                * All existing users of the __GFP_NOFAIL are blockable
+                * otherwise we introduce a busy loop with inside the page
+                * allocator from non-sleepable contexts
                 */
-               if (WARN_ON_ONCE_GFP(!can_direct_reclaim, gfp_mask))
-                       goto fail;
+               BUG_ON(!can_direct_reclaim);

I'm not in favor of using BUG_ON() here, as many call sites already
handle the return value of __GFP_NOFAIL.

it is not correct to handle the return value of __GFP_NOFAIL.
if you check the ret, don't use __GFP_NOFAIL.

If so, you have many code changes to make in the linux kernel ;)

Please list those code using __GFP_NOFAIL and check the result
might fail, we should get them fixed. This is insane. NOFAIL means
no fail.

You can find some instances with grep commands, but there's no
reliable way to capture them all with a single command. Here are a few
examples:

                // drivers/infiniband/hw/cxgb4/mem.c
                skb = alloc_skb(wr_len, GFP_KERNEL | __GFP_NOFAIL);
                if (!skb)
                        return -ENOMEM;

        // fs/xfs/libxfs/xfs_dir2.c
        args = kzalloc(sizeof(*args), GFP_KERNEL | __GFP_NOFAIL);
        if (!args)
                return -ENOMEM;


--
Regards
Yafang