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

[Kirill Tkhai]

On 27.03.2018 18:48, Vladimir Davydov wrote:
> 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).

But what the profit of prohibiting reclaim during shrinker id allocation?
In case of this is a IDR, it still may require 1 page, and still may get
in after fast reclaim. If we prohibit reclaim, we'll fail to register
the shrinker.

It's not a rare situation, when all the memory is occupied by page cache.
So, we will fail to mount something in some situation.

What the advantages do we have to be more significant, than this disadvantage?

Kirill