diff mbox series

[v2,15/46] mm/memcg: Add folio_uncharge_cgroup()

Message ID 20210622121551.3398730-16-willy@infradead.org (mailing list archive)
State New, archived
Headers show
Series Folio-enabling the page cache | expand

Commit Message

Matthew Wilcox June 22, 2021, 12:15 p.m. UTC
Reimplement mem_cgroup_uncharge() as a wrapper around
folio_uncharge_cgroup().

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/memcontrol.h |  5 +++++
 mm/folio-compat.c          |  5 +++++
 mm/memcontrol.c            | 14 +++++++-------
 3 files changed, 17 insertions(+), 7 deletions(-)

Comments

Christoph Hellwig June 23, 2021, 8:16 a.m. UTC | #1
On Tue, Jun 22, 2021 at 01:15:20PM +0100, Matthew Wilcox (Oracle) wrote:
> Reimplement mem_cgroup_uncharge() as a wrapper around
> folio_uncharge_cgroup().

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
Michal Hocko June 25, 2021, 8:25 a.m. UTC | #2
On Tue 22-06-21 13:15:20, Matthew Wilcox wrote:
> Reimplement mem_cgroup_uncharge() as a wrapper around
> folio_uncharge_cgroup().
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Similar to the previous patch. Is there any reason why we cannot simply
stick with mem_cgroup_{un}charge and only change the parameter to folio?

> ---
>  include/linux/memcontrol.h |  5 +++++
>  mm/folio-compat.c          |  5 +++++
>  mm/memcontrol.c            | 14 +++++++-------
>  3 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index a50e5cee6d2c..d4b2bc939eee 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -705,6 +705,7 @@ static inline bool mem_cgroup_below_min(struct mem_cgroup *memcg)
>  }
>  
>  int folio_charge_cgroup(struct folio *, struct mm_struct *, gfp_t);
> +void folio_uncharge_cgroup(struct folio *);
>  
>  int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp_mask);
>  int mem_cgroup_swapin_charge_page(struct page *page, struct mm_struct *mm,
> @@ -1224,6 +1225,10 @@ static inline int folio_charge_cgroup(struct folio *folio,
>  	return 0;
>  }
>  
> +static inline void folio_uncharge_cgroup(struct folio *folio)
> +{
> +}
> +
>  static inline int mem_cgroup_charge(struct page *page, struct mm_struct *mm,
>  				    gfp_t gfp_mask)
>  {
> diff --git a/mm/folio-compat.c b/mm/folio-compat.c
> index 1d71b8b587f8..d229b979b00d 100644
> --- a/mm/folio-compat.c
> +++ b/mm/folio-compat.c
> @@ -54,4 +54,9 @@ int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp)
>  {
>  	return folio_charge_cgroup(page_folio(page), mm, gfp);
>  }
> +
> +void mem_cgroup_uncharge(struct page *page)
> +{
> +	folio_uncharge_cgroup(page_folio(page));
> +}
>  #endif
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 69638f84d11b..a6befc0843e7 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -6717,24 +6717,24 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug)
>  }
>  
>  /**
> - * mem_cgroup_uncharge - uncharge a page
> - * @page: page to uncharge
> + * folio_uncharge_cgroup - Uncharge a folio.
> + * @folio: Folio to uncharge.
>   *
> - * Uncharge a page previously charged with mem_cgroup_charge().
> + * Uncharge a folio previously charged with folio_charge_cgroup().
>   */
> -void mem_cgroup_uncharge(struct page *page)
> +void folio_uncharge_cgroup(struct folio *folio)
>  {
>  	struct uncharge_gather ug;
>  
>  	if (mem_cgroup_disabled())
>  		return;
>  
> -	/* Don't touch page->lru of any random page, pre-check: */
> -	if (!page_memcg(page))
> +	/* Don't touch folio->lru of any random page, pre-check: */
> +	if (!folio_memcg(folio))
>  		return;
>  
>  	uncharge_gather_clear(&ug);
> -	uncharge_page(page, &ug);
> +	uncharge_page(&folio->page, &ug);
>  	uncharge_batch(&ug);
>  }
>  
> -- 
> 2.30.2
Matthew Wilcox June 25, 2021, 11:21 a.m. UTC | #3
On Fri, Jun 25, 2021 at 10:25:44AM +0200, Michal Hocko wrote:
> On Tue 22-06-21 13:15:20, Matthew Wilcox wrote:
> > Reimplement mem_cgroup_uncharge() as a wrapper around
> > folio_uncharge_cgroup().
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> Similar to the previous patch. Is there any reason why we cannot simply
> stick with mem_cgroup_{un}charge and only change the parameter to folio?

There are a dozen callers of mem_cgroup_charge() and most of them
aren't quite ready to convert to folios at this point in the patch
series.  So either we need a new name for the variant that takes a
folio, or we need to play fun games with _Generic to allow
mem_cgroup_charge() to take either a folio or a page, or we convert
all callers to open-code their call to page_folio, like this:

-	if (mem_cgroup_charge(vmf->cow_page, vma->vm_mm, GFP_KERNEL)) {
+	if (mem_cgroup_charge(page_folio(vmf->cow_page), vma->vm_mm,
+			GFP_KERNEL)) {

I've generally gone with creating compat functions to minimise the
merge conflicts when people are adding new callers or changing code near
existing ones.  But if you don't like the new name, we have options.

> > ---
> >  include/linux/memcontrol.h |  5 +++++
> >  mm/folio-compat.c          |  5 +++++
> >  mm/memcontrol.c            | 14 +++++++-------
> >  3 files changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index a50e5cee6d2c..d4b2bc939eee 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -705,6 +705,7 @@ static inline bool mem_cgroup_below_min(struct mem_cgroup *memcg)
> >  }
> >  
> >  int folio_charge_cgroup(struct folio *, struct mm_struct *, gfp_t);
> > +void folio_uncharge_cgroup(struct folio *);
> >  
> >  int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp_mask);
> >  int mem_cgroup_swapin_charge_page(struct page *page, struct mm_struct *mm,
> > @@ -1224,6 +1225,10 @@ static inline int folio_charge_cgroup(struct folio *folio,
> >  	return 0;
> >  }
> >  
> > +static inline void folio_uncharge_cgroup(struct folio *folio)
> > +{
> > +}
> > +
> >  static inline int mem_cgroup_charge(struct page *page, struct mm_struct *mm,
> >  				    gfp_t gfp_mask)
> >  {
> > diff --git a/mm/folio-compat.c b/mm/folio-compat.c
> > index 1d71b8b587f8..d229b979b00d 100644
> > --- a/mm/folio-compat.c
> > +++ b/mm/folio-compat.c
> > @@ -54,4 +54,9 @@ int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp)
> >  {
> >  	return folio_charge_cgroup(page_folio(page), mm, gfp);
> >  }
> > +
> > +void mem_cgroup_uncharge(struct page *page)
> > +{
> > +	folio_uncharge_cgroup(page_folio(page));
> > +}
> >  #endif
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 69638f84d11b..a6befc0843e7 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -6717,24 +6717,24 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug)
> >  }
> >  
> >  /**
> > - * mem_cgroup_uncharge - uncharge a page
> > - * @page: page to uncharge
> > + * folio_uncharge_cgroup - Uncharge a folio.
> > + * @folio: Folio to uncharge.
> >   *
> > - * Uncharge a page previously charged with mem_cgroup_charge().
> > + * Uncharge a folio previously charged with folio_charge_cgroup().
> >   */
> > -void mem_cgroup_uncharge(struct page *page)
> > +void folio_uncharge_cgroup(struct folio *folio)
> >  {
> >  	struct uncharge_gather ug;
> >  
> >  	if (mem_cgroup_disabled())
> >  		return;
> >  
> > -	/* Don't touch page->lru of any random page, pre-check: */
> > -	if (!page_memcg(page))
> > +	/* Don't touch folio->lru of any random page, pre-check: */
> > +	if (!folio_memcg(folio))
> >  		return;
> >  
> >  	uncharge_gather_clear(&ug);
> > -	uncharge_page(page, &ug);
> > +	uncharge_page(&folio->page, &ug);
> >  	uncharge_batch(&ug);
> >  }
> >  
> > -- 
> > 2.30.2
> 
> -- 
> Michal Hocko
> SUSE Labs
Michal Hocko June 25, 2021, 1:21 p.m. UTC | #4
On Fri 25-06-21 12:21:32, Matthew Wilcox wrote:
> On Fri, Jun 25, 2021 at 10:25:44AM +0200, Michal Hocko wrote:
> > On Tue 22-06-21 13:15:20, Matthew Wilcox wrote:
> > > Reimplement mem_cgroup_uncharge() as a wrapper around
> > > folio_uncharge_cgroup().
> > > 
> > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > 
> > Similar to the previous patch. Is there any reason why we cannot simply
> > stick with mem_cgroup_{un}charge and only change the parameter to folio?
> 
> There are a dozen callers of mem_cgroup_charge() and most of them
> aren't quite ready to convert to folios at this point in the patch
> series.  So either we need a new name for the variant that takes a
> folio, or we need to play fun games with _Generic to allow
> mem_cgroup_charge() to take either a folio or a page, or we convert
> all callers to open-code their call to page_folio, like this:
> 
> -	if (mem_cgroup_charge(vmf->cow_page, vma->vm_mm, GFP_KERNEL)) {
> +	if (mem_cgroup_charge(page_folio(vmf->cow_page), vma->vm_mm,
> +			GFP_KERNEL)) {
> 
> I've generally gone with creating compat functions to minimise the
> merge conflicts when people are adding new callers or changing code near
> existing ones.  But if you don't like the new name, we have options.

Well, I will not insist because I can see how the conversion is PITA in
general.
mem_cgroup_charge should be something to be added very often so if you
do not mind I would go with your above example of direct usage of
page_folio() rather than wrappers.

Thanks!
diff mbox series

Patch

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index a50e5cee6d2c..d4b2bc939eee 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -705,6 +705,7 @@  static inline bool mem_cgroup_below_min(struct mem_cgroup *memcg)
 }
 
 int folio_charge_cgroup(struct folio *, struct mm_struct *, gfp_t);
+void folio_uncharge_cgroup(struct folio *);
 
 int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp_mask);
 int mem_cgroup_swapin_charge_page(struct page *page, struct mm_struct *mm,
@@ -1224,6 +1225,10 @@  static inline int folio_charge_cgroup(struct folio *folio,
 	return 0;
 }
 
+static inline void folio_uncharge_cgroup(struct folio *folio)
+{
+}
+
 static inline int mem_cgroup_charge(struct page *page, struct mm_struct *mm,
 				    gfp_t gfp_mask)
 {
diff --git a/mm/folio-compat.c b/mm/folio-compat.c
index 1d71b8b587f8..d229b979b00d 100644
--- a/mm/folio-compat.c
+++ b/mm/folio-compat.c
@@ -54,4 +54,9 @@  int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp)
 {
 	return folio_charge_cgroup(page_folio(page), mm, gfp);
 }
+
+void mem_cgroup_uncharge(struct page *page)
+{
+	folio_uncharge_cgroup(page_folio(page));
+}
 #endif
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 69638f84d11b..a6befc0843e7 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -6717,24 +6717,24 @@  static void uncharge_page(struct page *page, struct uncharge_gather *ug)
 }
 
 /**
- * mem_cgroup_uncharge - uncharge a page
- * @page: page to uncharge
+ * folio_uncharge_cgroup - Uncharge a folio.
+ * @folio: Folio to uncharge.
  *
- * Uncharge a page previously charged with mem_cgroup_charge().
+ * Uncharge a folio previously charged with folio_charge_cgroup().
  */
-void mem_cgroup_uncharge(struct page *page)
+void folio_uncharge_cgroup(struct folio *folio)
 {
 	struct uncharge_gather ug;
 
 	if (mem_cgroup_disabled())
 		return;
 
-	/* Don't touch page->lru of any random page, pre-check: */
-	if (!page_memcg(page))
+	/* Don't touch folio->lru of any random page, pre-check: */
+	if (!folio_memcg(folio))
 		return;
 
 	uncharge_gather_clear(&ug);
-	uncharge_page(page, &ug);
+	uncharge_page(&folio->page, &ug);
 	uncharge_batch(&ug);
 }