diff mbox series

[v3,14/18] mm/memcg: Convert mem_cgroup_move_account() to use a folio

Message ID 20210630040034.1155892-15-willy@infradead.org (mailing list archive)
State New
Headers show
Series Folio conversion of memcg | expand

Commit Message

Matthew Wilcox (Oracle) June 30, 2021, 4 a.m. UTC
This saves dozens of bytes of text by eliminating a lot of calls to
compound_head().

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/memcontrol.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

Comments

Michal Hocko June 30, 2021, 8:30 a.m. UTC | #1
On Wed 30-06-21 05:00:30, Matthew Wilcox wrote:
> This saves dozens of bytes of text by eliminating a lot of calls to
> compound_head().
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  mm/memcontrol.c | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index b94a6122f27d..95795b65ae3e 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5585,38 +5585,39 @@ static int mem_cgroup_move_account(struct page *page,
>  				   struct mem_cgroup *from,
>  				   struct mem_cgroup *to)
>  {
> +	struct folio *folio = page_folio(page);
>  	struct lruvec *from_vec, *to_vec;
>  	struct pglist_data *pgdat;
> -	unsigned int nr_pages = compound ? thp_nr_pages(page) : 1;
> +	unsigned int nr_pages = compound ? folio_nr_pages(folio) : 1;
>  	int nid, ret;
>  
>  	VM_BUG_ON(from == to);
> -	VM_BUG_ON_PAGE(PageLRU(page), page);
> -	VM_BUG_ON(compound && !PageTransHuge(page));
> +	VM_BUG_ON_FOLIO(folio_lru(folio), folio);
> +	VM_BUG_ON(compound && !folio_multi(folio));
>  
>  	/*
>  	 * Prevent mem_cgroup_migrate() from looking at
>  	 * page's memory cgroup of its source page while we change it.
>  	 */
>  	ret = -EBUSY;
> -	if (!trylock_page(page))
> +	if (!folio_trylock(folio))
>  		goto out;
>  
>  	ret = -EINVAL;
> -	if (page_memcg(page) != from)
> +	if (folio_memcg(folio) != from)
>  		goto out_unlock;
>  
> -	pgdat = page_pgdat(page);
> +	pgdat = folio_pgdat(folio);
>  	from_vec = mem_cgroup_lruvec(from, pgdat);
>  	to_vec = mem_cgroup_lruvec(to, pgdat);
>  
> -	lock_page_memcg(page);
> +	folio_memcg_lock(folio);
>  
> -	if (PageAnon(page)) {
> -		if (page_mapped(page)) {
> +	if (folio_anon(folio)) {
> +		if (folio_mapped(folio)) {
>  			__mod_lruvec_state(from_vec, NR_ANON_MAPPED, -nr_pages);
>  			__mod_lruvec_state(to_vec, NR_ANON_MAPPED, nr_pages);
> -			if (PageTransHuge(page)) {
> +			if (folio_multi(folio)) {

Shouldn't be folio_transhuge? The resulting code is the same but
folio_transhuge is more explicit and matches the THP aspect.
Matthew Wilcox (Oracle) June 30, 2021, 11:22 a.m. UTC | #2
On Wed, Jun 30, 2021 at 10:30:38AM +0200, Michal Hocko wrote:
> > -	if (PageAnon(page)) {
> > -		if (page_mapped(page)) {
> > +	if (folio_anon(folio)) {
> > +		if (folio_mapped(folio)) {
> >  			__mod_lruvec_state(from_vec, NR_ANON_MAPPED, -nr_pages);
> >  			__mod_lruvec_state(to_vec, NR_ANON_MAPPED, nr_pages);
> > -			if (PageTransHuge(page)) {
> > +			if (folio_multi(folio)) {
> 
> Shouldn't be folio_transhuge? The resulting code is the same but
> folio_transhuge is more explicit and matches the THP aspect.

I genuinely don't know.  For the benefit of those reading along, the
important part of the context is:

                if (folio_mapped(folio)) {
                        __mod_lruvec_state(from_vec, NR_ANON_MAPPED, -nr_pages);
                        __mod_lruvec_state(to_vec, NR_ANON_MAPPED, nr_pages);
                        if (folio_multi(folio)) {
                                __mod_lruvec_state(from_vec, NR_ANON_THPS,
                                                   -nr_pages);
                                __mod_lruvec_state(to_vec, NR_ANON_THPS,
                                                   nr_pages);
                        }
                }

We need to decide what 'NR_ANON_THPS' means in a folio-based world where
we have folios of all orders.  Does it count only the number of pages
in folios >= HPAGE_PMD_SIZE?  Or does it count the number of pages in
folios > PAGE_SIZE?

Similar question (and I suspect the same answer) for NR_SHMEM_THPS and
NR_FILE_THPS.  Right now, I've been accounting any multi-page folio as
a THP, but I don't have a good sense of what the answer should be.
Michal Hocko June 30, 2021, 12:20 p.m. UTC | #3
On Wed 30-06-21 12:22:48, Matthew Wilcox wrote:
> On Wed, Jun 30, 2021 at 10:30:38AM +0200, Michal Hocko wrote:
> > > -	if (PageAnon(page)) {
> > > -		if (page_mapped(page)) {
> > > +	if (folio_anon(folio)) {
> > > +		if (folio_mapped(folio)) {
> > >  			__mod_lruvec_state(from_vec, NR_ANON_MAPPED, -nr_pages);
> > >  			__mod_lruvec_state(to_vec, NR_ANON_MAPPED, nr_pages);
> > > -			if (PageTransHuge(page)) {
> > > +			if (folio_multi(folio)) {
> > 
> > Shouldn't be folio_transhuge? The resulting code is the same but
> > folio_transhuge is more explicit and matches the THP aspect.
> 
> I genuinely don't know.  For the benefit of those reading along, the
> important part of the context is:
> 
>                 if (folio_mapped(folio)) {
>                         __mod_lruvec_state(from_vec, NR_ANON_MAPPED, -nr_pages);
>                         __mod_lruvec_state(to_vec, NR_ANON_MAPPED, nr_pages);
>                         if (folio_multi(folio)) {
>                                 __mod_lruvec_state(from_vec, NR_ANON_THPS,
>                                                    -nr_pages);
>                                 __mod_lruvec_state(to_vec, NR_ANON_THPS,
>                                                    nr_pages);
>                         }
>                 }
> 
> We need to decide what 'NR_ANON_THPS' means in a folio-based world where
> we have folios of all orders.  Does it count only the number of pages
> in folios >= HPAGE_PMD_SIZE?  Or does it count the number of pages in
> folios > PAGE_SIZE?

At this stage we only have PMD based, right? I believe it would be
simpler to stick with that at the moment and change that to a more
generic way along with other places which need updating.

Wrt. counters they do count pages so in this case this shouldn't be a
problem. But we do have counters for pmd mappings and that might need
some care.
Matthew Wilcox (Oracle) June 30, 2021, 12:31 p.m. UTC | #4
On Wed, Jun 30, 2021 at 02:20:38PM +0200, Michal Hocko wrote:
> On Wed 30-06-21 12:22:48, Matthew Wilcox wrote:
> > We need to decide what 'NR_ANON_THPS' means in a folio-based world where
> > we have folios of all orders.  Does it count only the number of pages
> > in folios >= HPAGE_PMD_SIZE?  Or does it count the number of pages in
> > folios > PAGE_SIZE?
> 
> At this stage we only have PMD based, right? I believe it would be
> simpler to stick with that at the moment and change that to a more
> generic way along with other places which need updating.
> 
> Wrt. counters they do count pages so in this case this shouldn't be a
> problem. But we do have counters for pmd mappings and that might need
> some care.

Looking at how these are reported:

        show_val_kb(m, "AnonHugePages:  ",
                    global_node_page_state(NR_ANON_THPS));
        show_val_kb(m, "ShmemHugePages: ",
                    global_node_page_state(NR_SHMEM_THPS));
        show_val_kb(m, "ShmemPmdMapped: ",
                    global_node_page_state(NR_SHMEM_PMDMAPPED));
        show_val_kb(m, "FileHugePages:  ",
                    global_node_page_state(NR_FILE_THPS));
        show_val_kb(m, "FilePmdMapped:  ",
                    global_node_page_state(NR_FILE_PMDMAPPED));

it specifically refers to 'HugePages', so I think we need to only
count folios with order >= PMD_ORDER.  I'll make that change to
folio_transhuge() and use folio_transhuge() here.
Michal Hocko June 30, 2021, 12:45 p.m. UTC | #5
On Wed 30-06-21 13:31:17, Matthew Wilcox wrote:
> On Wed, Jun 30, 2021 at 02:20:38PM +0200, Michal Hocko wrote:
> > On Wed 30-06-21 12:22:48, Matthew Wilcox wrote:
> > > We need to decide what 'NR_ANON_THPS' means in a folio-based world where
> > > we have folios of all orders.  Does it count only the number of pages
> > > in folios >= HPAGE_PMD_SIZE?  Or does it count the number of pages in
> > > folios > PAGE_SIZE?
> > 
> > At this stage we only have PMD based, right? I believe it would be
> > simpler to stick with that at the moment and change that to a more
> > generic way along with other places which need updating.
> > 
> > Wrt. counters they do count pages so in this case this shouldn't be a
> > problem. But we do have counters for pmd mappings and that might need
> > some care.
> 
> Looking at how these are reported:
> 
>         show_val_kb(m, "AnonHugePages:  ",
>                     global_node_page_state(NR_ANON_THPS));
>         show_val_kb(m, "ShmemHugePages: ",
>                     global_node_page_state(NR_SHMEM_THPS));
>         show_val_kb(m, "ShmemPmdMapped: ",
>                     global_node_page_state(NR_SHMEM_PMDMAPPED));
>         show_val_kb(m, "FileHugePages:  ",
>                     global_node_page_state(NR_FILE_THPS));
>         show_val_kb(m, "FilePmdMapped:  ",
>                     global_node_page_state(NR_FILE_PMDMAPPED));
> 
> it specifically refers to 'HugePages', so I think we need to only
> count folios with order >= PMD_ORDER.

Why? The presented value is in kB. It gives us a cumulative number of
transparent large pages.  Sure breakdown to respective orders would be
impossible in general but the same would be the case if order > PMD_ORDER.

I am not really sure how useful that information is in practice but that
is a different story.

> I'll make that change to
> folio_transhuge() and use folio_transhuge() here.

Thanks!
Matthew Wilcox (Oracle) July 7, 2021, 3:25 p.m. UTC | #6
On Wed, Jun 30, 2021 at 02:45:33PM +0200, Michal Hocko wrote:
> On Wed 30-06-21 13:31:17, Matthew Wilcox wrote:
> > On Wed, Jun 30, 2021 at 02:20:38PM +0200, Michal Hocko wrote:
> > > On Wed 30-06-21 12:22:48, Matthew Wilcox wrote:
> > > > We need to decide what 'NR_ANON_THPS' means in a folio-based world where
> > > > we have folios of all orders.  Does it count only the number of pages
> > > > in folios >= HPAGE_PMD_SIZE?  Or does it count the number of pages in
> > > > folios > PAGE_SIZE?
> > > 
> > > At this stage we only have PMD based, right? I believe it would be
> > > simpler to stick with that at the moment and change that to a more
> > > generic way along with other places which need updating.
> > > 
> > > Wrt. counters they do count pages so in this case this shouldn't be a
> > > problem. But we do have counters for pmd mappings and that might need
> > > some care.
> > 
> > Looking at how these are reported:
> > 
> >         show_val_kb(m, "AnonHugePages:  ",
> >                     global_node_page_state(NR_ANON_THPS));
> >         show_val_kb(m, "ShmemHugePages: ",
> >                     global_node_page_state(NR_SHMEM_THPS));
> >         show_val_kb(m, "ShmemPmdMapped: ",
> >                     global_node_page_state(NR_SHMEM_PMDMAPPED));
> >         show_val_kb(m, "FileHugePages:  ",
> >                     global_node_page_state(NR_FILE_THPS));
> >         show_val_kb(m, "FilePmdMapped:  ",
> >                     global_node_page_state(NR_FILE_PMDMAPPED));
> > 
> > it specifically refers to 'HugePages', so I think we need to only
> > count folios with order >= PMD_ORDER.
> 
> Why? The presented value is in kB. It gives us a cumulative number of
> transparent large pages.  Sure breakdown to respective orders would be
> impossible in general but the same would be the case if order > PMD_ORDER.
> 
> I am not really sure how useful that information is in practice but that
> is a different story.

The scenario I'm thinking about is a situation where we have gigabytes
of memory in the page cache in 16k-64k chunks and we'll see
FileHugePages: 5219348 kB
FilePmdMapped:       0 kB

which might cause the slightly-too-clever user to think there's a
problem.
Michal Hocko July 8, 2021, 7:30 a.m. UTC | #7
On Wed 07-07-21 16:25:07, Matthew Wilcox wrote:
> On Wed, Jun 30, 2021 at 02:45:33PM +0200, Michal Hocko wrote:
> > On Wed 30-06-21 13:31:17, Matthew Wilcox wrote:
> > > On Wed, Jun 30, 2021 at 02:20:38PM +0200, Michal Hocko wrote:
> > > > On Wed 30-06-21 12:22:48, Matthew Wilcox wrote:
> > > > > We need to decide what 'NR_ANON_THPS' means in a folio-based world where
> > > > > we have folios of all orders.  Does it count only the number of pages
> > > > > in folios >= HPAGE_PMD_SIZE?  Or does it count the number of pages in
> > > > > folios > PAGE_SIZE?
> > > > 
> > > > At this stage we only have PMD based, right? I believe it would be
> > > > simpler to stick with that at the moment and change that to a more
> > > > generic way along with other places which need updating.
> > > > 
> > > > Wrt. counters they do count pages so in this case this shouldn't be a
> > > > problem. But we do have counters for pmd mappings and that might need
> > > > some care.
> > > 
> > > Looking at how these are reported:
> > > 
> > >         show_val_kb(m, "AnonHugePages:  ",
> > >                     global_node_page_state(NR_ANON_THPS));
> > >         show_val_kb(m, "ShmemHugePages: ",
> > >                     global_node_page_state(NR_SHMEM_THPS));
> > >         show_val_kb(m, "ShmemPmdMapped: ",
> > >                     global_node_page_state(NR_SHMEM_PMDMAPPED));
> > >         show_val_kb(m, "FileHugePages:  ",
> > >                     global_node_page_state(NR_FILE_THPS));
> > >         show_val_kb(m, "FilePmdMapped:  ",
> > >                     global_node_page_state(NR_FILE_PMDMAPPED));
> > > 
> > > it specifically refers to 'HugePages', so I think we need to only
> > > count folios with order >= PMD_ORDER.
> > 
> > Why? The presented value is in kB. It gives us a cumulative number of
> > transparent large pages.  Sure breakdown to respective orders would be
> > impossible in general but the same would be the case if order > PMD_ORDER.
> > 
> > I am not really sure how useful that information is in practice but that
> > is a different story.
> 
> The scenario I'm thinking about is a situation where we have gigabytes
> of memory in the page cache in 16k-64k chunks and we'll see
> FileHugePages: 5219348 kB
> FilePmdMapped:       0 kB
> 
> which might cause the slightly-too-clever user to think there's a
> problem.

Well, cases like this one shouldn't be really hard to explain though so
I wouldn't be worried about this.
diff mbox series

Patch

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b94a6122f27d..95795b65ae3e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5585,38 +5585,39 @@  static int mem_cgroup_move_account(struct page *page,
 				   struct mem_cgroup *from,
 				   struct mem_cgroup *to)
 {
+	struct folio *folio = page_folio(page);
 	struct lruvec *from_vec, *to_vec;
 	struct pglist_data *pgdat;
-	unsigned int nr_pages = compound ? thp_nr_pages(page) : 1;
+	unsigned int nr_pages = compound ? folio_nr_pages(folio) : 1;
 	int nid, ret;
 
 	VM_BUG_ON(from == to);
-	VM_BUG_ON_PAGE(PageLRU(page), page);
-	VM_BUG_ON(compound && !PageTransHuge(page));
+	VM_BUG_ON_FOLIO(folio_lru(folio), folio);
+	VM_BUG_ON(compound && !folio_multi(folio));
 
 	/*
 	 * Prevent mem_cgroup_migrate() from looking at
 	 * page's memory cgroup of its source page while we change it.
 	 */
 	ret = -EBUSY;
-	if (!trylock_page(page))
+	if (!folio_trylock(folio))
 		goto out;
 
 	ret = -EINVAL;
-	if (page_memcg(page) != from)
+	if (folio_memcg(folio) != from)
 		goto out_unlock;
 
-	pgdat = page_pgdat(page);
+	pgdat = folio_pgdat(folio);
 	from_vec = mem_cgroup_lruvec(from, pgdat);
 	to_vec = mem_cgroup_lruvec(to, pgdat);
 
-	lock_page_memcg(page);
+	folio_memcg_lock(folio);
 
-	if (PageAnon(page)) {
-		if (page_mapped(page)) {
+	if (folio_anon(folio)) {
+		if (folio_mapped(folio)) {
 			__mod_lruvec_state(from_vec, NR_ANON_MAPPED, -nr_pages);
 			__mod_lruvec_state(to_vec, NR_ANON_MAPPED, nr_pages);
-			if (PageTransHuge(page)) {
+			if (folio_multi(folio)) {
 				__mod_lruvec_state(from_vec, NR_ANON_THPS,
 						   -nr_pages);
 				__mod_lruvec_state(to_vec, NR_ANON_THPS,
@@ -5627,18 +5628,18 @@  static int mem_cgroup_move_account(struct page *page,
 		__mod_lruvec_state(from_vec, NR_FILE_PAGES, -nr_pages);
 		__mod_lruvec_state(to_vec, NR_FILE_PAGES, nr_pages);
 
-		if (PageSwapBacked(page)) {
+		if (folio_swapbacked(folio)) {
 			__mod_lruvec_state(from_vec, NR_SHMEM, -nr_pages);
 			__mod_lruvec_state(to_vec, NR_SHMEM, nr_pages);
 		}
 
-		if (page_mapped(page)) {
+		if (folio_mapped(folio)) {
 			__mod_lruvec_state(from_vec, NR_FILE_MAPPED, -nr_pages);
 			__mod_lruvec_state(to_vec, NR_FILE_MAPPED, nr_pages);
 		}
 
-		if (PageDirty(page)) {
-			struct address_space *mapping = page_mapping(page);
+		if (folio_dirty(folio)) {
+			struct address_space *mapping = folio_mapping(folio);
 
 			if (mapping_can_writeback(mapping)) {
 				__mod_lruvec_state(from_vec, NR_FILE_DIRTY,
@@ -5649,7 +5650,7 @@  static int mem_cgroup_move_account(struct page *page,
 		}
 	}
 
-	if (PageWriteback(page)) {
+	if (folio_writeback(folio)) {
 		__mod_lruvec_state(from_vec, NR_WRITEBACK, -nr_pages);
 		__mod_lruvec_state(to_vec, NR_WRITEBACK, nr_pages);
 	}
@@ -5672,12 +5673,12 @@  static int mem_cgroup_move_account(struct page *page,
 	css_get(&to->css);
 	css_put(&from->css);
 
-	page->memcg_data = (unsigned long)to;
+	folio->memcg_data = (unsigned long)to;
 
 	__memcg_unlock(from);
 
 	ret = 0;
-	nid = page_to_nid(page);
+	nid = folio_nid(folio);
 
 	local_irq_disable();
 	mem_cgroup_charge_statistics(to, nr_pages);
@@ -5686,7 +5687,7 @@  static int mem_cgroup_move_account(struct page *page,
 	memcg_check_events(from, nid);
 	local_irq_enable();
 out_unlock:
-	unlock_page(page);
+	folio_unlock(folio);
 out:
 	return ret;
 }