Re: [PATCH] mm/vmalloc: Eliminate an extra orig_gfp_mask

From: Uladzislau Rezki <urezki@gmail.com>
To: Michal Hocko <mhocko@suse.com>
Cc: "Uladzislau Rezki (Sony)" <urezki@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-mm@kvack.org, LKML <linux-kernel@vger.kernel.org>,
	Mel Gorman <mgorman@suse.de>,
	Christoph Hellwig <hch@infradead.org>,
	Matthew Wilcox <willy@infradead.org>,
	Nicholas Piggin <npiggin@gmail.com>,
	Hillf Danton <hdanton@sina.com>,
	Oleksiy Avramchenko <oleksiy.avramchenko@sonymobile.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Vasily Averin <vvs@virtuozzo.com>
Subject: Re: [PATCH] mm/vmalloc: Eliminate an extra orig_gfp_mask
Date: Thu, 4 Nov 2021 12:14:14 +0100	[thread overview]
Message-ID: <20211104111414.GA1928@pc638.lan> (raw)
In-Reply-To: <YYOhBGACLb+p1jl0@dhcp22.suse.cz>

> [Cc Vasily]
> 
> On Wed 03-11-21 21:07:03, Uladzislau Rezki wrote:
> > That extra variable has been introduced just for keeping an original
> > passed gfp_mask because it is updated with __GFP_NOWARN on entry, thus
> > error handling messages were broken.
> 
> I am not sure what you mean by "error handling messages were broken"
> part.
> 
We slightly discussed it in another thread :) There was __GFP_NOWARN added
on entry(unconditionally), what leads to ignoring all our internal error
messages by the warn_alloc(). I have checked the linux-next and saw that
Vasily sent a patch fixing it:

<snip>
Author: Vasily Averin <vvs@virtuozzo.com>
Date:   Thu Oct 21 15:07:26 2021 +1100

    mm/vmalloc: repair warn_alloc()s in __vmalloc_area_node()

    Commit f255935b9767 ("mm: cleanup the gfp_mask handling in
    __vmalloc_area_node") added __GFP_NOWARN to gfp_mask unconditionally
    however it disabled all output inside warn_alloc() call.  This patch saves
    original gfp_mask and provides it to all warn_alloc() calls.
<snip>

> It is true that the current Linus tree has a broken allocation failure
> reporting but that is not a fault of orig_gfp_mask. In fact patch which
> is fixing that "mm/vmalloc: repair warn_alloc()s in
> __vmalloc_area_node()" currently in akpm tree is adding the additional
> mask.
>  
> > Instead we can keep an original gfp_mask without modifying it and add
> > an extra __GFP_NOWARN flag together with gfp_mask as a parameter to
> > the vm_area_alloc_pages() function. It will make it less confused.
> 
> I would tend to agree that this is a better approach. There is already
> nested_gfp mask and one more doesn't add to the readbility.
> 
Agree, that is why i decided to send a patch. Because i find that extra
gfp variable as odd one and confusing. I paid an attention on it during
our discussion about __GFP_NOFAIL. But on my tree it was not fixed at all
and after checking the linux-next i saw a fix.

>
> Maybe we should just drop the above patch and just go with one which
> doesn't introduce the intermediate step and an additional gfp mask.
> 
That we can do if all agree on. 

Thanks!

--
Vlad Rezki