[PATCH 1/9] memcg: accounting for allocations called with disabled BH

[PATCH 1/9] memcg: accounting for allocations called with disabled BH

[Vasily Averin]

in_interrupt() check in memcg_kmem_bypass() is incorrect because
it does not allow to account memory allocation called from task context
with disabled BH, i.e. inside spin_lock_bh()/spin_unlock_bh() sections

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 845eec0..568f2cb 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1076,7 +1076,7 @@ static __always_inline bool memcg_kmem_bypass(void)
 		return false;
 
 	/* Memcg to charge can't be determined. */
-	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
+	if (!in_task() || !current->mm || (current->flags & PF_KTHREAD))
 		return true;
 
 	return false;
-- 
1.8.3.1

Re: [PATCH 1/9] memcg: accounting for allocations called with disabled BH

[Michal Hocko]

On Tue 09-03-21 11:03:48, Vasily Averin wrote:
> in_interrupt() check in memcg_kmem_bypass() is incorrect because
> it does not allow to account memory allocation called from task context
> with disabled BH, i.e. inside spin_lock_bh()/spin_unlock_bh() sections

Is there any existing user in the tree? Or is this more of a preparatory
patch for a later one which will need it? In other words, is this a bug
fix or a preparatory work.

> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  mm/memcontrol.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 845eec0..568f2cb 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1076,7 +1076,7 @@ static __always_inline bool memcg_kmem_bypass(void)
>  		return false;
>  
>  	/* Memcg to charge can't be determined. */
> -	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
> +	if (!in_task() || !current->mm || (current->flags & PF_KTHREAD))
>  		return true;
>  
>  	return false;
> -- 
> 1.8.3.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/9] memcg: accounting for allocations called with disabled BH

[Vasily Averin]

On 3/9/21 5:57 PM, Michal Hocko wrote:
> On Tue 09-03-21 11:03:48, Vasily Averin wrote:
>> in_interrupt() check in memcg_kmem_bypass() is incorrect because
>> it does not allow to account memory allocation called from task context
>> with disabled BH, i.e. inside spin_lock_bh()/spin_unlock_bh() sections
> 
> Is there any existing user in the tree? Or is this more of a preparatory
> patch for a later one which will need it? In other words, is this a bug
> fix or a preparatory work.

struct fib6_node objects are allocated by this way
net/ipv6/route.c::__ip6_ins_rt()
...        write_lock_bh(&table->tb6_lock);
        err = fib6_add(&table->tb6_root, rt, info, mxc);
        write_unlock_bh(&table->tb6_lock);

I spend some time to understand why properly entries from properly configured cache
was not accounted to container's memcg.

Thank you,
	Vasily Averin

Re: [PATCH 1/9] memcg: accounting for allocations called with disabled BH

[Michal Hocko]

On Wed 10-03-21 12:11:26, Vasily Averin wrote:
> On 3/9/21 5:57 PM, Michal Hocko wrote:
> > On Tue 09-03-21 11:03:48, Vasily Averin wrote:
> >> in_interrupt() check in memcg_kmem_bypass() is incorrect because
> >> it does not allow to account memory allocation called from task context
> >> with disabled BH, i.e. inside spin_lock_bh()/spin_unlock_bh() sections
> > 
> > Is there any existing user in the tree? Or is this more of a preparatory
> > patch for a later one which will need it? In other words, is this a bug
> > fix or a preparatory work.
> 
> struct fib6_node objects are allocated by this way
> net/ipv6/route.c::__ip6_ins_rt()
> ...        write_lock_bh(&table->tb6_lock);
>         err = fib6_add(&table->tb6_root, rt, info, mxc);
>         write_unlock_bh(&table->tb6_lock);
> 
> I spend some time to understand why properly entries from properly configured cache
> was not accounted to container's memcg.

OK, that is a valuable information. If there are no other users
currently then I would recommend squashing this patch into the one which
introduces accounting for fib6_node cache (patch 2, IIUC).
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/9] memcg: accounting for allocations called with disabled BH

[Shakeel Butt]

On Tue, Mar 9, 2021 at 12:04 AM Vasily Averin <vvs@virtuozzo.com> wrote:
>
> in_interrupt() check in memcg_kmem_bypass() is incorrect because
> it does not allow to account memory allocation called from task context
> with disabled BH, i.e. inside spin_lock_bh()/spin_unlock_bh() sections
>
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

In that file in_interrupt() is used at other places too. Should we
change those too?

> ---
>  mm/memcontrol.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 845eec0..568f2cb 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1076,7 +1076,7 @@ static __always_inline bool memcg_kmem_bypass(void)
>                 return false;
>
>         /* Memcg to charge can't be determined. */
> -       if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
> +       if (!in_task() || !current->mm || (current->flags & PF_KTHREAD))
>                 return true;
>
>         return false;
> --
> 1.8.3.1
>

Re: [PATCH 1/9] memcg: accounting for allocations called with disabled BH

[Roman Gushchin]

On Tue, Mar 09, 2021 at 11:39:41AM -0800, Shakeel Butt wrote:
> On Tue, Mar 9, 2021 at 12:04 AM Vasily Averin <vvs@virtuozzo.com> wrote:
> >
> > in_interrupt() check in memcg_kmem_bypass() is incorrect because
> > it does not allow to account memory allocation called from task context
> > with disabled BH, i.e. inside spin_lock_bh()/spin_unlock_bh() sections
> >
> > Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> 
> In that file in_interrupt() is used at other places too. Should we
> change those too?

Yes, it seems so. Let me prepare a fix (it seems like most of them were
introduced by me).

Thanks!

Re: [PATCH 1/9] memcg: accounting for allocations called with disabled BH

[Vasily Averin]

On 3/9/21 11:18 PM, Roman Gushchin wrote:
> On Tue, Mar 09, 2021 at 11:39:41AM -0800, Shakeel Butt wrote:
>> On Tue, Mar 9, 2021 at 12:04 AM Vasily Averin <vvs@virtuozzo.com> wrote:
>>>
>>> in_interrupt() check in memcg_kmem_bypass() is incorrect because
>>> it does not allow to account memory allocation called from task context
>>> with disabled BH, i.e. inside spin_lock_bh()/spin_unlock_bh() sections
>>>
>>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
>>
>> In that file in_interrupt() is used at other places too. Should we
>> change those too?
> 
> Yes, it seems so. Let me prepare a fix (it seems like most of them were
> introduced by me).

No objects from my side, just keep me informed and add me in "Reported-by:" in your patches. 

Thank you,
	Vasily Averin

Re: [PATCH 1/9] memcg: accounting for allocations called with disabled BH

[Michal Hocko]

On Tue 09-03-21 12:18:28, Roman Gushchin wrote:
> On Tue, Mar 09, 2021 at 11:39:41AM -0800, Shakeel Butt wrote:
> > On Tue, Mar 9, 2021 at 12:04 AM Vasily Averin <vvs@virtuozzo.com> wrote:
> > >
> > > in_interrupt() check in memcg_kmem_bypass() is incorrect because
> > > it does not allow to account memory allocation called from task context
> > > with disabled BH, i.e. inside spin_lock_bh()/spin_unlock_bh() sections
> > >
> > > Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> > 
> > In that file in_interrupt() is used at other places too. Should we
> > change those too?
> 
> Yes, it seems so. Let me prepare a fix (it seems like most of them were
> introduced by me).

Does this affect any existing in-tree users?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/9] memcg: accounting for allocations called with disabled BH

[Roman Gushchin]

On Wed, Mar 10, 2021 at 10:42:29AM +0100, Michal Hocko wrote:
> On Tue 09-03-21 12:18:28, Roman Gushchin wrote:
> > On Tue, Mar 09, 2021 at 11:39:41AM -0800, Shakeel Butt wrote:
> > > On Tue, Mar 9, 2021 at 12:04 AM Vasily Averin <vvs@virtuozzo.com> wrote:
> > > >
> > > > in_interrupt() check in memcg_kmem_bypass() is incorrect because
> > > > it does not allow to account memory allocation called from task context
> > > > with disabled BH, i.e. inside spin_lock_bh()/spin_unlock_bh() sections
> > > >
> > > > Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> > > 
> > > In that file in_interrupt() is used at other places too. Should we
> > > change those too?
> > 
> > Yes, it seems so. Let me prepare a fix (it seems like most of them were
> > introduced by me).
> 
> Does this affect any existing in-tree users?

I'll double check this, but I doubt. We should fix it anyway, but I don't think
we need any stable backports.

Re: [PATCH 1/9] memcg: accounting for allocations called with disabled BH

[Vasily Averin]

On 3/9/21 10:39 PM, Shakeel Butt wrote:
> On Tue, Mar 9, 2021 at 12:04 AM Vasily Averin <vvs@virtuozzo.com> wrote:
>>
>> in_interrupt() check in memcg_kmem_bypass() is incorrect because
>> it does not allow to account memory allocation called from task context
>> with disabled BH, i.e. inside spin_lock_bh()/spin_unlock_bh() sections
>>
>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> 
> In that file in_interrupt() is used at other places too. Should we
> change those too?

Yes, you're right, in_interrupt() is used incorrectly in other places too,
but 
1) these cases are not so critical as this one,
2) and are not related to current patch set

They can be replaced later without urgency
(unless I missed something imporant).

thank you,
	Vasily Averin

>> ---
>>  mm/memcontrol.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 845eec0..568f2cb 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -1076,7 +1076,7 @@ static __always_inline bool memcg_kmem_bypass(void)
>>                 return false;
>>
>>         /* Memcg to charge can't be determined. */
>> -       if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
>> +       if (!in_task() || !current->mm || (current->flags & PF_KTHREAD))
>>                 return true;
>>
>>         return false;
>> --
>> 1.8.3.1
>>

Re: [PATCH 1/9] memcg: accounting for allocations called with disabled BH

[Roman Gushchin]

On Tue, Mar 09, 2021 at 11:03:48AM +0300, Vasily Averin wrote:
> in_interrupt() check in memcg_kmem_bypass() is incorrect because
> it does not allow to account memory allocation called from task context
> with disabled BH, i.e. inside spin_lock_bh()/spin_unlock_bh() sections
> 
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

Good catch!

It looks like the bug was there for years: in_interrupt() was there since
the commit 7ae1e1d0f8ac ("memcg: kmem controller infrastructure") from 2012!
So I guess there is no point for a stable fix, but it's definitely nice to
have it fixed.

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

for this patch and the rest of the series.

Thank you!

> ---
>  mm/memcontrol.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 845eec0..568f2cb 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1076,7 +1076,7 @@ static __always_inline bool memcg_kmem_bypass(void)
>  		return false;
>  
>  	/* Memcg to charge can't be determined. */
> -	if (in_interrupt() || !current->mm || (current->flags & PF_KTHREAD))
> +	if (!in_task() || !current->mm || (current->flags & PF_KTHREAD))
>  		return true;
>  
>  	return false;
> -- 
> 1.8.3.1
> 

Re: [PATCH 1/9] memcg: accounting for allocations called with disabled BH

[Vasily Averin]

On 3/9/21 11:39 PM, Roman Gushchin wrote:
> On Tue, Mar 09, 2021 at 11:03:48AM +0300, Vasily Averin wrote:
>> in_interrupt() check in memcg_kmem_bypass() is incorrect because
>> it does not allow to account memory allocation called from task context
>> with disabled BH, i.e. inside spin_lock_bh()/spin_unlock_bh() sections
>>
>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> 
> Good catch!
> 
> It looks like the bug was there for years: in_interrupt() was there since
> the commit 7ae1e1d0f8ac ("memcg: kmem controller infrastructure") from 2012!
> So I guess there is no point for a stable fix, but it's definitely nice to
> have it fixed.

I did not noticed that this problem affect existing allocation,
I just found that it blocked accounting of "struct fib6_node" in __ip6_ins_rt() 
added in my current patch set. So I do not think it should went to stable@.

Thank you,
	Vasily Averin