Re: [PATCH] mm/vmalloc: reduce the number of lazy_max_pages to reduce latency

[Joel Fernandes]

On Sun, Oct 9, 2016 at 5:42 AM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
[..]
>> > My understanding is that
>> >
>> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>> > index 91f44e78c516..3f7c6d6969ac 100644
>> > --- a/mm/vmalloc.c
>> > +++ b/mm/vmalloc.c
>> > @@ -626,7 +626,6 @@ void set_iounmap_nonlazy(void)
>> >  static void __purge_vmap_area_lazy(unsigned long *start, unsigned long *end,
>> >                                         int sync, int force_flush)
>> >  {
>> > -       static DEFINE_SPINLOCK(purge_lock);
>> >         struct llist_node *valist;
>> >         struct vmap_area *va;
>> >         struct vmap_area *n_va;
>> > @@ -637,12 +636,6 @@ static void __purge_vmap_area_lazy(unsigned long *start, unsigned long *end,
>> >          * should not expect such behaviour. This just simplifies locking for
>> >          * the case that isn't actually used at the moment anyway.
>> >          */
>> > -       if (!sync && !force_flush) {
>> > -               if (!spin_trylock(&purge_lock))
>> > -                       return;
>> > -       } else
>> > -               spin_lock(&purge_lock);
>> > -
>> >         if (sync)
>> >                 purge_fragmented_blocks_allcpus();
>> >
>> > @@ -667,7 +660,6 @@ static void __purge_vmap_area_lazy(unsigned long *start, unsigned long *end,
>> >                         __free_vmap_area(va);
>> >                 spin_unlock(&vmap_area_lock);
>> >         }
>> > -       spin_unlock(&purge_lock);
>> >  }
>> >
>> [..]
>> > should now be safe. That should significantly reduce the preempt-disabled
>> > section, I think.
>>
>> I believe that the purge_lock is supposed to prevent concurrent purges
>> from happening.
>>
>> For the case where if you have another concurrent overflow happen in
>> alloc_vmap_area() between the spin_unlock and purge :
>>
>> spin_unlock(&vmap_area_lock);
>> if (!purged)
>>    purge_vmap_area_lazy();
>>
>> Then the 2 purges would happen at the same time and could subtract
>> vmap_lazy_nr twice.
>
> That itself is not the problem, as each instance of
> __purge_vmap_area_lazy() operates on its own freelist, and so there will
> be no double accounting.
>
> However, removing the lock removes the serialisation which does mean
> that alloc_vmap_area() will not block on another thread conducting the
> purge, and so it will try to reallocate before that is complete and the
> free area made available. It also means that we are doing the
> atomic_sub(vmap_lazy_nr) too early.
>
> That supports making the outer lock a mutex as you suggested. But I think
> cond_resched_lock() is better for the vmap_area_lock (just because it
> turns out to be an expensive loop and we may want the reschedule).
> -Chris

Ok. So I'll submit a patch with mutex for purge_lock and use
cond_resched_lock for the vmap_area_lock as you suggested. I'll also
drop the lazy_max_pages to 8MB as Andi suggested to reduce the lock
hold time. Let me know if you have any objections.

Thanks,
Joel