[PATCH] memcg: remove unused mem_cgroup_from_obj()

[PATCH] memcg: remove unused mem_cgroup_from_obj()

[Miaohe Lin]

The function mem_cgroup_from_obj() is not used anymore. Remove it and
clean up relevant comments.

Signed-off-by: Miaohe Lin <linmiaohe-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 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().
- *
  * 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

Re: [PATCH] memcg: remove unused mem_cgroup_from_obj()

[Yosry Ahmed]

On Fri, May 26, 2023 at 7:40 PM Miaohe Lin <linmiaohe-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:
>
> The function mem_cgroup_from_obj() is not used anymore. Remove it and
> clean up relevant comments.
>
> Signed-off-by: Miaohe Lin <linmiaohe-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>


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

Re: [PATCH] memcg: remove unused mem_cgroup_from_obj()

[Matthew Wilcox]

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?

Re: [PATCH] memcg: remove unused mem_cgroup_from_obj()

[Yosry Ahmed]

On Fri, May 26, 2023 at 9:01 PM Matthew Wilcox <willy-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.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?

>
>

Re: [PATCH] memcg: remove unused mem_cgroup_from_obj()

[Matthew Wilcox]

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-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.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.

Re: [PATCH] memcg: remove unused mem_cgroup_from_obj()

[Yosry Ahmed]

On Sat, May 27, 2023 at 8:07 AM Matthew Wilcox <willy-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.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.

Re: [PATCH] memcg: remove unused mem_cgroup_from_obj()

[Muchun Song]

> On May 28, 2023, at 02:54, Yosry Ahmed <yosryahmed-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> 
> On Sat, May 27, 2023 at 8:07 AM Matthew Wilcox <willy-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.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-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.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.

Re: [PATCH] memcg: remove unused mem_cgroup_from_obj()

[Matthew Wilcox]

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.

Re: [PATCH] memcg: remove unused mem_cgroup_from_obj()

[Shakeel Butt]

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().

Re: [PATCH] memcg: remove unused mem_cgroup_from_obj()

[Yosry Ahmed]

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