| Message ID | 18a0ae77-89ff-2679-ab19-378e38ce2be2@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memcg accounting from OpenVZ | expand |
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
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 >
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!
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 >
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
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 >>
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
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
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).
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?
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.
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;
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(-)