| Message ID | 1575486978-45249-1-git-send-email-yang.shi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: vmscan: protect shrinker idr replace with CONFIG_MEMCG | expand |
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") 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 >
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? 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 > >
On Thu 05-12-19 03:16:18, 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> Acked-by: Michal Hocko <mhocko@suse.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 > -- > 1.8.3.1
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. > 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 >>> >
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. >> 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 >>>> >> >
On Thu 05-12-19 13:00:31, 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. OK, I see. The changelog should describe all that. Thanks for the clarification. > > 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 > >>> > > >
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 >>>>>
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
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> --- mm/vmscan.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)