Messages in this thread | | | Subject | Re: [PATCH] mm: vmscan: protect shrinker idr replace with CONFIG_MEMCG | From | Yang Shi <> | Date | Thu, 5 Dec 2019 09:05:09 -0800 |
| |
On 12/5/19 2:11 AM, Kirill Tkhai wrote: > On 05.12.2019 13:00, Kirill Tkhai wrote: >> On 05.12.2019 12:43, Michal Hocko wrote: >>> On Thu 05-12-19 11:23:28, Kirill Tkhai wrote: >>>> On 04.12.2019 22:16, Yang Shi wrote: >>>>> Since commit 0a432dcbeb32edcd211a5d8f7847d0da7642a8b4 ("mm: shrinker: >>>>> make shrinker not depend on memcg kmem"), shrinkers' idr is protected by >>>>> CONFIG_MEMCG instead of CONFIG_MEMCG_KMEM, so it makes no sense to >>>>> protect shrinker idr replace with CONFIG_MEMCG_KMEM. >>>>> >>>>> Cc: Kirill Tkhai <ktkhai@virtuozzo.com> >>>>> Cc: Johannes Weiner <hannes@cmpxchg.org> >>>>> Cc: Michal Hocko <mhocko@suse.com> >>>>> Cc: Shakeel Butt <shakeelb@google.com> >>>>> Cc: Roman Gushchin <guro@fb.com> >>>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> >>>> It looks like that in CONFIG_SLOB case we do not even call some shrinkers >>>> for subordinate mem cgroups (i.e., we don't call deferred_split_shrinker), >>>> since they never become completely registered. >>>> >>>> Fixes: 0a432dcbeb32edcd211a5d8f7847d0da7642a8b4 ("mm: shrinker: make shrinker not depend on memcg kmem") >>> I am confused. Why the Fixes tag? Nothing should be really broken with >>> KMEM config guard right? >> idr_replace() is disabled in CONFIG_MEMCG && CONFIG_SLOB case, and this is >> wrong. >> >> 0a432dcbeb32edcd211a5d8f7847d0da7642a8b4 goes in the series, which enables >> shrinker_idr infrastructure for huge_memory.c's deferred_split_shrinker >> in CONFIG_MEMCG case. Previously, all SHRINKER_MEMCG_AWARE shrinkers were >> based on LRUs, and they remain to base of CONFIG_MEMCG_KMEM. >> But deferred_split_shrinker is an exception. >> >> In CONFIG_MEMCG && CONFIG_SLOB case, shrinker_idr contains only shrinker, >> and it is deferred_split_shrinker. But it is never actually called, since >> idr_replace() is never compiled. deferred_split_shrinker all the time is >> staying in half-registered state, and it's never called for subordinate >> mem cgroups. >> >> So, this is a BUG, and this should go to stable. > The visible effect is that deferred_split_shrinker is never called > from shrink_slab() for subordinate mem cgroups. It's called only > for root_mem_cgroup.
Thanks for noticing the SLOB case, I didn't realize it and thought it was just a cleanup too.
Will update the information and cc to stable list for v2.
> >>> This is a mere clean up AFAICS. >>> >>>> Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com> >>>> >>>>> --- >>>>> mm/vmscan.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/mm/vmscan.c b/mm/vmscan.c >>>>> index ee4eecc..e7f10c4 100644 >>>>> --- a/mm/vmscan.c >>>>> +++ b/mm/vmscan.c >>>>> @@ -422,7 +422,7 @@ void register_shrinker_prepared(struct shrinker *shrinker) >>>>> { >>>>> down_write(&shrinker_rwsem); >>>>> list_add_tail(&shrinker->list, &shrinker_list); >>>>> -#ifdef CONFIG_MEMCG_KMEM >>>>> +#ifdef CONFIG_MEMCG >>>>> if (shrinker->flags & SHRINKER_MEMCG_AWARE) >>>>> idr_replace(&shrinker_idr, shrinker, shrinker->id); >>>>> #endif >>>>>
| |