On Thu 29-08-24 23:53:33, Barry Song wrote:

On Thu, Aug 29, 2024 at 10:24 PM Vlastimil Babka [off-list ref] wrote:

quoted

On 8/27/24 09:50, Barry Song wrote:

quoted

On Tue, Aug 27, 2024 at 7:38 PM Vlastimil Babka [off-list ref] wrote:

quoted

Ugh, wasn't aware, well spotted. So it means there at least shouldn't be
existing users of __GFP_NOFAIL with order > 1 :)

But also the check is in the hotpath, even before trying the pcplists, so we
could move it to __alloc_pages_slowpath() while extending it?

Agreed. I don't think it is reasonable to check the order and flags in
two different places especially rmqueue() has already had
gfp_flags & __GFP_NOFAIL operation and order > 1
overhead.

We can at least extend the current check to make some improvement
though I still believe Michal's suggestion of implementing OOPS_ON is a
better approach to pursue, as it doesn't crash the entire system
while ensuring the problematic process is terminated.

Linus made clear it's not a mm concern. If e.g. hardening people want to
pursuit that instead, they can.

BTW I think BUG_ON already works like this, if possible only the calling
process is terminated. panic happens in case of being in a irq context, or

you are right. This is a detail I overlooked in the last discussion.
BUG_ON has already been exactly the case to only terminate the bad
process if it can
(panic_on_oops=N and not in irq context).

Are you sure about that? Maybe x86 implementation treats BUG as oops but
is this what that does on all arches? BUG() has historically meant stop
everything and die and I am not really sure when that would have
changed TBH.

quoted

due to panic_on_oops. Which the security people are setting to 1 anyway and
OOPS_ON would have to observe it too. So AFAICS the only difference from
BUG_ON would be not panic in the irq context, if panic_on_oops isn't set.

right.

quoted

(as for "no mm locks held" I think it's already satisfied at the points we
check for __GFP_NOFAIL).

Let me summarize the discussion:

Patch 1/4, which fixes the misuse of combining gfp_nofail and atomic
in vdpa driver, is necessary.
Patch 2/4, which updates the documentation to clarify that
non-blockable gfp_nofail is not
                  supported, is needed.

Let's please have those merged now.

Patch 3/4: We will replace BUG_ON with WARN_ON_ONCE to warn when the
size is too large,
                 where gfp_nofail will return NULL.

I would pull this one out for a separate discussion. We should really
define what the too large really means and INT_MAX etc. is not it at
all.

Patch 4/4: We will move the order > 1 check from the current fast path
to the slow path and extend
                 the check of gfp_direct_reclaim flag also in the slow path.

OK, let's have that go in now as well.

-- 
Michal Hocko
SUSE Labs