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