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 <[email protected]>
Cc: Johannes Weiner <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Shakeel Butt <[email protected]>
Cc: Roman Gushchin <[email protected]>
Signed-off-by: Yang Shi <[email protected]>
---
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 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 <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Shakeel Butt <[email protected]>
> Cc: Roman Gushchin <[email protected]>
> Signed-off-by: Yang Shi <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]>
> > Cc: Johannes Weiner <[email protected]>
> > Cc: Michal Hocko <[email protected]>
> > Cc: Shakeel Butt <[email protected]>
> > Cc: Roman Gushchin <[email protected]>
> > Signed-off-by: Yang Shi <[email protected]>
>
> 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 <[email protected]>
>
> > ---
> > 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
> >
--
Michal Hocko
SUSE Labs
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 <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Shakeel Butt <[email protected]>
> Cc: Roman Gushchin <[email protected]>
> Signed-off-by: Yang Shi <[email protected]>
Acked-by: Michal Hocko <[email protected]>
> ---
> 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
--
Michal Hocko
SUSE Labs
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 <[email protected]>
>>> Cc: Johannes Weiner <[email protected]>
>>> Cc: Michal Hocko <[email protected]>
>>> Cc: Shakeel Butt <[email protected]>
>>> Cc: Roman Gushchin <[email protected]>
>>> Signed-off-by: Yang Shi <[email protected]>
>>
>> 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 <[email protected]>
>>
>>> ---
>>> 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 <[email protected]>
>>>> Cc: Johannes Weiner <[email protected]>
>>>> Cc: Michal Hocko <[email protected]>
>>>> Cc: Shakeel Butt <[email protected]>
>>>> Cc: Roman Gushchin <[email protected]>
>>>> Signed-off-by: Yang Shi <[email protected]>
>>>
>>> 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 <[email protected]>
>>>
>>>> ---
>>>> 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 <[email protected]>
> >>> Cc: Johannes Weiner <[email protected]>
> >>> Cc: Michal Hocko <[email protected]>
> >>> Cc: Shakeel Butt <[email protected]>
> >>> Cc: Roman Gushchin <[email protected]>
> >>> Signed-off-by: Yang Shi <[email protected]>
> >>
> >> 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 <[email protected]>
> >>
> >>> ---
> >>> 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
> >>>
> >
>
--
Michal Hocko
SUSE Labs
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 <[email protected]>
>>>>> Cc: Johannes Weiner <[email protected]>
>>>>> Cc: Michal Hocko <[email protected]>
>>>>> Cc: Shakeel Butt <[email protected]>
>>>>> Cc: Roman Gushchin <[email protected]>
>>>>> Signed-off-by: Yang Shi <[email protected]>
>>>> 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 <[email protected]>
>>>>
>>>>> ---
>>>>> 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
>>>>>