Re: [PATCH 3/4] mm/vmalloc: be more explicit about supported gfp flags.

From: Michal Hocko <mhocko@suse.com>
To: NeilBrown <neilb@suse.de>
Cc: linux-mm@kvack.org, Dave Chinner <david@fromorbit.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Christoph Hellwig <hch@infradead.org>,
	Uladzislau Rezki <urezki@gmail.com>,
	linux-fsdevel@vger.kernel.org,
	LKML <linux-kernel@vger.kernel.org>,
	Ilya Dryomov <idryomov@gmail.com>,
	Jeff Layton <jlayton@kernel.org>
Subject: Re: [PATCH 3/4] mm/vmalloc: be more explicit about supported gfp flags.
Date: Tue, 26 Oct 2021 09:10:13 +0200	[thread overview]
Message-ID: <YXep1ctN1wPP+1a8@dhcp22.suse.cz> (raw)
In-Reply-To: <163520436674.16092.18372437960890952300@noble.neil.brown.name>

On Tue 26-10-21 10:26:06, Neil Brown wrote:
> On Tue, 26 Oct 2021, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > The core of the vmalloc allocator __vmalloc_area_node doesn't say
> > anything about gfp mask argument. Not all gfp flags are supported
> > though. Be more explicit about constrains.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > ---
> >  mm/vmalloc.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index 602649919a9d..2199d821c981 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -2980,8 +2980,16 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> >   * @caller:		  caller's return address
> >   *
> >   * Allocate enough pages to cover @size from the page level
> > - * allocator with @gfp_mask flags.  Map them into contiguous
> > - * kernel virtual space, using a pagetable protection of @prot.
> > + * allocator with @gfp_mask flags. Please note that the full set of gfp
> > + * flags are not supported. GFP_KERNEL would be a preferred allocation mode
> > + * but GFP_NOFS and GFP_NOIO are supported as well. Zone modifiers are not
> 
> In what sense is GFP_KERNEL "preferred"??
> The choice of GFP_NOFS, when necessary, isn't based on preference but
> on need.
> 
> I understand that you would prefer no one ever used GFP_NOFs ever - just
> use the scope API.  I even agree.  But this is not the place to make
> that case. 

Any suggestion for a better wording?

> > + * supported. From the reclaim modifiers__GFP_DIRECT_RECLAIM is required (aka
> > + * GFP_NOWAIT is not supported) and only __GFP_NOFAIL is supported (aka
> 
> I don't think "aka" is the right thing to use here.  It is short for
> "also known as" and there is nothing that is being known as something
> else.
> It would be appropriate to say (i.e. GFP_NOWAIT is not supported).
> "i.e." is short for the Latin "id est" which means "that is" and
> normally introduces an alternate description (whereas aka introduces an
> alternate name).

OK

> > + * __GFP_NORETRY and __GFP_RETRY_MAYFAIL are not supported).
> 
> Why do you think __GFP_NORETRY and __GFP_RETRY_MAYFAIL are not supported.

Because they cannot be passed to the page table allocator. In both cases
the allocation would fail when system is short on memory. GFP_KERNEL
used for ptes implicitly doesn't behave that way.

> 
> > + * __GFP_NOWARN can be used to suppress error messages about failures.
> 
> Surely "NOWARN" suppresses warning messages, not error messages ....

I am not sure I follow. NOWARN means "do not warn" independently on the
log level chosen for the message. Is an allocation failure an error
message? Is the "vmalloc error: size %lu, failed to map pages" an error
message?

Anyway I will go with "__GFP_NOWARN can be used to suppress failure messages"

Is that better?
-- 
Michal Hocko
SUSE Labs