Re: [PATCH 2/4] mm/vmalloc: add support for __GFP_NOFAIL

[Uladzislau Rezki <urezki@gmail.com>]

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