[PATCH] mm: memcontrol: remove rcu_read_lock from get_mem_cgroup_from_page

[PATCH] mm: memcontrol: remove rcu_read_lock from get_mem_cgroup_from_page

[Muchun Song]

The get_mem_cgroup_from_page() is called under page lock, so the page
memcg cannot be changed under us. Also, css_get is enough because page
has a reference to the memcg.

If we really want to make the get_mem_cgroup_from_page() suitable for
arbitrary page, we should use page_memcg_rcu() instead of page_memcg()
and call it after rcu_read_lock().

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/memcontrol.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 87f01bc05d1f..6c7f1ea3955e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1063,16 +1063,15 @@ EXPORT_SYMBOL(get_mem_cgroup_from_mm);
  */
 struct mem_cgroup *get_mem_cgroup_from_page(struct page *page)
 {
-	struct mem_cgroup *memcg = page_memcg(page);
+	struct mem_cgroup *memcg;
 
 	if (mem_cgroup_disabled())
 		return NULL;
 
-	rcu_read_lock();
 	/* Page should not get uncharged and freed memcg under us. */
-	if (!memcg || WARN_ON_ONCE(!css_tryget(&memcg->css)))
-		memcg = root_mem_cgroup;
-	rcu_read_unlock();
+	memcg = page_memcg(page) ? : root_mem_cgroup;
+	css_get(&memcg->css);
+
 	return memcg;
 }
 EXPORT_SYMBOL(get_mem_cgroup_from_page);
-- 
2.11.0

Re: [PATCH] mm: memcontrol: remove rcu_read_lock from get_mem_cgroup_from_page

[Michal Hocko]

On Fri 05-02-21 14:27:19, Muchun Song wrote:
> The get_mem_cgroup_from_page() is called under page lock, so the page
> memcg cannot be changed under us.

Where is the page lock enforced?

> Also, css_get is enough because page
> has a reference to the memcg.

tryget used to be there to guard against offlined memcg but we have
concluded this is impossible in this path. tryget stayed there to catch
some unexpected cases IIRC.

> If we really want to make the get_mem_cgroup_from_page() suitable for
> arbitrary page, we should use page_memcg_rcu() instead of page_memcg()
> and call it after rcu_read_lock().

What is the primary motivation to change this code? is the overhead of
tryget/RCU something that needs optimizing?
 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/memcontrol.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 87f01bc05d1f..6c7f1ea3955e 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1063,16 +1063,15 @@ EXPORT_SYMBOL(get_mem_cgroup_from_mm);
>   */
>  struct mem_cgroup *get_mem_cgroup_from_page(struct page *page)
>  {
> -	struct mem_cgroup *memcg = page_memcg(page);
> +	struct mem_cgroup *memcg;
>  
>  	if (mem_cgroup_disabled())
>  		return NULL;
>  
> -	rcu_read_lock();
>  	/* Page should not get uncharged and freed memcg under us. */
> -	if (!memcg || WARN_ON_ONCE(!css_tryget(&memcg->css)))
> -		memcg = root_mem_cgroup;
> -	rcu_read_unlock();
> +	memcg = page_memcg(page) ? : root_mem_cgroup;
> +	css_get(&memcg->css);
> +
>  	return memcg;
>  }
>  EXPORT_SYMBOL(get_mem_cgroup_from_page);
> -- 
> 2.11.0

-- 
Michal Hocko
SUSE Labs

Re: [External] Re: [PATCH] mm: memcontrol: remove rcu_read_lock from get_mem_cgroup_from_page

[Muchun Song]

On Fri, Feb 5, 2021 at 4:36 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Fri 05-02-21 14:27:19, Muchun Song wrote:
> > The get_mem_cgroup_from_page() is called under page lock, so the page
> > memcg cannot be changed under us.
>
> Where is the page lock enforced?

Because it is called from alloc_page_buffers(). This path is under
page lock.

>
> > Also, css_get is enough because page
> > has a reference to the memcg.
>
> tryget used to be there to guard against offlined memcg but we have
> concluded this is impossible in this path. tryget stayed there to catch
> some unexpected cases IIRC.

Yeah, it can catch some unexpected cases. But why is this path
special so that we need a tryget?

>
> > If we really want to make the get_mem_cgroup_from_page() suitable for
> > arbitrary page, we should use page_memcg_rcu() instead of page_memcg()
> > and call it after rcu_read_lock().
>
> What is the primary motivation to change this code? is the overhead of
> tryget/RCU something that needs optimizing?

Actually, the rcu_read_lock() is not necessary here. So it is better to
remove it (indeed reduce some code).

Thanks.


>
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > ---
> >  mm/memcontrol.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 87f01bc05d1f..6c7f1ea3955e 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -1063,16 +1063,15 @@ EXPORT_SYMBOL(get_mem_cgroup_from_mm);
> >   */
> >  struct mem_cgroup *get_mem_cgroup_from_page(struct page *page)
> >  {
> > -     struct mem_cgroup *memcg = page_memcg(page);
> > +     struct mem_cgroup *memcg;
> >
> >       if (mem_cgroup_disabled())
> >               return NULL;
> >
> > -     rcu_read_lock();
> >       /* Page should not get uncharged and freed memcg under us. */
> > -     if (!memcg || WARN_ON_ONCE(!css_tryget(&memcg->css)))
> > -             memcg = root_mem_cgroup;
> > -     rcu_read_unlock();
> > +     memcg = page_memcg(page) ? : root_mem_cgroup;
> > +     css_get(&memcg->css);
> > +
> >       return memcg;
> >  }
> >  EXPORT_SYMBOL(get_mem_cgroup_from_page);
> > --
> > 2.11.0
>
> --
> Michal Hocko
> SUSE Labs

Re: [External] Re: [PATCH] mm: memcontrol: remove rcu_read_lock from get_mem_cgroup_from_page

[Michal Hocko]

On Fri 05-02-21 17:14:30, Muchun Song wrote:
> On Fri, Feb 5, 2021 at 4:36 PM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Fri 05-02-21 14:27:19, Muchun Song wrote:
> > > The get_mem_cgroup_from_page() is called under page lock, so the page
> > > memcg cannot be changed under us.
> >
> > Where is the page lock enforced?
> 
> Because it is called from alloc_page_buffers(). This path is under
> page lock.

I do not see any page lock enforecement there. There is not even a
comment requiring that. Can we grow more users where this is not the
case? There is no actual relation between alloc_page_buffers and
get_mem_cgroup_from_page except that the former is the only _current_
existing user. I would be careful to dictate locking based solely on
that.

> > > Also, css_get is enough because page
> > > has a reference to the memcg.
> >
> > tryget used to be there to guard against offlined memcg but we have
> > concluded this is impossible in this path. tryget stayed there to catch
> > some unexpected cases IIRC.
> 
> Yeah, it can catch some unexpected cases. But why is this path
> special so that we need a tryget?

I do not remember details and the changelog of that change is not
explicit but I suspect it was just because this one could trigger as
there are external callers to memcg. Is this protection needed? I am not
sure, this is for you to justify if you want to remove it.

> > > If we really want to make the get_mem_cgroup_from_page() suitable for
> > > arbitrary page, we should use page_memcg_rcu() instead of page_memcg()
> > > and call it after rcu_read_lock().
> >
> > What is the primary motivation to change this code? is the overhead of
> > tryget/RCU something that needs optimizing?
> 
> Actually, the rcu_read_lock() is not necessary here. So it is better to
> remove it (indeed reduce some code).

Please state your reasoning in the changelog including benefits we get
from it.
-- 
Michal Hocko
SUSE Labs

Re: [External] Re: [PATCH] mm: memcontrol: remove rcu_read_lock from get_mem_cgroup_from_page

[Muchun Song]

On Fri, Feb 5, 2021 at 6:32 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Fri 05-02-21 17:14:30, Muchun Song wrote:
> > On Fri, Feb 5, 2021 at 4:36 PM Michal Hocko <mhocko@suse.com> wrote:
> > >
> > > On Fri 05-02-21 14:27:19, Muchun Song wrote:
> > > > The get_mem_cgroup_from_page() is called under page lock, so the page
> > > > memcg cannot be changed under us.
> > >
> > > Where is the page lock enforced?
> >
> > Because it is called from alloc_page_buffers(). This path is under
> > page lock.
>
> I do not see any page lock enforecement there. There is not even a
> comment requiring that. Can we grow more users where this is not the
> case? There is no actual relation between alloc_page_buffers and
> get_mem_cgroup_from_page except that the former is the only _current_
> existing user. I would be careful to dictate locking based solely on
> that.

Yeah, there is no comment requiring that. I have seen all the callers
of the alloc_page_buffers. I found that it is under page lock.
But it seems it is not the key point here. I should delete those
comments from the commit log.

>
> > > > Also, css_get is enough because page
> > > > has a reference to the memcg.
> > >
> > > tryget used to be there to guard against offlined memcg but we have
> > > concluded this is impossible in this path. tryget stayed there to catch
> > > some unexpected cases IIRC.
> >
> > Yeah, it can catch some unexpected cases. But why is this path
> > special so that we need a tryget?
>
> I do not remember details and the changelog of that change is not
> explicit but I suspect it was just because this one could trigger as
> there are external callers to memcg. Is this protection needed? I am not
> sure, this is for you to justify if you want to remove it.

I am sure it is not needed.

>
> > > > If we really want to make the get_mem_cgroup_from_page() suitable for
> > > > arbitrary page, we should use page_memcg_rcu() instead of page_memcg()
> > > > and call it after rcu_read_lock().
> > >
> > > What is the primary motivation to change this code? is the overhead of
> > > tryget/RCU something that needs optimizing?
> >
> > Actually, the rcu_read_lock() is not necessary here. So it is better to
> > remove it (indeed reduce some code).
>
> Please state your reasoning in the changelog including benefits we get
> from it.

OK.

> --
> Michal Hocko
> SUSE Labs

Re: [External] Re: [PATCH] mm: memcontrol: remove rcu_read_lock from get_mem_cgroup_from_page

[Shakeel Butt]

+Cc Roman

On Fri, Feb 5, 2021 at 2:49 AM Michal Hocko <mhocko@suse.com> wrote:
>
[snip]
> > > > Also, css_get is enough because page
> > > > has a reference to the memcg.
> > >
> > > tryget used to be there to guard against offlined memcg but we have
> > > concluded this is impossible in this path. tryget stayed there to catch
> > > some unexpected cases IIRC.
> >
> > Yeah, it can catch some unexpected cases. But why is this path
> > special so that we need a tryget?
>
> I do not remember details and the changelog of that change is not
> explicit but I suspect it was just because this one could trigger as
> there are external callers to memcg. Is this protection needed? I am not
> sure, this is for you to justify if you want to remove it.
>

It used to be css_tryget_online() which was changed to css_tryget()
and from the discussion at [1], it seemed css_get() would be enough
but we took a safer route.

Anyways, I think we can either take the page_memcg_rcu() route or put
explicit restrictions with page lock or lock_page_memcg() to guarantee
page and memcg binding. I don't have a strong opinion either way but I
think removing restrictions in future for new use-cases will be much
harder, so, page_memcg_rcu() approach seems more appropriate at least
for now.

[1] https://lore.kernel.org/linux-mm/CALvZod5pAv=u8L2Tgk0hDY7XAiiF2dvjC1omQ5BSfzFu_2zSXA@mail.gmail.com/

Re: [External] Re: [PATCH] mm: memcontrol: remove rcu_read_lock from get_mem_cgroup_from_page

[Michal Hocko]

On Fri 05-02-21 07:59:06, Shakeel Butt wrote:
> +Cc Roman
> 
> On Fri, Feb 5, 2021 at 2:49 AM Michal Hocko <mhocko@suse.com> wrote:
> >
> [snip]
> > > > > Also, css_get is enough because page
> > > > > has a reference to the memcg.
> > > >
> > > > tryget used to be there to guard against offlined memcg but we have
> > > > concluded this is impossible in this path. tryget stayed there to catch
> > > > some unexpected cases IIRC.
> > >
> > > Yeah, it can catch some unexpected cases. But why is this path
> > > special so that we need a tryget?
> >
> > I do not remember details and the changelog of that change is not
> > explicit but I suspect it was just because this one could trigger as
> > there are external callers to memcg. Is this protection needed? I am not
> > sure, this is for you to justify if you want to remove it.
> >
> 
> It used to be css_tryget_online() which was changed to css_tryget()
> and from the discussion at [1], it seemed css_get() would be enough
> but we took a safer route.
> 
> Anyways, I think we can either take the page_memcg_rcu() route or put
> explicit restrictions with page lock or lock_page_memcg() to guarantee
> page and memcg binding. I don't have a strong opinion either way but I
> think removing restrictions in future for new use-cases will be much
> harder, so, page_memcg_rcu() approach seems more appropriate at least
> for now.

Yeah, I would like to not have very special locking requirements here.
Definitely not page_lock as that one is too overloaded already.

> 
> [1] https://lore.kernel.org/linux-mm/CALvZod5pAv=u8L2Tgk0hDY7XAiiF2dvjC1omQ5BSfzFu_2zSXA@mail.gmail.com/

-- 
Michal Hocko
SUSE Labs

Re: [External] Re: [PATCH] mm: memcontrol: remove rcu_read_lock from get_mem_cgroup_from_page

[Johannes Weiner]

On Fri, Feb 05, 2021 at 11:32:24AM +0100, Michal Hocko wrote:
> On Fri 05-02-21 17:14:30, Muchun Song wrote:
> > On Fri, Feb 5, 2021 at 4:36 PM Michal Hocko <mhocko@suse.com> wrote:
> > >
> > > On Fri 05-02-21 14:27:19, Muchun Song wrote:
> > > > The get_mem_cgroup_from_page() is called under page lock, so the page
> > > > memcg cannot be changed under us.
> > >
> > > Where is the page lock enforced?
> > 
> > Because it is called from alloc_page_buffers(). This path is under
> > page lock.
> 
> I do not see any page lock enforecement there. There is not even a
> comment requiring that. Can we grow more users where this is not the
> case? There is no actual relation between alloc_page_buffers and
> get_mem_cgroup_from_page except that the former is the only _current_
> existing user. I would be careful to dictate locking based solely on
> that.

Since alloc_page_buffers() holds the page lock throughout the entire
time it uses the memcg, there is no actual reason for it to use RCU or
even acquire an additional reference on the css. We know it's pinned,
the charge pins it, and the page lock pins the charge. It can neither
move to a different cgroup nor be uncharged.

So what do you say we switch alloc_page_buffers() to page_memcg()?

And because that removes the last user of get_mem_cgroup_from_page(),
we can kill it off and worry about a good interface once a consumer
materializes for it.

diff --git a/fs/buffer.c b/fs/buffer.c
index 96c7604f69b3..12a10f461b81 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -847,7 +847,7 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
 	if (retry)
 		gfp |= __GFP_NOFAIL;
 
-	memcg = get_mem_cgroup_from_page(page);
+	memcg = page_memcg(page);
 	old_memcg = set_active_memcg(memcg);
 
 	head = NULL;
@@ -868,7 +868,6 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
 	}
 out:
 	set_active_memcg(old_memcg);
-	mem_cgroup_put(memcg);
 	return head;
 /*
  * In case anything failed, we just free everything we got.
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index a8c7a0ccc759..a44b2d51aecc 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -687,8 +687,6 @@ struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p);
 
 struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm);
 
-struct mem_cgroup *get_mem_cgroup_from_page(struct page *page);
-
 struct lruvec *lock_page_lruvec(struct page *page);
 struct lruvec *lock_page_lruvec_irq(struct page *page);
 struct lruvec *lock_page_lruvec_irqsave(struct page *page,
@@ -1169,11 +1167,6 @@ static inline struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm)
 	return NULL;
 }
 
-static inline struct mem_cgroup *get_mem_cgroup_from_page(struct page *page)
-{
-	return NULL;
-}
-
 static inline void mem_cgroup_put(struct mem_cgroup *memcg)
 {
 }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 490357945f2c..ff52550d2f65 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1048,29 +1048,6 @@ struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm)
 }
 EXPORT_SYMBOL(get_mem_cgroup_from_mm);
 
-/**
- * get_mem_cgroup_from_page: Obtain a reference on given page's memcg.
- * @page: page from which memcg should be extracted.
- *
- * Obtain a reference on page->memcg and returns it if successful. Otherwise
- * root_mem_cgroup is returned.
- */
-struct mem_cgroup *get_mem_cgroup_from_page(struct page *page)
-{
-	struct mem_cgroup *memcg = page_memcg(page);
-
-	if (mem_cgroup_disabled())
-		return NULL;
-
-	rcu_read_lock();
-	/* Page should not get uncharged and freed memcg under us. */
-	if (!memcg || WARN_ON_ONCE(!css_tryget(&memcg->css)))
-		memcg = root_mem_cgroup;
-	rcu_read_unlock();
-	return memcg;
-}
-EXPORT_SYMBOL(get_mem_cgroup_from_page);
-
 static __always_inline struct mem_cgroup *active_memcg(void)
 {
 	if (in_interrupt())

Re: [External] Re: [PATCH] mm: memcontrol: remove rcu_read_lock from get_mem_cgroup_from_page

[Shakeel Butt]

On Fri, Feb 5, 2021 at 10:31 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Fri, Feb 05, 2021 at 11:32:24AM +0100, Michal Hocko wrote:
> > On Fri 05-02-21 17:14:30, Muchun Song wrote:
> > > On Fri, Feb 5, 2021 at 4:36 PM Michal Hocko <mhocko@suse.com> wrote:
> > > >
> > > > On Fri 05-02-21 14:27:19, Muchun Song wrote:
> > > > > The get_mem_cgroup_from_page() is called under page lock, so the page
> > > > > memcg cannot be changed under us.
> > > >
> > > > Where is the page lock enforced?
> > >
> > > Because it is called from alloc_page_buffers(). This path is under
> > > page lock.
> >
> > I do not see any page lock enforecement there. There is not even a
> > comment requiring that. Can we grow more users where this is not the
> > case? There is no actual relation between alloc_page_buffers and
> > get_mem_cgroup_from_page except that the former is the only _current_
> > existing user. I would be careful to dictate locking based solely on
> > that.
>
> Since alloc_page_buffers() holds the page lock throughout the entire
> time it uses the memcg, there is no actual reason for it to use RCU or
> even acquire an additional reference on the css. We know it's pinned,
> the charge pins it, and the page lock pins the charge. It can neither
> move to a different cgroup nor be uncharged.
>
> So what do you say we switch alloc_page_buffers() to page_memcg()?
>
> And because that removes the last user of get_mem_cgroup_from_page(),
> we can kill it off and worry about a good interface once a consumer
> materializes for it.

This seems like even better approach.

>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 96c7604f69b3..12a10f461b81 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -847,7 +847,7 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
>         if (retry)
>                 gfp |= __GFP_NOFAIL;
>
> -       memcg = get_mem_cgroup_from_page(page);
> +       memcg = page_memcg(page);

A comment explaining why page_memcg(page) is safe here and then the
patch looks good to me.

>         old_memcg = set_active_memcg(memcg);
>
>         head = NULL;
> @@ -868,7 +868,6 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
>         }
>  out:
>         set_active_memcg(old_memcg);
> -       mem_cgroup_put(memcg);
>         return head;
>  /*
>   * In case anything failed, we just free everything we got.
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index a8c7a0ccc759..a44b2d51aecc 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -687,8 +687,6 @@ struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p);
>
>  struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm);
>
> -struct mem_cgroup *get_mem_cgroup_from_page(struct page *page);
> -
>  struct lruvec *lock_page_lruvec(struct page *page);
>  struct lruvec *lock_page_lruvec_irq(struct page *page);
>  struct lruvec *lock_page_lruvec_irqsave(struct page *page,
> @@ -1169,11 +1167,6 @@ static inline struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm)
>         return NULL;
>  }
>
> -static inline struct mem_cgroup *get_mem_cgroup_from_page(struct page *page)
> -{
> -       return NULL;
> -}
> -
>  static inline void mem_cgroup_put(struct mem_cgroup *memcg)
>  {
>  }
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 490357945f2c..ff52550d2f65 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1048,29 +1048,6 @@ struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm)
>  }
>  EXPORT_SYMBOL(get_mem_cgroup_from_mm);
>
> -/**
> - * get_mem_cgroup_from_page: Obtain a reference on given page's memcg.
> - * @page: page from which memcg should be extracted.
> - *
> - * Obtain a reference on page->memcg and returns it if successful. Otherwise
> - * root_mem_cgroup is returned.
> - */
> -struct mem_cgroup *get_mem_cgroup_from_page(struct page *page)
> -{
> -       struct mem_cgroup *memcg = page_memcg(page);
> -
> -       if (mem_cgroup_disabled())
> -               return NULL;
> -
> -       rcu_read_lock();
> -       /* Page should not get uncharged and freed memcg under us. */
> -       if (!memcg || WARN_ON_ONCE(!css_tryget(&memcg->css)))
> -               memcg = root_mem_cgroup;
> -       rcu_read_unlock();
> -       return memcg;
> -}
> -EXPORT_SYMBOL(get_mem_cgroup_from_page);
> -
>  static __always_inline struct mem_cgroup *active_memcg(void)
>  {
>         if (in_interrupt())

Re: [External] Re: [PATCH] mm: memcontrol: remove rcu_read_lock from get_mem_cgroup_from_page

[Muchun Song]

On Sat, Feb 6, 2021 at 2:15 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Fri, Feb 05, 2021 at 11:32:24AM +0100, Michal Hocko wrote:
> > On Fri 05-02-21 17:14:30, Muchun Song wrote:
> > > On Fri, Feb 5, 2021 at 4:36 PM Michal Hocko <mhocko@suse.com> wrote:
> > > >
> > > > On Fri 05-02-21 14:27:19, Muchun Song wrote:
> > > > > The get_mem_cgroup_from_page() is called under page lock, so the page
> > > > > memcg cannot be changed under us.
> > > >
> > > > Where is the page lock enforced?
> > >
> > > Because it is called from alloc_page_buffers(). This path is under
> > > page lock.
> >
> > I do not see any page lock enforecement there. There is not even a
> > comment requiring that. Can we grow more users where this is not the
> > case? There is no actual relation between alloc_page_buffers and
> > get_mem_cgroup_from_page except that the former is the only _current_
> > existing user. I would be careful to dictate locking based solely on
> > that.
>
> Since alloc_page_buffers() holds the page lock throughout the entire
> time it uses the memcg, there is no actual reason for it to use RCU or
> even acquire an additional reference on the css. We know it's pinned,
> the charge pins it, and the page lock pins the charge. It can neither
> move to a different cgroup nor be uncharged.

Thanks for your patient explanation.

>
> So what do you say we switch alloc_page_buffers() to page_memcg()?

It's better than mine.

>
> And because that removes the last user of get_mem_cgroup_from_page(),
> we can kill it off and worry about a good interface once a consumer
> materializes for it.
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 96c7604f69b3..12a10f461b81 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -847,7 +847,7 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
>         if (retry)
>                 gfp |= __GFP_NOFAIL;
>
> -       memcg = get_mem_cgroup_from_page(page);
> +       memcg = page_memcg(page);
>         old_memcg = set_active_memcg(memcg);
>
>         head = NULL;
> @@ -868,7 +868,6 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
>         }
>  out:
>         set_active_memcg(old_memcg);
> -       mem_cgroup_put(memcg);
>         return head;
>  /*
>   * In case anything failed, we just free everything we got.
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index a8c7a0ccc759..a44b2d51aecc 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -687,8 +687,6 @@ struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p);
>
>  struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm);
>
> -struct mem_cgroup *get_mem_cgroup_from_page(struct page *page);
> -
>  struct lruvec *lock_page_lruvec(struct page *page);
>  struct lruvec *lock_page_lruvec_irq(struct page *page);
>  struct lruvec *lock_page_lruvec_irqsave(struct page *page,
> @@ -1169,11 +1167,6 @@ static inline struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm)
>         return NULL;
>  }
>
> -static inline struct mem_cgroup *get_mem_cgroup_from_page(struct page *page)
> -{
> -       return NULL;
> -}
> -
>  static inline void mem_cgroup_put(struct mem_cgroup *memcg)
>  {
>  }
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 490357945f2c..ff52550d2f65 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1048,29 +1048,6 @@ struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm)
>  }
>  EXPORT_SYMBOL(get_mem_cgroup_from_mm);
>
> -/**
> - * get_mem_cgroup_from_page: Obtain a reference on given page's memcg.
> - * @page: page from which memcg should be extracted.
> - *
> - * Obtain a reference on page->memcg and returns it if successful. Otherwise
> - * root_mem_cgroup is returned.
> - */
> -struct mem_cgroup *get_mem_cgroup_from_page(struct page *page)
> -{
> -       struct mem_cgroup *memcg = page_memcg(page);
> -
> -       if (mem_cgroup_disabled())
> -               return NULL;
> -
> -       rcu_read_lock();
> -       /* Page should not get uncharged and freed memcg under us. */
> -       if (!memcg || WARN_ON_ONCE(!css_tryget(&memcg->css)))
> -               memcg = root_mem_cgroup;
> -       rcu_read_unlock();
> -       return memcg;
> -}
> -EXPORT_SYMBOL(get_mem_cgroup_from_page);
> -
>  static __always_inline struct mem_cgroup *active_memcg(void)
>  {
>         if (in_interrupt())

Re: [External] Re: [PATCH] mm: memcontrol: remove rcu_read_lock from get_mem_cgroup_from_page

[Michal Hocko]

On Fri 05-02-21 13:15:40, Johannes Weiner wrote:
> On Fri, Feb 05, 2021 at 11:32:24AM +0100, Michal Hocko wrote:
> > On Fri 05-02-21 17:14:30, Muchun Song wrote:
> > > On Fri, Feb 5, 2021 at 4:36 PM Michal Hocko <mhocko@suse.com> wrote:
> > > >
> > > > On Fri 05-02-21 14:27:19, Muchun Song wrote:
> > > > > The get_mem_cgroup_from_page() is called under page lock, so the page
> > > > > memcg cannot be changed under us.
> > > >
> > > > Where is the page lock enforced?
> > > 
> > > Because it is called from alloc_page_buffers(). This path is under
> > > page lock.
> > 
> > I do not see any page lock enforecement there. There is not even a
> > comment requiring that. Can we grow more users where this is not the
> > case? There is no actual relation between alloc_page_buffers and
> > get_mem_cgroup_from_page except that the former is the only _current_
> > existing user. I would be careful to dictate locking based solely on
> > that.
> 
> Since alloc_page_buffers() holds the page lock throughout the entire
> time it uses the memcg, there is no actual reason for it to use RCU or
> even acquire an additional reference on the css. We know it's pinned,
> the charge pins it, and the page lock pins the charge. It can neither
> move to a different cgroup nor be uncharged.
> 
> So what do you say we switch alloc_page_buffers() to page_memcg()?
> 
> And because that removes the last user of get_mem_cgroup_from_page(),
> we can kill it off and worry about a good interface once a consumer
> materializes for it.

Yes, this makes much more sense than impose a weird locking rules to a
more general purpose helper. Thanks!
-- 
Michal Hocko
SUSE Labs