Re: [PATCH RFC] mm: warn potential return NULL for kmalloc_array and kvmalloc_array with __GFP_NOFAIL

[Michal Hocko <mhocko@suse.com>]

On Fri 19-07-24 13:13:28, Vlastimil Babka wrote:
> On 7/19/24 12:52 PM, Michal Hocko wrote:
> > On Fri 19-07-24 12:10:13, Vlastimil Babka wrote:
> >> On 7/19/24 11:33 AM, Michal Hocko wrote:
> >> > On Fri 19-07-24 10:50:07, Vlastimil Babka wrote:
> >> > [...]
> >> >> That wouldn't mean the busy loop is a correct and supported practice. It
> >> >> would just mean it's the least bad of the bad options we have to deal with
> >> >> an allocation that's wrong but we didn't catch soon enough in the development.
> >> > 
> >> > So you want to make those potential BUG_ONs hard/soft lockups (not sure
> >> > all arches have a reliable detection) instead?
> >> 
> >> I'd expect on a SMP machine there's fair chance of being rescued by kswapd
> >> or other direct reclaimer.
> > 
> > I would expect hard/soft lockups... Anyway, the question remains. What
> > is the preferred way to express this is not really supported scenario.
> 
> The documentation and warnings? I don't think the warnings are failing us
> fundamentally. We could limit the corner cases where the are not being
> reached in case a buggy code is being executed (maybe only in some debug
> config if the checks would be too intrusive for a fast path otherwise). That
> would leave the corner cases where the buggy code is executed rarely. Maybe
> Christoph's suggestion for a build-time check would catch some of those.

I fail to see what you are actually proposing here. I can see only two
ways

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 9ecf99190ea2..d5b06ce04a0f 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4391,7 +4391,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		 * of any new users that actually require GFP_NOWAIT
 		 */
 		if (WARN_ON_ONCE_GFP(!can_direct_reclaim, gfp_mask))
-			goto fail;
+			goto retry;
 
 		/*
 		 * PF_MEMALLOC request from this context is rather bizarre

or

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 9ecf99190ea2..6ca9c4d33732 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4387,11 +4387,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)
 
 		/*
 		 * PF_MEMALLOC request from this context is rather bizarre

Choose your own poison ;)

BUILD_BUG_ON will only work for statically defined masks AFAIU (which
would catch the existing abuser) but it will not catch the code that
builds up the mask conditionaly so there needs to be a stop gap.
-- 
Michal Hocko
SUSE Labs