| Message ID | 20230527103126.398267-1-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | memcg: remove unused mem_cgroup_from_obj() | expand |
On Fri, May 26, 2023 at 7:40 PM Miaohe Lin <linmiaohe@huawei.com> wrote: > > The function mem_cgroup_from_obj() is not used anymore. Remove it and > clean up relevant comments. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > include/linux/memcontrol.h | 6 ------ > mm/memcontrol.c | 31 ------------------------------- > 2 files changed, 37 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index 00a88cf947e1..ce8c2355ed9f 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -1813,7 +1813,6 @@ static inline int memcg_kmem_id(struct mem_cgroup *memcg) > return memcg ? memcg->kmemcg_id : -1; > } > > -struct mem_cgroup *mem_cgroup_from_obj(void *p); > struct mem_cgroup *mem_cgroup_from_slab_obj(void *p); > > static inline void count_objcg_event(struct obj_cgroup *objcg, > @@ -1876,11 +1875,6 @@ static inline int memcg_kmem_id(struct mem_cgroup *memcg) > return -1; > } > > -static inline struct mem_cgroup *mem_cgroup_from_obj(void *p) > -{ > - return NULL; > -} > - > static inline struct mem_cgroup *mem_cgroup_from_slab_obj(void *p) > { > return NULL; > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 6a3d4ce87b8a..532b29c9a0fe 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -2972,37 +2972,6 @@ struct mem_cgroup *mem_cgroup_from_obj_folio(struct folio *folio, void *p) > /* > * Returns a pointer to the memory cgroup to which the kernel object is charged. > * > - * A passed kernel object can be a slab object, vmalloc object or a generic > - * kernel page, so different mechanisms for getting the memory cgroup pointer > - * should be used. > - * > - * In certain cases (e.g. kernel stacks or large kmallocs with SLUB) the caller > - * can not know for sure how the kernel object is implemented. > - * mem_cgroup_from_obj() can be safely used in such cases. > - * > - * The caller must ensure the memcg lifetime, e.g. by taking rcu_read_lock(), > - * cgroup_mutex, etc. > - */ > -struct mem_cgroup *mem_cgroup_from_obj(void *p) > -{ > - struct folio *folio; > - > - if (mem_cgroup_disabled()) > - return NULL; > - > - if (unlikely(is_vmalloc_addr(p))) > - folio = page_folio(vmalloc_to_page(p)); > - else > - folio = virt_to_folio(p); > - > - return mem_cgroup_from_obj_folio(folio, p); > -} > - > -/* > - * Returns a pointer to the memory cgroup to which the kernel object is charged. > - * Similar to mem_cgroup_from_obj(), but faster and not suitable for objects, > - * allocated using vmalloc(). Perhaps keep the line about not being suitable for objects allocated using vmalloc()? To be fair it's obvious from the function name, but I am guessing whoever added it did for a reason. I don't feel strongly either way, LGTM. I can't see any references in Linus's tree or mm-unstable. Reviewed-by: Yosry Ahmed <yosryahmed@google.com> > - * > * A passed kernel object must be a slab object or a generic kernel page. > * > * The caller must ensure the memcg lifetime, e.g. by taking rcu_read_lock(), > -- > 2.27.0 >
On Sat, May 27, 2023 at 06:31:26PM +0800, Miaohe Lin wrote: > The function mem_cgroup_from_obj() is not used anymore. Remove it and > clean up relevant comments. You should have looked at the git history to see why it was created and who used it. Shakeel, Vasily, are you going to retry adding commit 1d0403d20f6c?
On Fri, May 26, 2023 at 9:01 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Sat, May 27, 2023 at 06:31:26PM +0800, Miaohe Lin wrote: > > The function mem_cgroup_from_obj() is not used anymore. Remove it and > > clean up relevant comments. > > You should have looked at the git history to see why it was created > and who used it. > > Shakeel, Vasily, are you going to retry adding commit 1d0403d20f6c? That commit did not introduce the function though, no? It was introduced before it and replaced by other variants over time (like mem_cgroup_from_slab_obj()). It looks like that commit was reverted ~9 months ago. We can always bring it back if/when needed. It also looks to me that 1d0403d20f6c was using mem_cgroup_from_obj() on a struct net object, which is allocated in net_alloc() from a slab cache, so mem_cgroup_from_slab_obj() should be sufficient, no? > >
On Fri, May 26, 2023 at 09:13:05PM -0700, Yosry Ahmed wrote: > On Fri, May 26, 2023 at 9:01 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Sat, May 27, 2023 at 06:31:26PM +0800, Miaohe Lin wrote: > > > The function mem_cgroup_from_obj() is not used anymore. Remove it and > > > clean up relevant comments. > > > > You should have looked at the git history to see why it was created > > and who used it. > > > > Shakeel, Vasily, are you going to retry adding commit 1d0403d20f6c? > > That commit did not introduce the function though, no? It was > introduced before it and replaced by other variants over time (like > mem_cgroup_from_slab_obj()). It looks like that commit was reverted ~9 > months ago. We can always bring it back if/when needed. The commit immediately preceding it is fc4db90fe71e. Of course we can bring it back. It's just code. But avoiding unnecessary churn is also good. Let's wait to hear from Vasily. > It also looks to me that 1d0403d20f6c was using mem_cgroup_from_obj() > on a struct net object, which is allocated in net_alloc() from a slab > cache, so mem_cgroup_from_slab_obj() should be sufficient, no? Clearly not.
On Sat, May 27, 2023 at 8:07 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Fri, May 26, 2023 at 09:13:05PM -0700, Yosry Ahmed wrote: > > On Fri, May 26, 2023 at 9:01 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > On Sat, May 27, 2023 at 06:31:26PM +0800, Miaohe Lin wrote: > > > > The function mem_cgroup_from_obj() is not used anymore. Remove it and > > > > clean up relevant comments. > > > > > > You should have looked at the git history to see why it was created > > > and who used it. > > > > > > Shakeel, Vasily, are you going to retry adding commit 1d0403d20f6c? > > > > That commit did not introduce the function though, no? It was > > introduced before it and replaced by other variants over time (like > > mem_cgroup_from_slab_obj()). It looks like that commit was reverted ~9 > > months ago. We can always bring it back if/when needed. > > The commit immediately preceding it is fc4db90fe71e. > > Of course we can bring it back. It's just code. But avoiding > unnecessary churn is also good. Let's wait to hear from Vasily. > > > It also looks to me that 1d0403d20f6c was using mem_cgroup_from_obj() > > on a struct net object, which is allocated in net_alloc() from a slab > > cache, so mem_cgroup_from_slab_obj() should be sufficient, no? > > Clearly not. I dived deeper into the history on LKML, and you are right: https://lore.kernel.org/all/Yp4F6n2Ie32re7Ed@qian/ I still do not understand why mem_cgroup_from_slab_obj() would not be sufficient, so I am hoping Vasily or Shakeel can help me understand here. Seems to be something arch-specific. Thanks for digging this up, Matthew.
> On May 28, 2023, at 02:54, Yosry Ahmed <yosryahmed@google.com> wrote: > > On Sat, May 27, 2023 at 8:07 AM Matthew Wilcox <willy@infradead.org> wrote: >> >> On Fri, May 26, 2023 at 09:13:05PM -0700, Yosry Ahmed wrote: >>> On Fri, May 26, 2023 at 9:01 PM Matthew Wilcox <willy@infradead.org> wrote: >>>> >>>> On Sat, May 27, 2023 at 06:31:26PM +0800, Miaohe Lin wrote: >>>>> The function mem_cgroup_from_obj() is not used anymore. Remove it and >>>>> clean up relevant comments. >>>> >>>> You should have looked at the git history to see why it was created >>>> and who used it. >>>> >>>> Shakeel, Vasily, are you going to retry adding commit 1d0403d20f6c? >>> >>> That commit did not introduce the function though, no? It was >>> introduced before it and replaced by other variants over time (like >>> mem_cgroup_from_slab_obj()). It looks like that commit was reverted ~9 >>> months ago. We can always bring it back if/when needed. >> >> The commit immediately preceding it is fc4db90fe71e. >> >> Of course we can bring it back. It's just code. But avoiding >> unnecessary churn is also good. Let's wait to hear from Vasily. >> >>> It also looks to me that 1d0403d20f6c was using mem_cgroup_from_obj() >>> on a struct net object, which is allocated in net_alloc() from a slab >>> cache, so mem_cgroup_from_slab_obj() should be sufficient, no? >> >> Clearly not. > > I dived deeper into the history on LKML, and you are right: > https://lore.kernel.org/all/Yp4F6n2Ie32re7Ed@qian/ > > I still do not understand why mem_cgroup_from_slab_obj() would not be > sufficient, so I am hoping Vasily or Shakeel can help me understand > here. Seems to be something arch-specific. I think it is because *init_net* which is not allocated from slab meant its address does not belong to linear mapping addresses on arm64. However, virt_to_page() is only applicable to linear mapping addresses. So, mem_cgroup_from_slab_obj() is not sufficient. mem_cgroup_from_obj() is used in this case, which will use vmalloc_to_page() for the page associated with *init_net*. If Vasily does not want to bring commit 1d0403d20f6c back, this patch LGTM. Otherwise, let's wait for Vasily. Thanks.
On Sun, May 28, 2023 at 09:01:37PM +0800, Muchun Song wrote: > with *init_net*. If Vasily does not want to bring commit 1d0403d20f6c back, > this patch LGTM. Otherwise, let's wait for Vasily. If we're not going to bring back 1d0403d20f6c then we should simply revert fc4db90fe71e instead of applying this patch.
On Sun, May 28, 2023 at 6:02 AM Muchun Song <muchun.song@linux.dev> wrote: > > > > > On May 28, 2023, at 02:54, Yosry Ahmed <yosryahmed@google.com> wrote: > > > > On Sat, May 27, 2023 at 8:07 AM Matthew Wilcox <willy@infradead.org> wrote: > >> > >> On Fri, May 26, 2023 at 09:13:05PM -0700, Yosry Ahmed wrote: > >>> On Fri, May 26, 2023 at 9:01 PM Matthew Wilcox <willy@infradead.org> wrote: > >>>> > >>>> On Sat, May 27, 2023 at 06:31:26PM +0800, Miaohe Lin wrote: > >>>>> The function mem_cgroup_from_obj() is not used anymore. Remove it and > >>>>> clean up relevant comments. > >>>> > >>>> You should have looked at the git history to see why it was created > >>>> and who used it. > >>>> > >>>> Shakeel, Vasily, are you going to retry adding commit 1d0403d20f6c? > >>> > >>> That commit did not introduce the function though, no? It was > >>> introduced before it and replaced by other variants over time (like > >>> mem_cgroup_from_slab_obj()). It looks like that commit was reverted ~9 > >>> months ago. We can always bring it back if/when needed. > >> > >> The commit immediately preceding it is fc4db90fe71e. > >> > >> Of course we can bring it back. It's just code. But avoiding > >> unnecessary churn is also good. Let's wait to hear from Vasily. > >> > >>> It also looks to me that 1d0403d20f6c was using mem_cgroup_from_obj() > >>> on a struct net object, which is allocated in net_alloc() from a slab > >>> cache, so mem_cgroup_from_slab_obj() should be sufficient, no? > >> > >> Clearly not. > > > > I dived deeper into the history on LKML, and you are right: > > https://lore.kernel.org/all/Yp4F6n2Ie32re7Ed@qian/ > > > > I still do not understand why mem_cgroup_from_slab_obj() would not be > > sufficient, so I am hoping Vasily or Shakeel can help me understand > > here. Seems to be something arch-specific. > > I think it is because *init_net* which is not allocated from slab meant > its address does not belong to linear mapping addresses on arm64. However, > virt_to_page() is only applicable to linear mapping addresses. So, > mem_cgroup_from_slab_obj() is not sufficient. mem_cgroup_from_obj() is used > in this case, which will use vmalloc_to_page() for the page associated > with *init_net*. If Vasily does not want to bring commit 1d0403d20f6c back, > this patch LGTM. Otherwise, let's wait for Vasily. I see, thanks for the context, Muchun! > > Thanks. >
On Sun, May 28, 2023 at 08:36:43PM +0100, Matthew Wilcox wrote: > On Sun, May 28, 2023 at 09:01:37PM +0800, Muchun Song wrote: > > with *init_net*. If Vasily does not want to bring commit 1d0403d20f6c back, > > this patch LGTM. Otherwise, let's wait for Vasily. > > If we're not going to bring back 1d0403d20f6c then we should > simply revert fc4db90fe71e instead of applying this patch. Initially I was thinking of adding virt_addr_valid() check in the mem_cgroup_from_obj() but it seems like that check is not cheap on arm64. I don't have any quick solutions other than adding a check against init_net in __register_pernet_operations(). I will wait for couple of days for Vasily otherwise I will retry 1d0403d20f6c with the init_net check in __register_pernet_operations().
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 00a88cf947e1..ce8c2355ed9f 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -1813,7 +1813,6 @@ static inline int memcg_kmem_id(struct mem_cgroup *memcg) return memcg ? memcg->kmemcg_id : -1; } -struct mem_cgroup *mem_cgroup_from_obj(void *p); struct mem_cgroup *mem_cgroup_from_slab_obj(void *p); static inline void count_objcg_event(struct obj_cgroup *objcg, @@ -1876,11 +1875,6 @@ static inline int memcg_kmem_id(struct mem_cgroup *memcg) return -1; } -static inline struct mem_cgroup *mem_cgroup_from_obj(void *p) -{ - return NULL; -} - static inline struct mem_cgroup *mem_cgroup_from_slab_obj(void *p) { return NULL; diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 6a3d4ce87b8a..532b29c9a0fe 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2972,37 +2972,6 @@ struct mem_cgroup *mem_cgroup_from_obj_folio(struct folio *folio, void *p) /* * Returns a pointer to the memory cgroup to which the kernel object is charged. * - * A passed kernel object can be a slab object, vmalloc object or a generic - * kernel page, so different mechanisms for getting the memory cgroup pointer - * should be used. - * - * In certain cases (e.g. kernel stacks or large kmallocs with SLUB) the caller - * can not know for sure how the kernel object is implemented. - * mem_cgroup_from_obj() can be safely used in such cases. - * - * The caller must ensure the memcg lifetime, e.g. by taking rcu_read_lock(), - * cgroup_mutex, etc. - */ -struct mem_cgroup *mem_cgroup_from_obj(void *p) -{ - struct folio *folio; - - if (mem_cgroup_disabled()) - return NULL; - - if (unlikely(is_vmalloc_addr(p))) - folio = page_folio(vmalloc_to_page(p)); - else - folio = virt_to_folio(p); - - return mem_cgroup_from_obj_folio(folio, p); -} - -/* - * Returns a pointer to the memory cgroup to which the kernel object is charged. - * Similar to mem_cgroup_from_obj(), but faster and not suitable for objects, - * allocated using vmalloc(). - * * A passed kernel object must be a slab object or a generic kernel page. * * The caller must ensure the memcg lifetime, e.g. by taking rcu_read_lock(),
The function mem_cgroup_from_obj() is not used anymore. Remove it and clean up relevant comments. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- include/linux/memcontrol.h | 6 ------ mm/memcontrol.c | 31 ------------------------------- 2 files changed, 37 deletions(-)