| Message ID | e6927a82-949c-bdfd-d717-0a14743c6759@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memcg: Fix memcg_kmem_bypass() for remote memcg charging | expand |
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
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;
[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;
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;
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(+)