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

[Matthew Wilcox]

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@xxxxxxxxxxxxx>
> 
> 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