Re: [PATCH 6/9] mm: vmscan: use per memcg nr_deferred of shrinker

[Kirill Tkhai]

On 10.12.2020 18:13, Johannes Weiner wrote:
> On Wed, Dec 09, 2020 at 09:32:37AM -0800, Yang Shi wrote:
>> On Wed, Dec 9, 2020 at 7:42 AM Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> wrote:
>>>
>>> On 08.12.2020 20:13, Yang Shi wrote:
>>>> On Thu, Dec 3, 2020 at 3:40 AM Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> wrote:
>>>>>
>>>>> On 02.12.2020 21:27, Yang Shi wrote:
>>>>>> Use per memcg's nr_deferred for memcg aware shrinkers.  The shrinker's nr_deferred
>>>>>> will be used in the following cases:
>>>>>>     1. Non memcg aware shrinkers
>>>>>>     2. !CONFIG_MEMCG
>>>>>>     3. memcg is disabled by boot parameter
>>>>>>
>>>>>> Signed-off-by: Yang Shi <shy828301@xxxxxxxxx>
>>>>>> ---
>>>>>>  mm/vmscan.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++----
>>>>>>  1 file changed, 82 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>>>>> index cba0bc8d4661..d569fdcaba79 100644
>>>>>> --- a/mm/vmscan.c
>>>>>> +++ b/mm/vmscan.c
>>>>>> @@ -203,6 +203,12 @@ static DECLARE_RWSEM(shrinker_rwsem);
>>>>>>  static DEFINE_IDR(shrinker_idr);
>>>>>>  static int shrinker_nr_max;
>>>>>>
>>>>>> +static inline bool is_deferred_memcg_aware(struct shrinker *shrinker)
>>>>>> +{
>>>>>> +     return (shrinker->flags & SHRINKER_MEMCG_AWARE) &&
>>>>>> +             !mem_cgroup_disabled();
>>>>>> +}
>>>>>> +
>>>>>>  static int prealloc_memcg_shrinker(struct shrinker *shrinker)
>>>>>>  {
>>>>>>       int id, ret = -ENOMEM;
>>>>>> @@ -271,7 +277,58 @@ static bool writeback_throttling_sane(struct scan_control *sc)
>>>>>>  #endif
>>>>>>       return false;
>>>>>>  }
>>>>>> +
>>>>>> +static inline long count_nr_deferred(struct shrinker *shrinker,
>>>>>> +                                  struct shrink_control *sc)
>>>>>> +{
>>>>>> +     bool per_memcg_deferred = is_deferred_memcg_aware(shrinker) && sc->memcg;
>>>>>> +     struct memcg_shrinker_deferred *deferred;
>>>>>> +     struct mem_cgroup *memcg = sc->memcg;
>>>>>> +     int nid = sc->nid;
>>>>>> +     int id = shrinker->id;
>>>>>> +     long nr;
>>>>>> +
>>>>>> +     if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
>>>>>> +             nid = 0;
>>>>>> +
>>>>>> +     if (per_memcg_deferred) {
>>>>>> +             deferred = rcu_dereference_protected(memcg->nodeinfo[nid]->shrinker_deferred,
>>>>>> +                                                  true);
>>>>>
>>>>> My comment is about both 5/9 and 6/9 patches.
>>>>
>>>> Sorry for the late reply, I don't know why Gmail filtered this out to spam.
>>>>
>>>>>
>>>>> shrink_slab_memcg() races with mem_cgroup_css_online(). A visibility of CSS_ONLINE flag
>>>>> in shrink_slab_memcg()->mem_cgroup_online() does not guarantee that you will see
>>>>> memcg->nodeinfo[nid]->shrinker_deferred != NULL in count_nr_deferred(). This may occur
>>>>> because of processor reordering on !x86 (there is no a common lock or memory barriers).
>>>>>
>>>>> Regarding to shrinker_map this is not a problem due to map check in shrink_slab_memcg().
>>>>> The map can't be NULL there.
>>>>>
>>>>> Regarding to shrinker_deferred you should prove either this is not a problem too,
>>>>> or to add proper synchronization (maybe, based on barriers) or to add some similar check
>>>>> (maybe, in shrink_slab_memcg() too).
>>>>
>>>> It seems shrink_slab_memcg() might see shrinker_deferred as NULL
>>>> either due to the same reason. I don't think there is a guarantee it
>>>> won't happen.
>>>>
>>>> We just need guarantee CSS_ONLINE is seen after shrinker_maps and
>>>> shrinker_deferred are allocated, so I'm supposed barriers before
>>>> "css->flags |= CSS_ONLINE" should work.
>>>>
>>>> So the below patch may be ok:
>>>>
>>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>>> index df128cab900f..9f7fb0450d69 100644
>>>> --- a/mm/memcontrol.c
>>>> +++ b/mm/memcontrol.c
>>>> @@ -5539,6 +5539,12 @@ static int mem_cgroup_css_online(struct
>>>> cgroup_subsys_state *css)
>>>>                 return -ENOMEM;
>>>>         }
>>>>
>>>>
>>>> +       /*
>>>> +        * Barrier for CSS_ONLINE, so that shrink_slab_memcg() sees
>>>> shirnker_maps
>>>> +        * and shrinker_deferred before CSS_ONLINE.
>>>> +        */
>>>> +       smp_mb();
>>>> +
>>>>         /* Online state pins memcg ID, memcg ID pins CSS */
>>>>         refcount_set(&memcg->id.ref, 1);
>>>>         css_get(css);
>>>
>>> smp barriers synchronize data access from different cpus. They should go in a pair.
>>> In case of you add the smp barrier into mem_cgroup_css_online(), we should also
>>> add one more smp barrier in another place, which we want to synchonize with this.
>>> Also, every place should contain a comment referring to its pair: "Pairs with...".
>>
>> Thanks, I think you are correct. Looked into it further, it seems the
>> race pattern looks like:
>>
>> CPU A                                                                  CPU B
>> store shrinker_maps pointer                      load CSS_ONLINE
>> store CSS_ONLINE                                   load shrinker_maps pointer
>>
>> By checking the memory-barriers document, it seems we need write
>> barrier/read barrier pair as below:
>>
>> CPU A                                                                  CPU B
>> store shrinker_maps pointer                       load CSS_ONLINE
>> <write barrier>                                             <read barrier>
>> store CSS_ONLINE                                    load shrinker_maps pointer
>>
>>
>> So, the patch should look like:
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index df128cab900f..489c0a84f82b 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -5539,6 +5539,13 @@ static int mem_cgroup_css_online(struct
>> cgroup_subsys_state *css)
>>                 return -ENOMEM;
>>         }
>>
>> +       /*
>> +        * Barrier for CSS_ONLINE, so that shrink_slab_memcg() sees
>> shirnker_maps
>> +        * and shrinker_deferred before CSS_ONLINE. It pairs with the
>> read barrier
>> +        * in shrink_slab_memcg().
>> +        */
>> +       smp_wmb();
> 
> Is there a reason why the shrinker allocations aren't done in
> .css_alloc()? That would take care of all necessary ordering:

The reason is that allocations have to be made in a place, where
mem-cgroup_iter() can't miss it, since memcg_expand_shrinker_maps()
shouldn't miss allocated shrinker maps.

> 
>       #0
>       css = ->css_alloc()
>       list_add_tail_rcu(css, parent->children)
>         rcu_assign_pointer()
>       ->css_online(css)
>       css->flags |= CSS_ONLINE
> 
>       #1
>       memcg = mem_cgroup_iter()
>         list_entry_rcu()
> 	  rcu_dereference()
>       shrink_slab(.., memcg)
> 
> RCU ensures that once the cgroup shows up in the reclaim cgroup it's
> also fully allocated.
> 
>>         /* Online state pins memcg ID, memcg ID pins CSS */
>>         refcount_set(&memcg->id.ref, 1);
>>         css_get(css);
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 9d2a6485e982..fc9bda576d98 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -603,13 +603,15 @@ static unsigned long shrink_slab_memcg(gfp_t
>> gfp_mask, int nid,
>>         if (!mem_cgroup_online(memcg))
>>                 return 0;
> 
> ...then we should be able to delete this online check here entirely:
> 
> A not-yet online cgroup is guaranteed to have a shrinker map, just
> with no bits set. The shrinker code handles that just fine.
> 
> An offlined cgroup will eventually have an empty bitmap as the called
> shrinkers return SHRINK_EMPTY. This could also be shortcut by clearing
> the bit in memcg_drain_list_lru_node() the same way we set it in the
> parent when we move all objects upward, but seems correct as-is.
>