Re: [PATCH 01/10] mm: Assign id to every memcg-aware shrinker

[Vladimir Davydov]

On Tue, Mar 27, 2018 at 06:09:20PM +0300, Kirill Tkhai wrote:
> >>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
> >>>> index 8fcd9f8d7390..91b5120b924f 100644
> >>>> --- a/mm/vmscan.c
> >>>> +++ b/mm/vmscan.c
> >>>> @@ -159,6 +159,56 @@ unsigned long vm_total_pages;
> >>>>  static LIST_HEAD(shrinker_list);
> >>>>  static DECLARE_RWSEM(shrinker_rwsem);
> >>>>  
> >>>> +#if defined(CONFIG_MEMCG) && !defined(CONFIG_SLOB)
> >>>> +static DEFINE_IDA(bitmap_id_ida);
> >>>> +static DECLARE_RWSEM(bitmap_rwsem);
> >>>
> >>> Can't we reuse shrinker_rwsem for protecting the ida?
> >>
> >> I think it won't be better, since we allocate memory under this semaphore.
> >> After we use shrinker_rwsem, we'll have to allocate the memory with GFP_ATOMIC,
> >> which does not seems good. Currently, the patchset makes shrinker_rwsem be taken
> >> for a small time, just to assign already allocated memory to maps.
> > 
> > AFAIR it's OK to sleep under an rwsem so GFP_ATOMIC wouldn't be
> > necessary. Anyway, we only need to allocate memory when we extend
> > shrinker bitmaps, which is rare. In fact, there can only be a limited
> > number of such calls, as we never shrink these bitmaps (which is fine
> > by me).
> 
> We take bitmap_rwsem for writing to expand shrinkers maps. If we replace
> it with shrinker_rwsem and the memory allocation get into reclaim, there
> will be deadlock.

Hmm, AFAICS we use down_read_trylock() in shrink_slab() so no deadlock
would be possible. We wouldn't be able to reclaim slabs though, that's
true, but I don't think it would be a problem for small allocations.

That's how I see this. We use shrinker_rwsem to protect IDR mapping
shrink_id => shrinker (I still insist on IDR). It may allocate, but the
allocation size is going to be fairly small so it's OK that we don't
call shrinkers there. After we allocated a shrinker ID, we release
shrinker_rwsem and call mem_cgroup_grow_shrinker_map (or whatever it
will be called), which checks if per-memcg shrinker bitmaps need growing
and if they do it takes its own mutex used exclusively for protecting
the bitmaps and reallocates the bitmaps (we will need the mutex anyway
to synchronize css_online vs shrinker bitmap reallocation as the
shrinker_rwsem is private to vmscan.c and we don't want to export it
to memcontrol.c).