[v4,3/3] mm: warn about illegal __GFP_NOFAIL usage in a more appropriate location and manner

Commit Message

Barry Song Aug. 30, 2024, 8:28 p.m. UTC

From: Barry Song <[e-mail]>

Three points for this change:

1. We should consolidate all warnings in one place. Currently, the
   order > 1 warning is in the hotpath, while others are in less
   likely scenarios. Moving all warnings to the slowpath will reduce
   the overhead for order > 1 and increase the visibility of other
   warnings.

2. We currently have two warnings for order: one for order > 1 in
   the hotpath and another for order > costly_order in the laziest
   path. I suggest standardizing on order > 1 since it’s been in
   use for a long time.

3. We don't need to check for __GFP_NOWARN in this case. __GFP_NOWARN
   is meant to suppress allocation failure reports, but here we're
   dealing with bug detection, not allocation failures. So replace
   WARN_ON_ONCE_GFP by WARN_ON_ONCE.

Suggested-by: Vlastimil Babka <[e-mail]>
Signed-off-by: Barry Song <[e-mail]>
---
 mm/page_alloc.c | 50 ++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

Comments

Vlastimil Babka Sept. 1, 2024, 8:18 p.m. UTC | #1

On 8/30/24 22:28, Barry Song wrote:
> From: Barry Song <[e-mail]>
> 
> Three points for this change:
> 
> 1. We should consolidate all warnings in one place. Currently, the
>    order > 1 warning is in the hotpath, while others are in less
>    likely scenarios. Moving all warnings to the slowpath will reduce
>    the overhead for order > 1 and increase the visibility of other
>    warnings.
> 
> 2. We currently have two warnings for order: one for order > 1 in
>    the hotpath and another for order > costly_order in the laziest
>    path. I suggest standardizing on order > 1 since it’s been in
>    use for a long time.
> 
> 3. We don't need to check for __GFP_NOWARN in this case. __GFP_NOWARN
>    is meant to suppress allocation failure reports, but here we're
>    dealing with bug detection, not allocation failures. So replace
>    WARN_ON_ONCE_GFP by WARN_ON_ONCE.
> 
> Suggested-by: Vlastimil Babka <[e-mail]>
> Signed-off-by: Barry Song <[e-mail]>

Reviewed-by: Vlastimil Babka <[e-mail]>
Thanks!

> ---
>  mm/page_alloc.c | 50 ++++++++++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c81ee5662cc7..e790b4227322 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3033,12 +3033,6 @@ struct page *rmqueue(struct zone *preferred_zone,
>  {
>  	struct page *page;
>  
> -	/*
> -	 * We most definitely don't want callers attempting to
> -	 * allocate greater than order-1 page units with __GFP_NOFAIL.
> -	 */
> -	WARN_ON_ONCE((gfp_flags & __GFP_NOFAIL) && (order > 1));
> -
>  	if (likely(pcp_allowed_order(order))) {
>  		page = rmqueue_pcplist(preferred_zone, zone, order,
>  				       migratetype, alloc_flags);
> @@ -4175,6 +4169,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  {
>  	bool can_direct_reclaim = gfp_mask & __GFP_DIRECT_RECLAIM;
>  	bool can_compact = gfp_compaction_allowed(gfp_mask);
> +	bool nofail = gfp_mask & __GFP_NOFAIL;
>  	const bool costly_order = order > PAGE_ALLOC_COSTLY_ORDER;
>  	struct page *page = NULL;
>  	unsigned int alloc_flags;
> @@ -4187,6 +4182,25 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	unsigned int zonelist_iter_cookie;
>  	int reserve_flags;
>  
> +	if (unlikely(nofail)) {
> +		/*
> +		 * We most definitely don't want callers attempting to
> +		 * allocate greater than order-1 page units with __GFP_NOFAIL.
> +		 */
> +		WARN_ON_ONCE(order > 1);
> +		/*
> +		 * Also we don't support __GFP_NOFAIL without __GFP_DIRECT_RECLAIM,
> +		 * otherwise, we may result in lockup.
> +		 */
> +		WARN_ON_ONCE(!can_direct_reclaim);
> +		/*
> +		 * PF_MEMALLOC request from this context is rather bizarre
> +		 * because we cannot reclaim anything and only can loop waiting
> +		 * for somebody to do a work for us.
> +		 */
> +		WARN_ON_ONCE(current->flags & PF_MEMALLOC);
> +	}
> +
>  restart:
>  	compaction_retries = 0;
>  	no_progress_loops = 0;
> @@ -4404,29 +4418,15 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	 * Make sure that __GFP_NOFAIL request doesn't leak out and make sure
>  	 * we always retry
>  	 */
> -	if (gfp_mask & __GFP_NOFAIL) {
> +	if (unlikely(nofail)) {
>  		/*
> -		 * All existing users of the __GFP_NOFAIL are blockable, so warn
> -		 * of any new users that actually require GFP_NOWAIT
> +		 * Lacking direct_reclaim we can't do anything to reclaim memory,
> +		 * we disregard these unreasonable nofail requests and still
> +		 * return NULL
>  		 */
> -		if (WARN_ON_ONCE_GFP(!can_direct_reclaim, gfp_mask))
> +		if (!can_direct_reclaim)
>  			goto fail;
>  
> -		/*
> -		 * PF_MEMALLOC request from this context is rather bizarre
> -		 * because we cannot reclaim anything and only can loop waiting
> -		 * for somebody to do a work for us
> -		 */
> -		WARN_ON_ONCE_GFP(current->flags & PF_MEMALLOC, gfp_mask);
> -
> -		/*
> -		 * non failing costly orders are a hard requirement which we
> -		 * are not prepared for much so let's warn about these users
> -		 * so that we can identify them and convert them to something
> -		 * else.
> -		 */
> -		WARN_ON_ONCE_GFP(costly_order, gfp_mask);
> -
>  		/*
>  		 * Help non-failing allocations by giving some access to memory
>  		 * reserves normally used for high priority non-blocking

Yafang Shao Sept. 2, 2024, 3:23 a.m. UTC | #2

On Sat, Aug 31, 2024 at 4:29 AM Barry Song <[e-mail]> wrote:
>
> From: Barry Song <[e-mail]>
>
> Three points for this change:
>
> 1. We should consolidate all warnings in one place. Currently, the
>    order > 1 warning is in the hotpath, while others are in less
>    likely scenarios. Moving all warnings to the slowpath will reduce
>    the overhead for order > 1 and increase the visibility of other
>    warnings.
>
> 2. We currently have two warnings for order: one for order > 1 in
>    the hotpath and another for order > costly_order in the laziest
>    path. I suggest standardizing on order > 1 since it’s been in
>    use for a long time.
>
> 3. We don't need to check for __GFP_NOWARN in this case. __GFP_NOWARN
>    is meant to suppress allocation failure reports, but here we're
>    dealing with bug detection, not allocation failures. So replace
>    WARN_ON_ONCE_GFP by WARN_ON_ONCE.
>
> Suggested-by: Vlastimil Babka <[e-mail]>
> Signed-off-by: Barry Song <[e-mail]>
> ---
>  mm/page_alloc.c | 50 ++++++++++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c81ee5662cc7..e790b4227322 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3033,12 +3033,6 @@ struct page *rmqueue(struct zone *preferred_zone,
>  {
>         struct page *page;
>
> -       /*
> -        * We most definitely don't want callers attempting to
> -        * allocate greater than order-1 page units with __GFP_NOFAIL.
> -        */
> -       WARN_ON_ONCE((gfp_flags & __GFP_NOFAIL) && (order > 1));
> -
>         if (likely(pcp_allowed_order(order))) {
>                 page = rmqueue_pcplist(preferred_zone, zone, order,
>                                        migratetype, alloc_flags);
> @@ -4175,6 +4169,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  {
>         bool can_direct_reclaim = gfp_mask & __GFP_DIRECT_RECLAIM;
>         bool can_compact = gfp_compaction_allowed(gfp_mask);
> +       bool nofail = gfp_mask & __GFP_NOFAIL;
>         const bool costly_order = order > PAGE_ALLOC_COSTLY_ORDER;
>         struct page *page = NULL;
>         unsigned int alloc_flags;
> @@ -4187,6 +4182,25 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>         unsigned int zonelist_iter_cookie;
>         int reserve_flags;
>
> +       if (unlikely(nofail)) {
> +               /*
> +                * We most definitely don't want callers attempting to
> +                * allocate greater than order-1 page units with __GFP_NOFAIL.
> +                */
> +               WARN_ON_ONCE(order > 1);
> +               /*
> +                * Also we don't support __GFP_NOFAIL without __GFP_DIRECT_RECLAIM,
> +                * otherwise, we may result in lockup.
> +                */
> +               WARN_ON_ONCE(!can_direct_reclaim);
> +               /*
> +                * PF_MEMALLOC request from this context is rather bizarre
> +                * because we cannot reclaim anything and only can loop waiting
> +                * for somebody to do a work for us.
> +                */
> +               WARN_ON_ONCE(current->flags & PF_MEMALLOC);

I believe we should add below warning as well:

  WARN_ON_ONCE(gfp_mask & __GFP_NOMEMALLOC);
  WARN_ON_ONCE(gfp_mask & __GFP_NORETRY);
  WARN_ON_ONCE(gfp_mask & __GFP_RETRY_MAYFAIL);
  ...

I'm not sure if that is enough.
__GFP_NOFAIL is a really horrible thing.

Barry Song Sept. 2, 2024, 4 a.m. UTC | #3

On Mon, Sep 2, 2024 at 3:23 PM Yafang Shao <[e-mail]> wrote:
>
> On Sat, Aug 31, 2024 at 4:29 AM Barry Song <[e-mail]> wrote:
> >
> > From: Barry Song <[e-mail]>
> >
> > Three points for this change:
> >
> > 1. We should consolidate all warnings in one place. Currently, the
> >    order > 1 warning is in the hotpath, while others are in less
> >    likely scenarios. Moving all warnings to the slowpath will reduce
> >    the overhead for order > 1 and increase the visibility of other
> >    warnings.
> >
> > 2. We currently have two warnings for order: one for order > 1 in
> >    the hotpath and another for order > costly_order in the laziest
> >    path. I suggest standardizing on order > 1 since it’s been in
> >    use for a long time.
> >
> > 3. We don't need to check for __GFP_NOWARN in this case. __GFP_NOWARN
> >    is meant to suppress allocation failure reports, but here we're
> >    dealing with bug detection, not allocation failures. So replace
> >    WARN_ON_ONCE_GFP by WARN_ON_ONCE.
> >
> > Suggested-by: Vlastimil Babka <[e-mail]>
> > Signed-off-by: Barry Song <[e-mail]>
> > ---
> >  mm/page_alloc.c | 50 ++++++++++++++++++++++++-------------------------
> >  1 file changed, 25 insertions(+), 25 deletions(-)
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index c81ee5662cc7..e790b4227322 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -3033,12 +3033,6 @@ struct page *rmqueue(struct zone *preferred_zone,
> >  {
> >         struct page *page;
> >
> > -       /*
> > -        * We most definitely don't want callers attempting to
> > -        * allocate greater than order-1 page units with __GFP_NOFAIL.
> > -        */
> > -       WARN_ON_ONCE((gfp_flags & __GFP_NOFAIL) && (order > 1));
> > -
> >         if (likely(pcp_allowed_order(order))) {
> >                 page = rmqueue_pcplist(preferred_zone, zone, order,
> >                                        migratetype, alloc_flags);
> > @@ -4175,6 +4169,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> >  {
> >         bool can_direct_reclaim = gfp_mask & __GFP_DIRECT_RECLAIM;
> >         bool can_compact = gfp_compaction_allowed(gfp_mask);
> > +       bool nofail = gfp_mask & __GFP_NOFAIL;
> >         const bool costly_order = order > PAGE_ALLOC_COSTLY_ORDER;
> >         struct page *page = NULL;
> >         unsigned int alloc_flags;
> > @@ -4187,6 +4182,25 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> >         unsigned int zonelist_iter_cookie;
> >         int reserve_flags;
> >
> > +       if (unlikely(nofail)) {
> > +               /*
> > +                * We most definitely don't want callers attempting to
> > +                * allocate greater than order-1 page units with __GFP_NOFAIL.
> > +                */
> > +               WARN_ON_ONCE(order > 1);
> > +               /*
> > +                * Also we don't support __GFP_NOFAIL without __GFP_DIRECT_RECLAIM,
> > +                * otherwise, we may result in lockup.
> > +                */
> > +               WARN_ON_ONCE(!can_direct_reclaim);
> > +               /*
> > +                * PF_MEMALLOC request from this context is rather bizarre
> > +                * because we cannot reclaim anything and only can loop waiting
> > +                * for somebody to do a work for us.
> > +                */
> > +               WARN_ON_ONCE(current->flags & PF_MEMALLOC);
>
> I believe we should add below warning as well:
>
>   WARN_ON_ONCE(gfp_mask & __GFP_NOMEMALLOC);
>   WARN_ON_ONCE(gfp_mask & __GFP_NORETRY);
>   WARN_ON_ONCE(gfp_mask & __GFP_RETRY_MAYFAIL);
>   ...
>
> I'm not sure if that is enough.
> __GFP_NOFAIL is a really horrible thing.

Thanks! I'd prefer to keep this patchset focused on the existing
warnings and bugs. Any new warnings about size limits or checks
for new flags can be addressed separately.

>
> --
> Regards
> Yafang

Thanks
Barry

Yafang Shao Sept. 2, 2024, 5:47 a.m. UTC | #4

On Mon, Sep 2, 2024 at 12:00 PM Barry Song <[e-mail]> wrote:
>
> On Mon, Sep 2, 2024 at 3:23 PM Yafang Shao <[e-mail]> wrote:
> >
> > On Sat, Aug 31, 2024 at 4:29 AM Barry Song <[e-mail]> wrote:
> > >
> > > From: Barry Song <[e-mail]>
> > >
> > > Three points for this change:
> > >
> > > 1. We should consolidate all warnings in one place. Currently, the
> > >    order > 1 warning is in the hotpath, while others are in less
> > >    likely scenarios. Moving all warnings to the slowpath will reduce
> > >    the overhead for order > 1 and increase the visibility of other
> > >    warnings.
> > >
> > > 2. We currently have two warnings for order: one for order > 1 in
> > >    the hotpath and another for order > costly_order in the laziest
> > >    path. I suggest standardizing on order > 1 since it’s been in
> > >    use for a long time.
> > >
> > > 3. We don't need to check for __GFP_NOWARN in this case. __GFP_NOWARN
> > >    is meant to suppress allocation failure reports, but here we're
> > >    dealing with bug detection, not allocation failures. So replace
> > >    WARN_ON_ONCE_GFP by WARN_ON_ONCE.
> > >
> > > Suggested-by: Vlastimil Babka <[e-mail]>
> > > Signed-off-by: Barry Song <[e-mail]>
> > > ---
> > >  mm/page_alloc.c | 50 ++++++++++++++++++++++++-------------------------
> > >  1 file changed, 25 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index c81ee5662cc7..e790b4227322 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -3033,12 +3033,6 @@ struct page *rmqueue(struct zone *preferred_zone,
> > >  {
> > >         struct page *page;
> > >
> > > -       /*
> > > -        * We most definitely don't want callers attempting to
> > > -        * allocate greater than order-1 page units with __GFP_NOFAIL.
> > > -        */
> > > -       WARN_ON_ONCE((gfp_flags & __GFP_NOFAIL) && (order > 1));
> > > -
> > >         if (likely(pcp_allowed_order(order))) {
> > >                 page = rmqueue_pcplist(preferred_zone, zone, order,
> > >                                        migratetype, alloc_flags);
> > > @@ -4175,6 +4169,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> > >  {
> > >         bool can_direct_reclaim = gfp_mask & __GFP_DIRECT_RECLAIM;
> > >         bool can_compact = gfp_compaction_allowed(gfp_mask);
> > > +       bool nofail = gfp_mask & __GFP_NOFAIL;
> > >         const bool costly_order = order > PAGE_ALLOC_COSTLY_ORDER;
> > >         struct page *page = NULL;
> > >         unsigned int alloc_flags;
> > > @@ -4187,6 +4182,25 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> > >         unsigned int zonelist_iter_cookie;
> > >         int reserve_flags;
> > >
> > > +       if (unlikely(nofail)) {
> > > +               /*
> > > +                * We most definitely don't want callers attempting to
> > > +                * allocate greater than order-1 page units with __GFP_NOFAIL.
> > > +                */
> > > +               WARN_ON_ONCE(order > 1);
> > > +               /*
> > > +                * Also we don't support __GFP_NOFAIL without __GFP_DIRECT_RECLAIM,
> > > +                * otherwise, we may result in lockup.
> > > +                */
> > > +               WARN_ON_ONCE(!can_direct_reclaim);
> > > +               /*
> > > +                * PF_MEMALLOC request from this context is rather bizarre
> > > +                * because we cannot reclaim anything and only can loop waiting
> > > +                * for somebody to do a work for us.
> > > +                */
> > > +               WARN_ON_ONCE(current->flags & PF_MEMALLOC);
> >
> > I believe we should add below warning as well:
> >
> >   WARN_ON_ONCE(gfp_mask & __GFP_NOMEMALLOC);
> >   WARN_ON_ONCE(gfp_mask & __GFP_NORETRY);
> >   WARN_ON_ONCE(gfp_mask & __GFP_RETRY_MAYFAIL);
> >   ...
> >
> > I'm not sure if that is enough.
> > __GFP_NOFAIL is a really horrible thing.
>
> Thanks! I'd prefer to keep this patchset focused on the existing
> warnings and bugs. Any new warnings about size limits or checks
> for new flags can be addressed separately.

OK
Thanks for your work.

David Hildenbrand Sept. 2, 2024, 7:40 a.m. UTC | #5

On 30.08.24 22:28, Barry Song wrote:
> From: Barry Song <[e-mail]>
> 
> Three points for this change:
> 
> 1. We should consolidate all warnings in one place. Currently, the
>     order > 1 warning is in the hotpath, while others are in less
>     likely scenarios. Moving all warnings to the slowpath will reduce
>     the overhead for order > 1 and increase the visibility of other
>     warnings.
> 
> 2. We currently have two warnings for order: one for order > 1 in
>     the hotpath and another for order > costly_order in the laziest
>     path. I suggest standardizing on order > 1 since it’s been in
>     use for a long time.
> 
> 3. We don't need to check for __GFP_NOWARN in this case. __GFP_NOWARN
>     is meant to suppress allocation failure reports, but here we're
>     dealing with bug detection, not allocation failures. So replace
>     WARN_ON_ONCE_GFP by WARN_ON_ONCE.
> 
> Suggested-by: Vlastimil Babka <[e-mail]>
> Signed-off-by: Barry Song <[e-mail]>
> ---
>   mm/page_alloc.c | 50 ++++++++++++++++++++++++-------------------------
>   1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c81ee5662cc7..e790b4227322 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3033,12 +3033,6 @@ struct page *rmqueue(struct zone *preferred_zone,
>   {
>   	struct page *page;
>   
> -	/*
> -	 * We most definitely don't want callers attempting to
> -	 * allocate greater than order-1 page units with __GFP_NOFAIL.
> -	 */
> -	WARN_ON_ONCE((gfp_flags & __GFP_NOFAIL) && (order > 1));
> -
>   	if (likely(pcp_allowed_order(order))) {
>   		page = rmqueue_pcplist(preferred_zone, zone, order,
>   				       migratetype, alloc_flags);
> @@ -4175,6 +4169,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>   {
>   	bool can_direct_reclaim = gfp_mask & __GFP_DIRECT_RECLAIM;
>   	bool can_compact = gfp_compaction_allowed(gfp_mask);
> +	bool nofail = gfp_mask & __GFP_NOFAIL;
>   	const bool costly_order = order > PAGE_ALLOC_COSTLY_ORDER;
>   	struct page *page = NULL;
>   	unsigned int alloc_flags;
> @@ -4187,6 +4182,25 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>   	unsigned int zonelist_iter_cookie;
>   	int reserve_flags;
>   
> +	if (unlikely(nofail)) {
> +		/*
> +		 * We most definitely don't want callers attempting to
> +		 * allocate greater than order-1 page units with __GFP_NOFAIL.
> +		 */

Acked-by: David Hildenbrand <[e-mail]>

Should we also clarify that in the docs? Currently we have "Using this 
flag for costly allocations is _highly_ discouraged."

We'd likely want to say something like "Allocating pages from the buddy 
with __GFP_NOFAIL and order > 1 is not supported."

Michal Hocko Sept. 2, 2024, 7:58 a.m. UTC | #6

On Sat 31-08-24 08:28:23, Barry Song wrote:
> From: Barry Song <[e-mail]>
> 
> Three points for this change:
> 
> 1. We should consolidate all warnings in one place. Currently, the
>    order > 1 warning is in the hotpath, while others are in less
>    likely scenarios. Moving all warnings to the slowpath will reduce
>    the overhead for order > 1 and increase the visibility of other
>    warnings.
> 
> 2. We currently have two warnings for order: one for order > 1 in
>    the hotpath and another for order > costly_order in the laziest
>    path. I suggest standardizing on order > 1 since it’s been in
>    use for a long time.
> 
> 3. We don't need to check for __GFP_NOWARN in this case. __GFP_NOWARN
>    is meant to suppress allocation failure reports, but here we're
>    dealing with bug detection, not allocation failures. So replace
>    WARN_ON_ONCE_GFP by WARN_ON_ONCE.
> 
> Suggested-by: Vlastimil Babka <[e-mail]>
> Signed-off-by: Barry Song <[e-mail]>

Acked-by: Michal Hocko <[e-mail]>

Updating the doc about order > 1 sounds like it would still fall into
the scope of this patch. I don not think we absolutely have to document
each unsupported gfp flags combination for GFP_NOFAIL but the order is a
good addition with a note that kvmalloc should be used instead in such a
case.

> ---
>  mm/page_alloc.c | 50 ++++++++++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c81ee5662cc7..e790b4227322 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3033,12 +3033,6 @@ struct page *rmqueue(struct zone *preferred_zone,
>  {
>  	struct page *page;
>  
> -	/*
> -	 * We most definitely don't want callers attempting to
> -	 * allocate greater than order-1 page units with __GFP_NOFAIL.
> -	 */
> -	WARN_ON_ONCE((gfp_flags & __GFP_NOFAIL) && (order > 1));
> -
>  	if (likely(pcp_allowed_order(order))) {
>  		page = rmqueue_pcplist(preferred_zone, zone, order,
>  				       migratetype, alloc_flags);
> @@ -4175,6 +4169,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  {
>  	bool can_direct_reclaim = gfp_mask & __GFP_DIRECT_RECLAIM;
>  	bool can_compact = gfp_compaction_allowed(gfp_mask);
> +	bool nofail = gfp_mask & __GFP_NOFAIL;
>  	const bool costly_order = order > PAGE_ALLOC_COSTLY_ORDER;
>  	struct page *page = NULL;
>  	unsigned int alloc_flags;
> @@ -4187,6 +4182,25 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	unsigned int zonelist_iter_cookie;
>  	int reserve_flags;
>  
> +	if (unlikely(nofail)) {
> +		/*
> +		 * We most definitely don't want callers attempting to
> +		 * allocate greater than order-1 page units with __GFP_NOFAIL.
> +		 */
> +		WARN_ON_ONCE(order > 1);
> +		/*
> +		 * Also we don't support __GFP_NOFAIL without __GFP_DIRECT_RECLAIM,
> +		 * otherwise, we may result in lockup.
> +		 */
> +		WARN_ON_ONCE(!can_direct_reclaim);
> +		/*
> +		 * PF_MEMALLOC request from this context is rather bizarre
> +		 * because we cannot reclaim anything and only can loop waiting
> +		 * for somebody to do a work for us.
> +		 */
> +		WARN_ON_ONCE(current->flags & PF_MEMALLOC);
> +	}
> +
>  restart:
>  	compaction_retries = 0;
>  	no_progress_loops = 0;
> @@ -4404,29 +4418,15 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	 * Make sure that __GFP_NOFAIL request doesn't leak out and make sure
>  	 * we always retry
>  	 */
> -	if (gfp_mask & __GFP_NOFAIL) {
> +	if (unlikely(nofail)) {
>  		/*
> -		 * All existing users of the __GFP_NOFAIL are blockable, so warn
> -		 * of any new users that actually require GFP_NOWAIT
> +		 * Lacking direct_reclaim we can't do anything to reclaim memory,
> +		 * we disregard these unreasonable nofail requests and still
> +		 * return NULL
>  		 */
> -		if (WARN_ON_ONCE_GFP(!can_direct_reclaim, gfp_mask))
> +		if (!can_direct_reclaim)
>  			goto fail;
>  
> -		/*
> -		 * PF_MEMALLOC request from this context is rather bizarre
> -		 * because we cannot reclaim anything and only can loop waiting
> -		 * for somebody to do a work for us
> -		 */
> -		WARN_ON_ONCE_GFP(current->flags & PF_MEMALLOC, gfp_mask);
> -
> -		/*
> -		 * non failing costly orders are a hard requirement which we
> -		 * are not prepared for much so let's warn about these users
> -		 * so that we can identify them and convert them to something
> -		 * else.
> -		 */
> -		WARN_ON_ONCE_GFP(costly_order, gfp_mask);
> -
>  		/*
>  		 * Help non-failing allocations by giving some access to memory
>  		 * reserves normally used for high priority non-blocking
> -- 
> 2.34.1

Barry Song Sept. 3, 2024, 10:39 p.m. UTC | #7

On Mon, Sep 2, 2024 at 7:58 PM Michal Hocko <[e-mail]> wrote:
>
> On Sat 31-08-24 08:28:23, Barry Song wrote:
> > From: Barry Song <[e-mail]>
> >
> > Three points for this change:
> >
> > 1. We should consolidate all warnings in one place. Currently, the
> >    order > 1 warning is in the hotpath, while others are in less
> >    likely scenarios. Moving all warnings to the slowpath will reduce
> >    the overhead for order > 1 and increase the visibility of other
> >    warnings.
> >
> > 2. We currently have two warnings for order: one for order > 1 in
> >    the hotpath and another for order > costly_order in the laziest
> >    path. I suggest standardizing on order > 1 since it’s been in
> >    use for a long time.
> >
> > 3. We don't need to check for __GFP_NOWARN in this case. __GFP_NOWARN
> >    is meant to suppress allocation failure reports, but here we're
> >    dealing with bug detection, not allocation failures. So replace
> >    WARN_ON_ONCE_GFP by WARN_ON_ONCE.
> >
> > Suggested-by: Vlastimil Babka <[e-mail]>
> > Signed-off-by: Barry Song <[e-mail]>
>
> Acked-by: Michal Hocko <[e-mail]>
>
> Updating the doc about order > 1 sounds like it would still fall into
> the scope of this patch. I don not think we absolutely have to document
> each unsupported gfp flags combination for GFP_NOFAIL but the order is a
> good addition with a note that kvmalloc should be used instead in such a
> case.

Hi Andrew,
If there are no objections from Michal and David, could you please
squash the following:

From fc7a2a49e8d0811d706d13d2080393274f316806 Mon Sep 17 00:00:00 2001
From: Barry Song <[e-mail]>
Date: Wed, 4 Sep 2024 10:26:19 +1200
Subject: [PATCH] mm: also update the doc for __GFP_NOFAIL with order > 1

Obviously we only support order <= 1 __GFP_NOFAIL allocation and if
someone wants larger memory, they should consider using kvmalloc()
instead.

Suggested-by: David Hildenbrand <[e-mail]>
Suggested-by: Michal Hocko <[e-mail]>
Signed-off-by: Barry Song <[e-mail]>
---
 include/linux/gfp_types.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/gfp_types.h b/include/linux/gfp_types.h
index 4a1fa7706b0c..65db9349f905 100644
--- a/include/linux/gfp_types.h
+++ b/include/linux/gfp_types.h
@@ -253,7 +253,8 @@ enum {
  * used only when there is no reasonable failure policy) but it is
  * definitely preferable to use the flag rather than opencode endless
  * loop around allocator.
- * Using this flag for costly allocations is _highly_ discouraged.
+ * Allocating pages from the buddy with __GFP_NOFAIL and order > 1 is
+ * not supported. Please consider using kvmalloc() instead.
  */
 #define __GFP_IO	((__force gfp_t)___GFP_IO)
 #define __GFP_FS	((__force gfp_t)___GFP_FS)

Michal Hocko Sept. 4, 2024, 7:22 a.m. UTC | #8

On Wed 04-09-24 10:39:35, Barry Song wrote:
> On Mon, Sep 2, 2024 at 7:58 PM Michal Hocko <[e-mail]> wrote:
> >
> > On Sat 31-08-24 08:28:23, Barry Song wrote:
> > > From: Barry Song <[e-mail]>
> > >
> > > Three points for this change:
> > >
> > > 1. We should consolidate all warnings in one place. Currently, the
> > >    order > 1 warning is in the hotpath, while others are in less
> > >    likely scenarios. Moving all warnings to the slowpath will reduce
> > >    the overhead for order > 1 and increase the visibility of other
> > >    warnings.
> > >
> > > 2. We currently have two warnings for order: one for order > 1 in
> > >    the hotpath and another for order > costly_order in the laziest
> > >    path. I suggest standardizing on order > 1 since it’s been in
> > >    use for a long time.
> > >
> > > 3. We don't need to check for __GFP_NOWARN in this case. __GFP_NOWARN
> > >    is meant to suppress allocation failure reports, but here we're
> > >    dealing with bug detection, not allocation failures. So replace
> > >    WARN_ON_ONCE_GFP by WARN_ON_ONCE.
> > >
> > > Suggested-by: Vlastimil Babka <[e-mail]>
> > > Signed-off-by: Barry Song <[e-mail]>
> >
> > Acked-by: Michal Hocko <[e-mail]>
> >
> > Updating the doc about order > 1 sounds like it would still fall into
> > the scope of this patch. I don not think we absolutely have to document
> > each unsupported gfp flags combination for GFP_NOFAIL but the order is a
> > good addition with a note that kvmalloc should be used instead in such a
> > case.
> 
> Hi Andrew,
> If there are no objections from Michal and David, could you please
> squash the following:
> 
> >From fc7a2a49e8d0811d706d13d2080393274f316806 Mon Sep 17 00:00:00 2001
> From: Barry Song <[e-mail]>
> Date: Wed, 4 Sep 2024 10:26:19 +1200
> Subject: [PATCH] mm: also update the doc for __GFP_NOFAIL with order > 1
> 
> Obviously we only support order <= 1 __GFP_NOFAIL allocation and if
> someone wants larger memory, they should consider using kvmalloc()
> instead.
> 
> Suggested-by: David Hildenbrand <[e-mail]>
> Suggested-by: Michal Hocko <[e-mail]>
> Signed-off-by: Barry Song <[e-mail]>

LGTM. Thanks!

> ---
>  include/linux/gfp_types.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/gfp_types.h b/include/linux/gfp_types.h
> index 4a1fa7706b0c..65db9349f905 100644
> --- a/include/linux/gfp_types.h
> +++ b/include/linux/gfp_types.h
> @@ -253,7 +253,8 @@ enum {
>   * used only when there is no reasonable failure policy) but it is
>   * definitely preferable to use the flag rather than opencode endless
>   * loop around allocator.
> - * Using this flag for costly allocations is _highly_ discouraged.
> + * Allocating pages from the buddy with __GFP_NOFAIL and order > 1 is
> + * not supported. Please consider using kvmalloc() instead.
>   */
>  #define __GFP_IO	((__force gfp_t)___GFP_IO)
>  #define __GFP_FS	((__force gfp_t)___GFP_FS)
> -- 
> 2.34.1
> 
> 
> >
> > > ---
> > >  mm/page_alloc.c | 50 ++++++++++++++++++++++++-------------------------
> > >  1 file changed, 25 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index c81ee5662cc7..e790b4227322 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -3033,12 +3033,6 @@ struct page *rmqueue(struct zone *preferred_zone,
> > >  {
> > >       struct page *page;
> > > 
> > > -     /*
> > > -      * We most definitely don't want callers attempting to
> > > -      * allocate greater than order-1 page units with __GFP_NOFAIL.
> > > -      */
> > > -     WARN_ON_ONCE((gfp_flags & __GFP_NOFAIL) && (order > 1));
> > > -
> > >       if (likely(pcp_allowed_order(order))) {
> > >               page = rmqueue_pcplist(preferred_zone, zone, order,
> > >                                      migratetype, alloc_flags);
> > > @@ -4175,6 +4169,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> > >  {
> > >       bool can_direct_reclaim = gfp_mask & __GFP_DIRECT_RECLAIM;
> > >       bool can_compact = gfp_compaction_allowed(gfp_mask);
> > > +     bool nofail = gfp_mask & __GFP_NOFAIL;
> > >       const bool costly_order = order > PAGE_ALLOC_COSTLY_ORDER;
> > >       struct page *page = NULL;
> > >       unsigned int alloc_flags;
> > > @@ -4187,6 +4182,25 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> > >       unsigned int zonelist_iter_cookie;
> > >       int reserve_flags;
> > > 
> > > +     if (unlikely(nofail)) {
> > > +             /*
> > > +              * We most definitely don't want callers attempting to
> > > +              * allocate greater than order-1 page units with __GFP_NOFAIL.
> > > +              */
> > > +             WARN_ON_ONCE(order > 1);
> > > +             /*
> > > +              * Also we don't support __GFP_NOFAIL without __GFP_DIRECT_RECLAIM,
> > > +              * otherwise, we may result in lockup.
> > > +              */
> > > +             WARN_ON_ONCE(!can_direct_reclaim);
> > > +             /*
> > > +              * PF_MEMALLOC request from this context is rather bizarre
> > > +              * because we cannot reclaim anything and only can loop waiting
> > > +              * for somebody to do a work for us.
> > > +              */
> > > +             WARN_ON_ONCE(current->flags & PF_MEMALLOC);
> > > +     }
> > > +
> > >  restart:
> > >       compaction_retries = 0;
> > >       no_progress_loops = 0;
> > > @@ -4404,29 +4418,15 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> > >        * Make sure that __GFP_NOFAIL request doesn't leak out and make sure
> > >        * we always retry
> > >        */
> > > -     if (gfp_mask & __GFP_NOFAIL) {
> > > +     if (unlikely(nofail)) {
> > >               /*
> > > -              * All existing users of the __GFP_NOFAIL are blockable, so warn
> > > -              * of any new users that actually require GFP_NOWAIT
> > > +              * Lacking direct_reclaim we can't do anything to reclaim memory,
> > > +              * we disregard these unreasonable nofail requests and still
> > > +              * return NULL
> > >                */
> > > -             if (WARN_ON_ONCE_GFP(!can_direct_reclaim, gfp_mask))
> > > +             if (!can_direct_reclaim)
> > >                       goto fail;
> > > 
> > > -             /*
> > > -              * PF_MEMALLOC request from this context is rather bizarre
> > > -              * because we cannot reclaim anything and only can loop waiting
> > > -              * for somebody to do a work for us
> > > -              */
> > > -             WARN_ON_ONCE_GFP(current->flags & PF_MEMALLOC, gfp_mask);
> > > -
> > > -             /*
> > > -              * non failing costly orders are a hard requirement which we
> > > -              * are not prepared for much so let's warn about these users
> > > -              * so that we can identify them and convert them to something
> > > -              * else.
> > > -              */
> > > -             WARN_ON_ONCE_GFP(costly_order, gfp_mask);
> > > -
> > >               /*
> > >                * Help non-failing allocations by giving some access to memory
> > >                * reserves normally used for high priority non-blocking
> > > --
> > > 2.34.1
> >
> > --
> > Michal Hocko
> > SUSE Labs
> 
> Thanks
> Barry

Patch

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c81ee5662cc7..e790b4227322 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3033,12 +3033,6 @@  struct page *rmqueue(struct zone *preferred_zone,
 {
 	struct page *page;
 
-	/*
-	 * We most definitely don't want callers attempting to
-	 * allocate greater than order-1 page units with __GFP_NOFAIL.
-	 */
-	WARN_ON_ONCE((gfp_flags & __GFP_NOFAIL) && (order > 1));
-
 	if (likely(pcp_allowed_order(order))) {
 		page = rmqueue_pcplist(preferred_zone, zone, order,
 				       migratetype, alloc_flags);
@@ -4175,6 +4169,7 @@  __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 {
 	bool can_direct_reclaim = gfp_mask & __GFP_DIRECT_RECLAIM;
 	bool can_compact = gfp_compaction_allowed(gfp_mask);
+	bool nofail = gfp_mask & __GFP_NOFAIL;
 	const bool costly_order = order > PAGE_ALLOC_COSTLY_ORDER;
 	struct page *page = NULL;
 	unsigned int alloc_flags;
@@ -4187,6 +4182,25 @@  __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	unsigned int zonelist_iter_cookie;
 	int reserve_flags;
 
+	if (unlikely(nofail)) {
+		/*
+		 * We most definitely don't want callers attempting to
+		 * allocate greater than order-1 page units with __GFP_NOFAIL.
+		 */
+		WARN_ON_ONCE(order > 1);
+		/*
+		 * Also we don't support __GFP_NOFAIL without __GFP_DIRECT_RECLAIM,
+		 * otherwise, we may result in lockup.
+		 */
+		WARN_ON_ONCE(!can_direct_reclaim);
+		/*
+		 * PF_MEMALLOC request from this context is rather bizarre
+		 * because we cannot reclaim anything and only can loop waiting
+		 * for somebody to do a work for us.
+		 */
+		WARN_ON_ONCE(current->flags & PF_MEMALLOC);
+	}
+
 restart:
 	compaction_retries = 0;
 	no_progress_loops = 0;
@@ -4404,29 +4418,15 @@  __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	 * Make sure that __GFP_NOFAIL request doesn't leak out and make sure
 	 * we always retry
 	 */
-	if (gfp_mask & __GFP_NOFAIL) {
+	if (unlikely(nofail)) {
 		/*
-		 * All existing users of the __GFP_NOFAIL are blockable, so warn
-		 * of any new users that actually require GFP_NOWAIT
+		 * Lacking direct_reclaim we can't do anything to reclaim memory,
+		 * we disregard these unreasonable nofail requests and still
+		 * return NULL
 		 */
-		if (WARN_ON_ONCE_GFP(!can_direct_reclaim, gfp_mask))
+		if (!can_direct_reclaim)
 			goto fail;
 
-		/*
-		 * PF_MEMALLOC request from this context is rather bizarre
-		 * because we cannot reclaim anything and only can loop waiting
-		 * for somebody to do a work for us
-		 */
-		WARN_ON_ONCE_GFP(current->flags & PF_MEMALLOC, gfp_mask);
-
-		/*
-		 * non failing costly orders are a hard requirement which we
-		 * are not prepared for much so let's warn about these users
-		 * so that we can identify them and convert them to something
-		 * else.
-		 */
-		WARN_ON_ONCE_GFP(costly_order, gfp_mask);
-
 		/*
 		 * Help non-failing allocations by giving some access to memory
 		 * reserves normally used for high priority non-blocking