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

[Uladzislau Rezki <urezki@gmail.com>]

> [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