[PATCH] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[PATCH] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Zefan Li]

While trying to use remote memcg charging in an out-of-tree kernel module
I found it's not working, because the current thread is a workqueue thread.

Signed-off-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---

No need to queue this for v5.7 as currently no upstream users of this memcg
feature suffer from this bug.

---
 mm/memcontrol.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index a3b97f1..db836fc 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2802,6 +2802,8 @@ static void memcg_schedule_kmem_cache_create(struct mem_cgroup *memcg,
 
 static inline bool memcg_kmem_bypass(void)
 {
+	if (unlikely(current->active_memcg))
+		return false;
 	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
 		return true;
 	return false;
-- 
2.7.4

Re: [PATCH] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Michal Hocko]

On Wed 13-05-20 15:28:28, Li Zefan wrote:
> While trying to use remote memcg charging in an out-of-tree kernel module
> I found it's not working, because the current thread is a workqueue thread.
> 
> Signed-off-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
> 
> No need to queue this for v5.7 as currently no upstream users of this memcg
> feature suffer from this bug.
> 
> ---
>  mm/memcontrol.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index a3b97f1..db836fc 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2802,6 +2802,8 @@ static void memcg_schedule_kmem_cache_create(struct mem_cgroup *memcg,
>  
>  static inline bool memcg_kmem_bypass(void)
>  {
> +	if (unlikely(current->active_memcg))
> +		return false;

I am confused. Why the check below is insufficient? It checks for both mm
and PF_KTHREAD?

>  	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
>  		return true;
>  	return false;
> -- 
> 2.7.4

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Zefan Li]

On 2020/5/13 17:05, Michal Hocko wrote:
> On Wed 13-05-20 15:28:28, Li Zefan wrote:
>> While trying to use remote memcg charging in an out-of-tree kernel module
>> I found it's not working, because the current thread is a workqueue thread.
>>
>> Signed-off-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>
>> No need to queue this for v5.7 as currently no upstream users of this memcg
>> feature suffer from this bug.
>>
>> ---
>>  mm/memcontrol.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index a3b97f1..db836fc 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -2802,6 +2802,8 @@ static void memcg_schedule_kmem_cache_create(struct mem_cgroup *memcg,
>>  
>>  static inline bool memcg_kmem_bypass(void)
>>  {
>> +	if (unlikely(current->active_memcg))
>> +		return false;
> 
> I am confused. Why the check below is insufficient? It checks for both mm
> and PF_KTHREAD?
> 

memalloc_use_memcg(memcg);
alloc_page(GFP_KERNEL_ACCOUNT);
memalloc_unuse_memcg();

If we run above code in a workqueue thread the memory won't be charged to the specific
memcg, because memcg_kmem_bypass() returns true in this case.

>>  	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
>>  		return true;
>>  	return false;

Re: [PATCH] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Michal Hocko]

[Cc Roman - initial email is http://lkml.kernel.org/r/e6927a82-949c-bdfd-d717-0a14743c6759-hv44wF8Li93QT0dZR+AlfA@public.gmane.org]

On Wed 13-05-20 19:19:56, Li Zefan wrote:
> On 2020/5/13 17:05, Michal Hocko wrote:
> > On Wed 13-05-20 15:28:28, Li Zefan wrote:
> >> While trying to use remote memcg charging in an out-of-tree kernel module
> >> I found it's not working, because the current thread is a workqueue thread.
> >>
> >> Signed-off-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> >> ---
> >>
> >> No need to queue this for v5.7 as currently no upstream users of this memcg
> >> feature suffer from this bug.
> >>
> >> ---
> >>  mm/memcontrol.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> >> index a3b97f1..db836fc 100644
> >> --- a/mm/memcontrol.c
> >> +++ b/mm/memcontrol.c
> >> @@ -2802,6 +2802,8 @@ static void memcg_schedule_kmem_cache_create(struct mem_cgroup *memcg,
> >>  
> >>  static inline bool memcg_kmem_bypass(void)
> >>  {
> >> +	if (unlikely(current->active_memcg))
> >> +		return false;
> > 
> > I am confused. Why the check below is insufficient? It checks for both mm
> > and PF_KTHREAD?
> > 
> 
> memalloc_use_memcg(memcg);
> alloc_page(GFP_KERNEL_ACCOUNT);
> memalloc_unuse_memcg();
> 
> If we run above code in a workqueue thread the memory won't be charged to the specific
> memcg, because memcg_kmem_bypass() returns true in this case.

Ohh, right I have misread your patch. Sorry about that. A comment for
the above branch would make it more clear. Something like
	/* Allow memalloc_use_memcg usage from kthread contexts */

On the other hand adding a code for an out of tree code is usually not
welcome. But in this particular case the branch is correct for the
existing code already so I am OK with it. Roman is de-facto kmem
implementation maintainer so I will defer to him.

> >>  	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
> >>  		return true;
> >>  	return false;

-- 
Michal Hocko
SUSE Labs

[PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Zefan Li]

While trying to use remote memcg charging in an out-of-tree kernel module
I found it's not working, because the current thread is a workqueue thread.

As we will probably encounter this issue in the future as the users of
memalloc_use_memcg() grow, it's better we fix it now.

Signed-off-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---

v2: add a comment as sugguested by Michal. and add changelog to explain why
upstream kernel needs this fix.

---

 mm/memcontrol.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index a3b97f1..43a12ed 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2802,6 +2802,9 @@ static void memcg_schedule_kmem_cache_create(struct mem_cgroup *memcg,
 
 static inline bool memcg_kmem_bypass(void)
 {
+	/* Allow remote memcg charging in kthread contexts. */
+	if (unlikely(current->active_memcg))
+		return false;
 	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
 		return true;
 	return false;
-- 
2.7.4

Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Shakeel Butt]

On Wed, May 13, 2020 at 4:47 AM Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:
>
> While trying to use remote memcg charging in an out-of-tree kernel module
> I found it's not working, because the current thread is a workqueue thread.
>
> As we will probably encounter this issue in the future as the users of
> memalloc_use_memcg() grow, it's better we fix it now.
>
> Signed-off-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>
> v2: add a comment as sugguested by Michal. and add changelog to explain why
> upstream kernel needs this fix.
>
> ---
>
>  mm/memcontrol.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index a3b97f1..43a12ed 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2802,6 +2802,9 @@ static void memcg_schedule_kmem_cache_create(struct mem_cgroup *memcg,
>
>  static inline bool memcg_kmem_bypass(void)
>  {
> +       /* Allow remote memcg charging in kthread contexts. */
> +       if (unlikely(current->active_memcg))
> +               return false;

What about __GFP_ACCOUNT allocations in the interrupt context?

e.g.

memalloc_use_memcg(memcg);
--->interrupt
--->alloc_page(GFP_KERNEL_ACCOUNT) in interrupt context.
alloc_page(GFP_KERNEL_ACCOUNT);
memalloc_unuse_memcg();


>         if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
>                 return true;
>         return false;
> --
> 2.7.4
>

Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Johannes Weiner]

On Wed, May 13, 2020 at 07:47:49PM +0800, Zefan Li wrote:
> While trying to use remote memcg charging in an out-of-tree kernel module
> I found it's not working, because the current thread is a workqueue thread.
> 
> As we will probably encounter this issue in the future as the users of
> memalloc_use_memcg() grow, it's better we fix it now.
> 
> Signed-off-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Acked-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>

Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Roman Gushchin]

On Wed, May 13, 2020 at 07:47:49PM +0800, Zefan Li wrote:
> While trying to use remote memcg charging in an out-of-tree kernel module
> I found it's not working, because the current thread is a workqueue thread.
> 
> As we will probably encounter this issue in the future as the users of
> memalloc_use_memcg() grow, it's better we fix it now.
> 
> Signed-off-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
> 
> v2: add a comment as sugguested by Michal. and add changelog to explain why
> upstream kernel needs this fix.
> 
> ---
> 
>  mm/memcontrol.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index a3b97f1..43a12ed 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2802,6 +2802,9 @@ static void memcg_schedule_kmem_cache_create(struct mem_cgroup *memcg,
>  
>  static inline bool memcg_kmem_bypass(void)
>  {
> +	/* Allow remote memcg charging in kthread contexts. */
> +	if (unlikely(current->active_memcg))
> +		return false;
>  	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
>  		return true;

Shakeel is right about interrupts. How about something like this?

static inline bool memcg_kmem_bypass(void)
{
	if (in_interrupt())
		return true;

	if ((!current->mm || current->flags & PF_KTHREAD) && !current->active_memcg)
		return true;

	return false;
}

Thanks!

Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Zefan Li]

On 2020/5/14 0:11, Roman Gushchin wrote:
> On Wed, May 13, 2020 at 07:47:49PM +0800, Zefan Li wrote:
>> While trying to use remote memcg charging in an out-of-tree kernel module
>> I found it's not working, because the current thread is a workqueue thread.
>>
>> As we will probably encounter this issue in the future as the users of
>> memalloc_use_memcg() grow, it's better we fix it now.
>>
>> Signed-off-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>
>> v2: add a comment as sugguested by Michal. and add changelog to explain why
>> upstream kernel needs this fix.
>>
>> ---
>>
>>  mm/memcontrol.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index a3b97f1..43a12ed 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -2802,6 +2802,9 @@ static void memcg_schedule_kmem_cache_create(struct mem_cgroup *memcg,
>>  
>>  static inline bool memcg_kmem_bypass(void)
>>  {
>> +	/* Allow remote memcg charging in kthread contexts. */
>> +	if (unlikely(current->active_memcg))
>> +		return false;
>>  	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
>>  		return true;
> 
> Shakeel is right about interrupts. How about something like this?
> 
> static inline bool memcg_kmem_bypass(void)
> {
> 	if (in_interrupt())
> 		return true;
> 
> 	if ((!current->mm || current->flags & PF_KTHREAD) && !current->active_memcg)
> 		return true;
> 
> 	return false;
> }
> 

I thought the user should ensure not do this, but now I think it makes sense to just bypass
the interrupt case.

Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Roman Gushchin]

On Thu, May 14, 2020 at 09:16:29AM +0800, Zefan Li wrote:
> On 2020/5/14 0:11, Roman Gushchin wrote:
> > On Wed, May 13, 2020 at 07:47:49PM +0800, Zefan Li wrote:
> >> While trying to use remote memcg charging in an out-of-tree kernel module
> >> I found it's not working, because the current thread is a workqueue thread.
> >>
> >> As we will probably encounter this issue in the future as the users of
> >> memalloc_use_memcg() grow, it's better we fix it now.
> >>
> >> Signed-off-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> >> ---
> >>
> >> v2: add a comment as sugguested by Michal. and add changelog to explain why
> >> upstream kernel needs this fix.
> >>
> >> ---
> >>
> >>  mm/memcontrol.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> >> index a3b97f1..43a12ed 100644
> >> --- a/mm/memcontrol.c
> >> +++ b/mm/memcontrol.c
> >> @@ -2802,6 +2802,9 @@ static void memcg_schedule_kmem_cache_create(struct mem_cgroup *memcg,
> >>  
> >>  static inline bool memcg_kmem_bypass(void)
> >>  {
> >> +	/* Allow remote memcg charging in kthread contexts. */
> >> +	if (unlikely(current->active_memcg))
> >> +		return false;
> >>  	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
> >>  		return true;
> > 
> > Shakeel is right about interrupts. How about something like this?
> > 
> > static inline bool memcg_kmem_bypass(void)
> > {
> > 	if (in_interrupt())
> > 		return true;
> > 
> > 	if ((!current->mm || current->flags & PF_KTHREAD) && !current->active_memcg)
> > 		return true;
> > 
> > 	return false;
> > }
> > 
> 
> I thought the user should ensure not do this, but now I think it makes sense to just bypass
> the interrupt case.

I think now it's mostly a legacy of the opt-out kernel memory accounting.
Actually we can relax this requirement by forcibly overcommit the memory cgroup
if the allocation is happening from the irq context, and punish it afterwards.
Idk how much we wanna this, hopefully nobody is allocating large non-temporarily
objects from an irq.

Will you send a v3?

Thanks!

Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Michal Hocko]

On Thu 14-05-20 15:52:59, Roman Gushchin wrote:
> On Thu, May 14, 2020 at 09:16:29AM +0800, Zefan Li wrote:
> > On 2020/5/14 0:11, Roman Gushchin wrote:
> > > On Wed, May 13, 2020 at 07:47:49PM +0800, Zefan Li wrote:
> > >> While trying to use remote memcg charging in an out-of-tree kernel module
> > >> I found it's not working, because the current thread is a workqueue thread.
> > >>
> > >> As we will probably encounter this issue in the future as the users of
> > >> memalloc_use_memcg() grow, it's better we fix it now.
> > >>
> > >> Signed-off-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> > >> ---
> > >>
> > >> v2: add a comment as sugguested by Michal. and add changelog to explain why
> > >> upstream kernel needs this fix.
> > >>
> > >> ---
> > >>
> > >>  mm/memcontrol.c | 3 +++
> > >>  1 file changed, 3 insertions(+)
> > >>
> > >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > >> index a3b97f1..43a12ed 100644
> > >> --- a/mm/memcontrol.c
> > >> +++ b/mm/memcontrol.c
> > >> @@ -2802,6 +2802,9 @@ static void memcg_schedule_kmem_cache_create(struct mem_cgroup *memcg,
> > >>  
> > >>  static inline bool memcg_kmem_bypass(void)
> > >>  {
> > >> +	/* Allow remote memcg charging in kthread contexts. */
> > >> +	if (unlikely(current->active_memcg))
> > >> +		return false;
> > >>  	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
> > >>  		return true;
> > > 
> > > Shakeel is right about interrupts. How about something like this?
> > > 
> > > static inline bool memcg_kmem_bypass(void)
> > > {
> > > 	if (in_interrupt())
> > > 		return true;
> > > 
> > > 	if ((!current->mm || current->flags & PF_KTHREAD) && !current->active_memcg)
> > > 		return true;
> > > 
> > > 	return false;
> > > }
> > > 
> > 
> > I thought the user should ensure not do this, but now I think it makes sense to just bypass
> > the interrupt case.
> 
> I think now it's mostly a legacy of the opt-out kernel memory accounting.
> Actually we can relax this requirement by forcibly overcommit the memory cgroup
> if the allocation is happening from the irq context, and punish it afterwards.
> Idk how much we wanna this, hopefully nobody is allocating large non-temporarily
> objects from an irq.

I do not think we want to pretend that remote charging from the IRQ
context is supported. Why don't we simply WARN_ON(in_interrupt()) there?

> 
> Will you send a v3?
> 
> Thanks!

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Zefan Li]

On 2020/5/15 14:56, Michal Hocko wrote:
> On Thu 14-05-20 15:52:59, Roman Gushchin wrote:
>> On Thu, May 14, 2020 at 09:16:29AM +0800, Zefan Li wrote:
>>> On 2020/5/14 0:11, Roman Gushchin wrote:
>>>> On Wed, May 13, 2020 at 07:47:49PM +0800, Zefan Li wrote:
>>>>> While trying to use remote memcg charging in an out-of-tree kernel module
>>>>> I found it's not working, because the current thread is a workqueue thread.
>>>>>
>>>>> As we will probably encounter this issue in the future as the users of
>>>>> memalloc_use_memcg() grow, it's better we fix it now.
>>>>>
>>>>> Signed-off-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>>>> ---
>>>>>
>>>>> v2: add a comment as sugguested by Michal. and add changelog to explain why
>>>>> upstream kernel needs this fix.
>>>>>
>>>>> ---
>>>>>
>>>>>  mm/memcontrol.c | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>>>> index a3b97f1..43a12ed 100644
>>>>> --- a/mm/memcontrol.c
>>>>> +++ b/mm/memcontrol.c
>>>>> @@ -2802,6 +2802,9 @@ static void memcg_schedule_kmem_cache_create(struct mem_cgroup *memcg,
>>>>>  
>>>>>  static inline bool memcg_kmem_bypass(void)
>>>>>  {
>>>>> +	/* Allow remote memcg charging in kthread contexts. */
>>>>> +	if (unlikely(current->active_memcg))
>>>>> +		return false;
>>>>>  	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
>>>>>  		return true;
>>>>
>>>> Shakeel is right about interrupts. How about something like this?
>>>>
>>>> static inline bool memcg_kmem_bypass(void)
>>>> {
>>>> 	if (in_interrupt())
>>>> 		return true;
>>>>
>>>> 	if ((!current->mm || current->flags & PF_KTHREAD) && !current->active_memcg)
>>>> 		return true;
>>>>
>>>> 	return false;
>>>> }
>>>>
>>>
>>> I thought the user should ensure not do this, but now I think it makes sense to just bypass
>>> the interrupt case.
>>
>> I think now it's mostly a legacy of the opt-out kernel memory accounting.
>> Actually we can relax this requirement by forcibly overcommit the memory cgroup
>> if the allocation is happening from the irq context, and punish it afterwards.
>> Idk how much we wanna this, hopefully nobody is allocating large non-temporarily
>> objects from an irq.
> 
> I do not think we want to pretend that remote charging from the IRQ
> context is supported. Why don't we simply WARN_ON(in_interrupt()) there?
> 

How about:

static inline bool memcg_kmem_bypass(void)
{
        if (in_interrupt()) {
                WARN_ON(current->active_memcg);
                return true;
        }

        /* Allow remote memcg charging in kthread contexts. */
        if ((!current->mm || (current->flags & PF_KTHREAD)) && !current->active_memcg)
                return true;
        return false;
}

Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Michal Hocko]

On Fri 15-05-20 16:20:04, Li Zefan wrote:
> On 2020/5/15 14:56, Michal Hocko wrote:
> > On Thu 14-05-20 15:52:59, Roman Gushchin wrote:
[...]
> >>> I thought the user should ensure not do this, but now I think it makes sense to just bypass
> >>> the interrupt case.
> >>
> >> I think now it's mostly a legacy of the opt-out kernel memory accounting.
> >> Actually we can relax this requirement by forcibly overcommit the memory cgroup
> >> if the allocation is happening from the irq context, and punish it afterwards.
> >> Idk how much we wanna this, hopefully nobody is allocating large non-temporarily
> >> objects from an irq.
> > 
> > I do not think we want to pretend that remote charging from the IRQ
> > context is supported. Why don't we simply WARN_ON(in_interrupt()) there?
> > 
> 
> How about:
> 
> static inline bool memcg_kmem_bypass(void)
> {
>         if (in_interrupt()) {
>                 WARN_ON(current->active_memcg);
>                 return true;
>         }

Why not simply 

	if (WARN_ON_ONCE(in_interrupt())
		return true;

the idea is that we want to catch any __GFP_ACCOUNT user from IRQ
context because this just doesn't work and we do not plan to support it
for now and foreseeable future. If this is reduced only to active_memcg
then we are only getting a partial picture.
		
> 
>         /* Allow remote memcg charging in kthread contexts. */
>         if ((!current->mm || (current->flags & PF_KTHREAD)) && !current->active_memcg)
>                 return true;
>         return false;
> }

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Shakeel Butt]

On Fri, May 15, 2020 at 1:35 AM Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> On Fri 15-05-20 16:20:04, Li Zefan wrote:
> > On 2020/5/15 14:56, Michal Hocko wrote:
> > > On Thu 14-05-20 15:52:59, Roman Gushchin wrote:
> [...]
> > >>> I thought the user should ensure not do this, but now I think it makes sense to just bypass
> > >>> the interrupt case.
> > >>
> > >> I think now it's mostly a legacy of the opt-out kernel memory accounting.
> > >> Actually we can relax this requirement by forcibly overcommit the memory cgroup
> > >> if the allocation is happening from the irq context, and punish it afterwards.
> > >> Idk how much we wanna this, hopefully nobody is allocating large non-temporarily
> > >> objects from an irq.
> > >
> > > I do not think we want to pretend that remote charging from the IRQ
> > > context is supported. Why don't we simply WARN_ON(in_interrupt()) there?
> > >
> >
> > How about:
> >
> > static inline bool memcg_kmem_bypass(void)
> > {
> >         if (in_interrupt()) {
> >                 WARN_ON(current->active_memcg);
> >                 return true;
> >         }
>
> Why not simply
>
>         if (WARN_ON_ONCE(in_interrupt())
>                 return true;
>
> the idea is that we want to catch any __GFP_ACCOUNT user from IRQ
> context because this just doesn't work and we do not plan to support it
> for now and foreseeable future. If this is reduced only to active_memcg
> then we are only getting a partial picture.
>

There are SLAB_ACCOUNT kmem caches which do allocations in IRQ context
(see sk_prot_alloc()), so, either we make charging work in IRQ or no
warnings at all.

Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Roman Gushchin]

On Fri, May 15, 2020 at 09:22:25AM -0700, Shakeel Butt wrote:
> On Fri, May 15, 2020 at 1:35 AM Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > On Fri 15-05-20 16:20:04, Li Zefan wrote:
> > > On 2020/5/15 14:56, Michal Hocko wrote:
> > > > On Thu 14-05-20 15:52:59, Roman Gushchin wrote:
> > [...]
> > > >>> I thought the user should ensure not do this, but now I think it makes sense to just bypass
> > > >>> the interrupt case.
> > > >>
> > > >> I think now it's mostly a legacy of the opt-out kernel memory accounting.
> > > >> Actually we can relax this requirement by forcibly overcommit the memory cgroup
> > > >> if the allocation is happening from the irq context, and punish it afterwards.
> > > >> Idk how much we wanna this, hopefully nobody is allocating large non-temporarily
> > > >> objects from an irq.
> > > >
> > > > I do not think we want to pretend that remote charging from the IRQ
> > > > context is supported. Why don't we simply WARN_ON(in_interrupt()) there?
> > > >
> > >
> > > How about:
> > >
> > > static inline bool memcg_kmem_bypass(void)
> > > {
> > >         if (in_interrupt()) {
> > >                 WARN_ON(current->active_memcg);
> > >                 return true;
> > >         }
> >
> > Why not simply
> >
> >         if (WARN_ON_ONCE(in_interrupt())
> >                 return true;
> >
> > the idea is that we want to catch any __GFP_ACCOUNT user from IRQ
> > context because this just doesn't work and we do not plan to support it
> > for now and foreseeable future.

Actually, why not?
It should be fairly simple, especially after the rework of the slab controller.

> If this is reduced only to active_memcg
> > then we are only getting a partial picture.
> >
> 
> There are SLAB_ACCOUNT kmem caches which do allocations in IRQ context
> (see sk_prot_alloc()), so, either we make charging work in IRQ or no
> warnings at all.

I agree. Actually, there is nothing wrong to warn about, it's just a limitation
of the current accounting implementation.

Thanks!

Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Michal Hocko]

On Fri 15-05-20 09:22:25, Shakeel Butt wrote:
> On Fri, May 15, 2020 at 1:35 AM Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > On Fri 15-05-20 16:20:04, Li Zefan wrote:
> > > On 2020/5/15 14:56, Michal Hocko wrote:
> > > > On Thu 14-05-20 15:52:59, Roman Gushchin wrote:
> > [...]
> > > >>> I thought the user should ensure not do this, but now I think it makes sense to just bypass
> > > >>> the interrupt case.
> > > >>
> > > >> I think now it's mostly a legacy of the opt-out kernel memory accounting.
> > > >> Actually we can relax this requirement by forcibly overcommit the memory cgroup
> > > >> if the allocation is happening from the irq context, and punish it afterwards.
> > > >> Idk how much we wanna this, hopefully nobody is allocating large non-temporarily
> > > >> objects from an irq.
> > > >
> > > > I do not think we want to pretend that remote charging from the IRQ
> > > > context is supported. Why don't we simply WARN_ON(in_interrupt()) there?
> > > >
> > >
> > > How about:
> > >
> > > static inline bool memcg_kmem_bypass(void)
> > > {
> > >         if (in_interrupt()) {
> > >                 WARN_ON(current->active_memcg);
> > >                 return true;
> > >         }
> >
> > Why not simply
> >
> >         if (WARN_ON_ONCE(in_interrupt())
> >                 return true;
> >
> > the idea is that we want to catch any __GFP_ACCOUNT user from IRQ
> > context because this just doesn't work and we do not plan to support it
> > for now and foreseeable future. If this is reduced only to active_memcg
> > then we are only getting a partial picture.
> >
> 
> There are SLAB_ACCOUNT kmem caches which do allocations in IRQ context
> (see sk_prot_alloc()), so, either we make charging work in IRQ or no
> warnings at all.

OK, I see. I wasn't aware that those caches are used from IRQ context.

-- 
Michal Hocko
SUSE Labs

[PATCH v3] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Zefan Li]

While trying to use remote memcg charging in an out-of-tree kernel module
I found it's not working, because the current thread is a workqueue thread.

As we will probably encounter this issue in the future as the users of
memalloc_use_memcg() grow, and it's nothing wrong for this usage, it's
better we fix it now.

Acked-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
Signed-off-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---

v3: bypass __GFP_ACCOUNT allocations in interrupt contexts.

---
 mm/memcontrol.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index a3b97f1..3c7717a 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2802,7 +2802,12 @@ static void memcg_schedule_kmem_cache_create(struct mem_cgroup *memcg,
 
 static inline bool memcg_kmem_bypass(void)
 {
-	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
+	if (in_interrupt())
+		return true;
+
+	/* Allow remote memcg charging in kthread contexts. */
+	if ((!current->mm || (current->flags & PF_KTHREAD)) &&
+	     !current->active_memcg)
 		return true;
 	return false;
 }
-- 
2.7.4

Re: [PATCH v3] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Roman Gushchin]

On Tue, May 26, 2020 at 09:25:25AM +0800, Zefan Li wrote:
> While trying to use remote memcg charging in an out-of-tree kernel module
> I found it's not working, because the current thread is a workqueue thread.
> 
> As we will probably encounter this issue in the future as the users of
> memalloc_use_memcg() grow, and it's nothing wrong for this usage, it's
> better we fix it now.
> 
> Acked-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
> Signed-off-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---

Reviewed-by: Roman Gushchin <guro-b10kYP2dOMg@public.gmane.org>

Thanks!

> 
> v3: bypass __GFP_ACCOUNT allocations in interrupt contexts.
> 
> ---
>  mm/memcontrol.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index a3b97f1..3c7717a 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2802,7 +2802,12 @@ static void memcg_schedule_kmem_cache_create(struct mem_cgroup *memcg,
>  
>  static inline bool memcg_kmem_bypass(void)
>  {
> -	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
> +	if (in_interrupt())
> +		return true;
> +
> +	/* Allow remote memcg charging in kthread contexts. */
> +	if ((!current->mm || (current->flags & PF_KTHREAD)) &&
> +	     !current->active_memcg)
>  		return true;
>  	return false;
>  }
> -- 
> 2.7.4
> 

Re: [PATCH v3] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Shakeel Butt]

On Mon, May 25, 2020 at 6:25 PM Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:
>
> While trying to use remote memcg charging in an out-of-tree kernel module
> I found it's not working, because the current thread is a workqueue thread.
>
> As we will probably encounter this issue in the future as the users of
> memalloc_use_memcg() grow, and it's nothing wrong for this usage, it's
> better we fix it now.
>
> Acked-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
> Signed-off-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Reviewed-by: Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Re: [PATCH v3] memcg: Fix memcg_kmem_bypass() for remote memcg charging

[Michal Hocko]

On Tue 26-05-20 09:25:25, Li Zefan wrote:
> While trying to use remote memcg charging in an out-of-tree kernel module
> I found it's not working, because the current thread is a workqueue thread.
> 
> As we will probably encounter this issue in the future as the users of
> memalloc_use_memcg() grow, and it's nothing wrong for this usage, it's
> better we fix it now.
> 
> Acked-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
> Signed-off-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Acked-by: Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org>

> ---
> 
> v3: bypass __GFP_ACCOUNT allocations in interrupt contexts.
> 
> ---
>  mm/memcontrol.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index a3b97f1..3c7717a 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2802,7 +2802,12 @@ static void memcg_schedule_kmem_cache_create(struct mem_cgroup *memcg,
>  
>  static inline bool memcg_kmem_bypass(void)
>  {
> -	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
> +	if (in_interrupt())
> +		return true;
> +
> +	/* Allow remote memcg charging in kthread contexts. */
> +	if ((!current->mm || (current->flags & PF_KTHREAD)) &&
> +	     !current->active_memcg)
>  		return true;
>  	return false;
>  }
> -- 
> 2.7.4

-- 
Michal Hocko
SUSE Labs