linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] memcg: Fix memcg_kmem_bypass() for remote memcg charging
@ 2020-05-13  7:28 Zefan Li
  2020-05-13  9:05 ` Michal Hocko
  0 siblings, 1 reply; 20+ messages in thread
From: Zefan Li @ 2020-05-13  7:28 UTC (permalink / raw)
  To: Johannes Weiner, Michal Hocko, Vladimir Davydov
  Cc: Cgroups, linux-mm, Andrew Morton

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@huawei.com>
---

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



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-13  7:28 [PATCH] memcg: Fix memcg_kmem_bypass() for remote memcg charging Zefan Li
@ 2020-05-13  9:05 ` Michal Hocko
  2020-05-13 11:19   ` Zefan Li
  0 siblings, 1 reply; 20+ messages in thread
From: Michal Hocko @ 2020-05-13  9:05 UTC (permalink / raw)
  To: Zefan Li
  Cc: Johannes Weiner, Vladimir Davydov, Cgroups, linux-mm, Andrew Morton

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@huawei.com>
> ---
> 
> 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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-13  9:05 ` Michal Hocko
@ 2020-05-13 11:19   ` Zefan Li
  2020-05-13 11:29     ` Michal Hocko
  0 siblings, 1 reply; 20+ messages in thread
From: Zefan Li @ 2020-05-13 11:19 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Johannes Weiner, Vladimir Davydov, Cgroups, linux-mm, Andrew Morton

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@huawei.com>
>> ---
>>
>> 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;


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-13 11:19   ` Zefan Li
@ 2020-05-13 11:29     ` Michal Hocko
  2020-05-13 11:47       ` [PATCH v2] " Zefan Li
  2020-05-26  1:25       ` [PATCH v3] " Zefan Li
  0 siblings, 2 replies; 20+ messages in thread
From: Michal Hocko @ 2020-05-13 11:29 UTC (permalink / raw)
  To: Zefan Li
  Cc: Johannes Weiner, Vladimir Davydov, Cgroups, linux-mm, Andrew Morton

[Cc Roman - initial email is http://lkml.kernel.org/r/e6927a82-949c-bdfd-d717-0a14743c6759@huawei.com]

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@huawei.com>
> >> ---
> >>
> >> 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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-13 11:29     ` Michal Hocko
@ 2020-05-13 11:47       ` Zefan Li
  2020-05-13 12:22         ` Shakeel Butt
                           ` (2 more replies)
  2020-05-26  1:25       ` [PATCH v3] " Zefan Li
  1 sibling, 3 replies; 20+ messages in thread
From: Zefan Li @ 2020-05-13 11:47 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Johannes Weiner, Vladimir Davydov, Cgroups, linux-mm,
	Andrew Morton, Roman Gushchin, Shakeel Butt

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@huawei.com>
---

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



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-13 11:47       ` [PATCH v2] " Zefan Li
@ 2020-05-13 12:22         ` Shakeel Butt
  2020-05-13 13:05         ` Johannes Weiner
  2020-05-13 16:11         ` Roman Gushchin
  2 siblings, 0 replies; 20+ messages in thread
From: Shakeel Butt @ 2020-05-13 12:22 UTC (permalink / raw)
  To: Zefan Li
  Cc: Michal Hocko, Johannes Weiner, Vladimir Davydov, Cgroups,
	Linux MM, Andrew Morton, Roman Gushchin

On Wed, May 13, 2020 at 4:47 AM Zefan Li <lizefan@huawei.com> 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@huawei.com>
> ---
>
> 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
>


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-13 11:47       ` [PATCH v2] " Zefan Li
  2020-05-13 12:22         ` Shakeel Butt
@ 2020-05-13 13:05         ` Johannes Weiner
  2020-05-13 16:11         ` Roman Gushchin
  2 siblings, 0 replies; 20+ messages in thread
From: Johannes Weiner @ 2020-05-13 13:05 UTC (permalink / raw)
  To: Zefan Li
  Cc: Michal Hocko, Vladimir Davydov, Cgroups, linux-mm, Andrew Morton,
	Roman Gushchin, Shakeel Butt

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@huawei.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-13 11:47       ` [PATCH v2] " Zefan Li
  2020-05-13 12:22         ` Shakeel Butt
  2020-05-13 13:05         ` Johannes Weiner
@ 2020-05-13 16:11         ` Roman Gushchin
  2020-05-14  1:16           ` Zefan Li
  2 siblings, 1 reply; 20+ messages in thread
From: Roman Gushchin @ 2020-05-13 16:11 UTC (permalink / raw)
  To: Zefan Li
  Cc: Michal Hocko, Johannes Weiner, Vladimir Davydov, Cgroups,
	linux-mm, Andrew Morton, Shakeel Butt

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@huawei.com>
> ---
> 
> 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!


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-13 16:11         ` Roman Gushchin
@ 2020-05-14  1:16           ` Zefan Li
  2020-05-14 22:52             ` Roman Gushchin
  0 siblings, 1 reply; 20+ messages in thread
From: Zefan Li @ 2020-05-14  1:16 UTC (permalink / raw)
  To: Roman Gushchin
  Cc: Michal Hocko, Johannes Weiner, Vladimir Davydov, Cgroups,
	linux-mm, Andrew Morton, Shakeel Butt

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@huawei.com>
>> ---
>>
>> 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.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-14  1:16           ` Zefan Li
@ 2020-05-14 22:52             ` Roman Gushchin
  2020-05-15  6:56               ` Michal Hocko
  0 siblings, 1 reply; 20+ messages in thread
From: Roman Gushchin @ 2020-05-14 22:52 UTC (permalink / raw)
  To: Zefan Li
  Cc: Michal Hocko, Johannes Weiner, Vladimir Davydov, Cgroups,
	linux-mm, Andrew Morton, Shakeel Butt

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@huawei.com>
> >> ---
> >>
> >> 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!


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-14 22:52             ` Roman Gushchin
@ 2020-05-15  6:56               ` Michal Hocko
  2020-05-15  8:20                 ` Zefan Li
  0 siblings, 1 reply; 20+ messages in thread
From: Michal Hocko @ 2020-05-15  6:56 UTC (permalink / raw)
  To: Roman Gushchin
  Cc: Zefan Li, Johannes Weiner, Vladimir Davydov, Cgroups, linux-mm,
	Andrew Morton, Shakeel Butt

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@huawei.com>
> > >> ---
> > >>
> > >> 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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-15  6:56               ` Michal Hocko
@ 2020-05-15  8:20                 ` Zefan Li
  2020-05-15  8:34                   ` Michal Hocko
  0 siblings, 1 reply; 20+ messages in thread
From: Zefan Li @ 2020-05-15  8:20 UTC (permalink / raw)
  To: Michal Hocko, Roman Gushchin
  Cc: Johannes Weiner, Vladimir Davydov, Cgroups, linux-mm,
	Andrew Morton, Shakeel Butt

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@huawei.com>
>>>>> ---
>>>>>
>>>>> 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;
}



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-15  8:20                 ` Zefan Li
@ 2020-05-15  8:34                   ` Michal Hocko
  2020-05-15 16:22                     ` Shakeel Butt
  0 siblings, 1 reply; 20+ messages in thread
From: Michal Hocko @ 2020-05-15  8:34 UTC (permalink / raw)
  To: Zefan Li
  Cc: Roman Gushchin, Johannes Weiner, Vladimir Davydov, Cgroups,
	linux-mm, Andrew Morton, Shakeel Butt

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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-15  8:34                   ` Michal Hocko
@ 2020-05-15 16:22                     ` Shakeel Butt
  2020-05-15 17:31                       ` Roman Gushchin
  2020-05-18  9:13                       ` Michal Hocko
  0 siblings, 2 replies; 20+ messages in thread
From: Shakeel Butt @ 2020-05-15 16:22 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Zefan Li, Roman Gushchin, Johannes Weiner, Vladimir Davydov,
	Cgroups, Linux MM, Andrew Morton

On Fri, May 15, 2020 at 1:35 AM Michal Hocko <mhocko@kernel.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.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-15 16:22                     ` Shakeel Butt
@ 2020-05-15 17:31                       ` Roman Gushchin
  2020-05-18  9:13                       ` Michal Hocko
  1 sibling, 0 replies; 20+ messages in thread
From: Roman Gushchin @ 2020-05-15 17:31 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Michal Hocko, Zefan Li, Johannes Weiner, Vladimir Davydov,
	Cgroups, Linux MM, Andrew Morton

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@kernel.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!


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-15 16:22                     ` Shakeel Butt
  2020-05-15 17:31                       ` Roman Gushchin
@ 2020-05-18  9:13                       ` Michal Hocko
  1 sibling, 0 replies; 20+ messages in thread
From: Michal Hocko @ 2020-05-18  9:13 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Zefan Li, Roman Gushchin, Johannes Weiner, Vladimir Davydov,
	Cgroups, Linux MM, Andrew Morton

On Fri 15-05-20 09:22:25, Shakeel Butt wrote:
> On Fri, May 15, 2020 at 1:35 AM Michal Hocko <mhocko@kernel.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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v3] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-13 11:29     ` Michal Hocko
  2020-05-13 11:47       ` [PATCH v2] " Zefan Li
@ 2020-05-26  1:25       ` Zefan Li
  2020-05-26 15:53         ` Roman Gushchin
                           ` (2 more replies)
  1 sibling, 3 replies; 20+ messages in thread
From: Zefan Li @ 2020-05-26  1:25 UTC (permalink / raw)
  To: Michal Hocko, Andrew Morton
  Cc: Johannes Weiner, Vladimir Davydov, Cgroups, linux-mm,
	Roman Gushchin, Shakeel Butt

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@cmpxchg.org>
Signed-off-by: Zefan Li <lizefan@huawei.com>
---

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



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-26  1:25       ` [PATCH v3] " Zefan Li
@ 2020-05-26 15:53         ` Roman Gushchin
  2020-05-27 16:50         ` Shakeel Butt
  2020-05-28 14:44         ` Michal Hocko
  2 siblings, 0 replies; 20+ messages in thread
From: Roman Gushchin @ 2020-05-26 15:53 UTC (permalink / raw)
  To: Zefan Li
  Cc: Michal Hocko, Andrew Morton, Johannes Weiner, Vladimir Davydov,
	Cgroups, linux-mm, Shakeel Butt

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@cmpxchg.org>
> Signed-off-by: Zefan Li <lizefan@huawei.com>
> ---

Reviewed-by: Roman Gushchin <guro@fb.com>

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
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-26  1:25       ` [PATCH v3] " Zefan Li
  2020-05-26 15:53         ` Roman Gushchin
@ 2020-05-27 16:50         ` Shakeel Butt
  2020-05-28 14:44         ` Michal Hocko
  2 siblings, 0 replies; 20+ messages in thread
From: Shakeel Butt @ 2020-05-27 16:50 UTC (permalink / raw)
  To: Zefan Li
  Cc: Michal Hocko, Andrew Morton, Johannes Weiner, Vladimir Davydov,
	Cgroups, Linux MM, Roman Gushchin

On Mon, May 25, 2020 at 6:25 PM Zefan Li <lizefan@huawei.com> 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@cmpxchg.org>
> Signed-off-by: Zefan Li <lizefan@huawei.com>

Reviewed-by: Shakeel Butt <shakeelb@google.com>


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3] memcg: Fix memcg_kmem_bypass() for remote memcg charging
  2020-05-26  1:25       ` [PATCH v3] " Zefan Li
  2020-05-26 15:53         ` Roman Gushchin
  2020-05-27 16:50         ` Shakeel Butt
@ 2020-05-28 14:44         ` Michal Hocko
  2 siblings, 0 replies; 20+ messages in thread
From: Michal Hocko @ 2020-05-28 14:44 UTC (permalink / raw)
  To: Zefan Li
  Cc: Andrew Morton, Johannes Weiner, Vladimir Davydov, Cgroups,
	linux-mm, Roman Gushchin, Shakeel Butt

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@cmpxchg.org>
> Signed-off-by: Zefan Li <lizefan@huawei.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
> 
> 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


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-05-28 14:44 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-13  7:28 [PATCH] memcg: Fix memcg_kmem_bypass() for remote memcg charging Zefan Li
2020-05-13  9:05 ` Michal Hocko
2020-05-13 11:19   ` Zefan Li
2020-05-13 11:29     ` Michal Hocko
2020-05-13 11:47       ` [PATCH v2] " Zefan Li
2020-05-13 12:22         ` Shakeel Butt
2020-05-13 13:05         ` Johannes Weiner
2020-05-13 16:11         ` Roman Gushchin
2020-05-14  1:16           ` Zefan Li
2020-05-14 22:52             ` Roman Gushchin
2020-05-15  6:56               ` Michal Hocko
2020-05-15  8:20                 ` Zefan Li
2020-05-15  8:34                   ` Michal Hocko
2020-05-15 16:22                     ` Shakeel Butt
2020-05-15 17:31                       ` Roman Gushchin
2020-05-18  9:13                       ` Michal Hocko
2020-05-26  1:25       ` [PATCH v3] " Zefan Li
2020-05-26 15:53         ` Roman Gushchin
2020-05-27 16:50         ` Shakeel Butt
2020-05-28 14:44         ` Michal Hocko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).