| Message ID | 20210622121551.3398730-16-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Folio-enabling the page cache | expand |
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>
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
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
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 --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); }
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(-)