Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211025150223.13621-1-mhocko@kernel.org> <20211025150223.13621-3-mhocko@kernel.org>
 <CA+KHdyVqOuKny7bT+CtrCk8BrnARYz744Ze6cKMuy2BXo5e7jw@mail.gmail.com>
 <YXgsxF/NRlHjH+Ng@dhcp22.suse.cz> <20211026193315.GA1860@pc638.lan>
 <20211027175550.GA1776@pc638.lan> <YXupZjQgLAi6ClRi@dhcp22.suse.cz>
In-Reply-To: <YXupZjQgLAi6ClRi@dhcp22.suse.cz>
From:   Uladzislau Rezki <urezki@gmail.com>
Date:   Fri, 29 Oct 2021 16:05:32 +0200
Message-ID: <CA+KHdyX_0B-hM8m0eZBetcdBC9X3ddnA4dMyZvA2_xCjJJeJCA@mail.gmail.com>
Subject: Re: [PATCH 2/4] mm/vmalloc: add support for __GFP_NOFAIL
To:     Michal Hocko <mhocko@suse.com>
Cc:     Linux Memory Management List <linux-mm@kvack.org>,
        Dave Chinner <david@fromorbit.com>, Neil Brown <neilb@suse.de>,
        Andrew Morton <akpm@linux-foundation.org>,
        Christoph Hellwig <hch@infradead.org>,
        linux-fsdevel@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
        Ilya Dryomov <idryomov@gmail.com>,
        Jeff Layton <jlayton@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index d77830ff604c..f4b7927e217e 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -2889,8 +2889,14 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> >       unsigned long array_size;
> >       unsigned int nr_small_pages = size >> PAGE_SHIFT;
> >       unsigned int page_order;
> > +     unsigned long flags;
> > +     int ret;
> >
> >       array_size = (unsigned long)nr_small_pages * sizeof(struct page *);
> > +
> > +     /*
> > +      * This is i do not understand why we do not want to see warning messages.
> > +      */
> >       gfp_mask |= __GFP_NOWARN;
>
> I suspect this is becauser vmalloc wants to have its own failure
> reporting.
>
But as i see it is broken. All three warn_alloc() reports in the
__vmalloc_area_node()
are useless because the __GFP_NOWARN is added on top of gfp_mask:

void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...)
{
        struct va_format vaf;
        va_list args;
        static DEFINE_RATELIMIT_STATE(nopage_rs, 10*HZ, 1);

        if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs))
                return;
...

everything with the __GFP_NOWARN is just reverted.

> [...]
> > @@ -3010,16 +3037,22 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
> >       area = __get_vm_area_node(real_size, align, shift, VM_ALLOC |
> >                                 VM_UNINITIALIZED | vm_flags, start, end, node,
> >                                 gfp_mask, caller);
> > -     if (!area) {
> > -             warn_alloc(gfp_mask, NULL,
> > -                     "vmalloc error: size %lu, vm_struct allocation failed",
> > -                     real_size);
> > -             goto fail;
> > -     }
> > +     if (area)
> > +             addr = __vmalloc_area_node(area, gfp_mask, prot, shift, node);
> > +
> > +     if (!area || !addr) {
> > +             if (gfp_mask & __GFP_NOFAIL) {
> > +                     schedule_timeout_uninterruptible(1);
> > +                     goto again;
> > +             }
> > +
> > +             if (!area)
> > +                     warn_alloc(gfp_mask, NULL,
> > +                             "vmalloc error: size %lu, vm_struct allocation failed",
> > +                             real_size);
> >
> > -     addr = __vmalloc_area_node(area, gfp_mask, prot, shift, node);
> > -     if (!addr)
> >               goto fail;
> > +     }
> >
> >       /*
> >        * In this function, newly allocated vm_struct has VM_UNINITIALIZED
> > <snip>
>
> OK, this looks easier from the code reading but isn't it quite wasteful
> to throw all the pages backing the area (all of them allocated as
> __GFP_NOFAIL) just to then fail to allocate few page tables pages and
> drop all of that on the floor (this will happen in __vunmap AFAICS).
>
> I mean I do not care all that strongly but it seems to me that more
> changes would need to be done here and optimizations can be done on top.
>
> Is this something you feel strongly about?
>
Will try to provide some motivations :)

It depends on how to look at it. My view is as follows a more simple code
is preferred. It is not considered as a hot path and it is rather a corner
case to me. I think "unwinding" has some advantage. At least one motivation
is to release a memory(on failure) before a delay that will prevent holding
of extra memory in case of __GFP_NOFAIL infinitelly does not succeed, i.e.
if a process stuck due to __GFP_NOFAIL it does not "hold" an extra memory
forever.

-- 
Uladzislau Rezki