[PATCH] mm/memcontrol: avoid unnecessary PageTransHuge() when counting compound page

[PATCH] mm/memcontrol: avoid unnecessary PageTransHuge() when counting compound page

[Yafang Shao]

If CONFIG_TRANSPARENT_HUGEPAGE is not set, hpage_nr_pages() is always 1;
if CONFIG_TRANSPARENT_HUGEPAGE is set, hpage_nr_pages() will
call PageTransHuge() to judge whether the page is compound page or not.
So we can use the result of hpage_nr_pages() to avoid uneccessary
PageTransHuge().

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 mm/memcontrol.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 2535e54..65c6f7c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -6306,7 +6306,6 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage)
 {
 	struct mem_cgroup *memcg;
 	unsigned int nr_pages;
-	bool compound;
 	unsigned long flags;
 
 	VM_BUG_ON_PAGE(!PageLocked(oldpage), oldpage);
@@ -6328,8 +6327,7 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage)
 		return;
 
 	/* Force-charge the new page. The old one will be freed soon */
-	compound = PageTransHuge(newpage);
-	nr_pages = compound ? hpage_nr_pages(newpage) : 1;
+	nr_pages = hpage_nr_pages(newpage);
 
 	page_counter_charge(&memcg->memory, nr_pages);
 	if (do_memsw_account())
@@ -6339,7 +6337,7 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage)
 	commit_charge(newpage, memcg, false);
 
 	local_irq_save(flags);
-	mem_cgroup_charge_statistics(memcg, newpage, compound, nr_pages);
+	mem_cgroup_charge_statistics(memcg, newpage, nr_pages > 1, nr_pages);
 	memcg_check_events(memcg, newpage);
 	local_irq_restore(flags);
 }
@@ -6533,6 +6531,7 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
 	struct mem_cgroup *memcg, *swap_memcg;
 	unsigned int nr_entries;
 	unsigned short oldid;
+	bool compound;
 
 	VM_BUG_ON_PAGE(PageLRU(page), page);
 	VM_BUG_ON_PAGE(page_count(page), page);
@@ -6553,8 +6552,9 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
 	 */
 	swap_memcg = mem_cgroup_id_get_online(memcg);
 	nr_entries = hpage_nr_pages(page);
+	compound = nr_entries > 1;
 	/* Get references for the tail pages, too */
-	if (nr_entries > 1)
+	if (compound)
 		mem_cgroup_id_get_many(swap_memcg, nr_entries - 1);
 	oldid = swap_cgroup_record(entry, mem_cgroup_id(swap_memcg),
 				   nr_entries);
@@ -6579,8 +6579,7 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
 	 * only synchronisation we have for updating the per-CPU variables.
 	 */
 	VM_BUG_ON(!irqs_disabled());
-	mem_cgroup_charge_statistics(memcg, page, PageTransHuge(page),
-				     -nr_entries);
+	mem_cgroup_charge_statistics(memcg, page, compound, -nr_entries);
 	memcg_check_events(memcg, page);
 
 	if (!mem_cgroup_is_root(memcg))
-- 
1.8.3.1

Re: [PATCH] mm/memcontrol: avoid unnecessary PageTransHuge() when counting compound page

[Michal Hocko]

On Sun 05-05-19 14:40:57, Yafang Shao wrote:
> If CONFIG_TRANSPARENT_HUGEPAGE is not set, hpage_nr_pages() is always 1;
> if CONFIG_TRANSPARENT_HUGEPAGE is set, hpage_nr_pages() will
> call PageTransHuge() to judge whether the page is compound page or not.
> So we can use the result of hpage_nr_pages() to avoid uneccessary
> PageTransHuge().

The changelog doesn't describe motivation. Does this result in a better
code/performance?
 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  mm/memcontrol.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 2535e54..65c6f7c 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -6306,7 +6306,6 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage)
>  {
>  	struct mem_cgroup *memcg;
>  	unsigned int nr_pages;
> -	bool compound;
>  	unsigned long flags;
>  
>  	VM_BUG_ON_PAGE(!PageLocked(oldpage), oldpage);
> @@ -6328,8 +6327,7 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage)
>  		return;
>  
>  	/* Force-charge the new page. The old one will be freed soon */
> -	compound = PageTransHuge(newpage);
> -	nr_pages = compound ? hpage_nr_pages(newpage) : 1;
> +	nr_pages = hpage_nr_pages(newpage);
>  
>  	page_counter_charge(&memcg->memory, nr_pages);
>  	if (do_memsw_account())
> @@ -6339,7 +6337,7 @@ void mem_cgroup_migrate(struct page *oldpage, struct page *newpage)
>  	commit_charge(newpage, memcg, false);
>  
>  	local_irq_save(flags);
> -	mem_cgroup_charge_statistics(memcg, newpage, compound, nr_pages);
> +	mem_cgroup_charge_statistics(memcg, newpage, nr_pages > 1, nr_pages);
>  	memcg_check_events(memcg, newpage);
>  	local_irq_restore(flags);
>  }
> @@ -6533,6 +6531,7 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
>  	struct mem_cgroup *memcg, *swap_memcg;
>  	unsigned int nr_entries;
>  	unsigned short oldid;
> +	bool compound;
>  
>  	VM_BUG_ON_PAGE(PageLRU(page), page);
>  	VM_BUG_ON_PAGE(page_count(page), page);
> @@ -6553,8 +6552,9 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
>  	 */
>  	swap_memcg = mem_cgroup_id_get_online(memcg);
>  	nr_entries = hpage_nr_pages(page);
> +	compound = nr_entries > 1;
>  	/* Get references for the tail pages, too */
> -	if (nr_entries > 1)
> +	if (compound)
>  		mem_cgroup_id_get_many(swap_memcg, nr_entries - 1);
>  	oldid = swap_cgroup_record(entry, mem_cgroup_id(swap_memcg),
>  				   nr_entries);
> @@ -6579,8 +6579,7 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
>  	 * only synchronisation we have for updating the per-CPU variables.
>  	 */
>  	VM_BUG_ON(!irqs_disabled());
> -	mem_cgroup_charge_statistics(memcg, page, PageTransHuge(page),
> -				     -nr_entries);
> +	mem_cgroup_charge_statistics(memcg, page, compound, -nr_entries);
>  	memcg_check_events(memcg, page);
>  
>  	if (!mem_cgroup_is_root(memcg))
> -- 
> 1.8.3.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/memcontrol: avoid unnecessary PageTransHuge() when counting compound page

[Yafang Shao]

On Mon, May 6, 2019 at 9:59 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Sun 05-05-19 14:40:57, Yafang Shao wrote:
> > If CONFIG_TRANSPARENT_HUGEPAGE is not set, hpage_nr_pages() is always 1;
> > if CONFIG_TRANSPARENT_HUGEPAGE is set, hpage_nr_pages() will
> > call PageTransHuge() to judge whether the page is compound page or not.
> > So we can use the result of hpage_nr_pages() to avoid uneccessary
> > PageTransHuge().
>
> The changelog doesn't describe motivation. Does this result in a better
> code/performance?
>

It is a better code, I think.
Regarding the performance, I don't think it is easy to measure.

Thanks
Yafang

Re: [PATCH] mm/memcontrol: avoid unnecessary PageTransHuge() when counting compound page

[Michal Hocko]

On Mon 06-05-19 23:22:11, Yafang Shao wrote:
> On Mon, May 6, 2019 at 9:59 PM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Sun 05-05-19 14:40:57, Yafang Shao wrote:
> > > If CONFIG_TRANSPARENT_HUGEPAGE is not set, hpage_nr_pages() is always 1;
> > > if CONFIG_TRANSPARENT_HUGEPAGE is set, hpage_nr_pages() will
> > > call PageTransHuge() to judge whether the page is compound page or not.
> > > So we can use the result of hpage_nr_pages() to avoid uneccessary
> > > PageTransHuge().
> >
> > The changelog doesn't describe motivation. Does this result in a better
> > code/performance?
> >
> 
> It is a better code, I think.
> Regarding the performance, I don't think it is easy to measure.

I am not convinced the patch is worth it. The code aesthetic is a matter
of taste. On the other hand, the change will be an additional step in
the git history so git blame take an additional step to get to the
original commit which is a bit annoying. Also every change, even a
trivially looking one, can cause surprising side effects. These are all
arguments make a change to the code.

So unless the resulting code is really much more cleaner, easier to read
or maintain, or it is a part of a larger series that makes further steps
easier,then I would prefer not touching the code.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/memcontrol: avoid unnecessary PageTransHuge() when counting compound page

[Chris Down]

Michal Hocko writes:
>On Mon 06-05-19 23:22:11, Yafang Shao wrote:
>> It is a better code, I think.
>> Regarding the performance, I don't think it is easy to measure.
>
>I am not convinced the patch is worth it. The code aesthetic is a matter
>of taste. On the other hand, the change will be an additional step in
>the git history so git blame take an additional step to get to the
>original commit which is a bit annoying. Also every change, even a
>trivially looking one, can cause surprising side effects. These are all
>arguments make a change to the code.
>
>So unless the resulting code is really much more cleaner, easier to read
>or maintain, or it is a part of a larger series that makes further steps
>easier,then I would prefer not touching the code.

Aside from what Michal already said, which I agree with, when skimming code 
reading PageTransHuge has much clearer intent to me than checking nr_pages. We 
already have a non-trivial number of checks which are unclear at first glance 
in the mm code and, while this isn't nearly as bad as some of those, and might 
not make the situation much worse, I also don't think changing to nr_pages 
checks makes the situation any better, either.

Re: [PATCH] mm/memcontrol: avoid unnecessary PageTransHuge() when counting compound page

[Yafang Shao]

On Tue, May 7, 2019 at 10:21 PM Chris Down <chris@chrisdown.name> wrote:
>
> Michal Hocko writes:
> >On Mon 06-05-19 23:22:11, Yafang Shao wrote:
> >> It is a better code, I think.
> >> Regarding the performance, I don't think it is easy to measure.
> >
> >I am not convinced the patch is worth it. The code aesthetic is a matter
> >of taste. On the other hand, the change will be an additional step in
> >the git history so git blame take an additional step to get to the
> >original commit which is a bit annoying. Also every change, even a
> >trivially looking one, can cause surprising side effects. These are all
> >arguments make a change to the code.
> >
> >So unless the resulting code is really much more cleaner, easier to read
> >or maintain, or it is a part of a larger series that makes further steps
> >easier,then I would prefer not touching the code.
>
> Aside from what Michal already said, which I agree with, when skimming code
> reading PageTransHuge has much clearer intent to me than checking nr_pages. We
> already have a non-trivial number of checks which are unclear at first glance
> in the mm code and, while this isn't nearly as bad as some of those, and might
> not make the situation much worse, I also don't think changing to nr_pages
> checks makes the situation any better, either.

I agree with dropping this patch, but I don't agree with your opinion
that PageTransHuge() can make the code clear.

The motivation I send this patch is because 'compound' and
'PageTransHuge' confused me.
I prefer to remove the paratmeter 'compound' compeletely from some
functions(i.e. mem_cgroup_commit_charge,  mem_cgroup_migrate,
mem_cgroup_swapout,  mem_cgroup_move_account) in memcontrol.c
completely,
because whether this page is compound or not doesn't depends on the
callsite, but only depends on the page itself.
I mean we can use the page to judge whether the page is compound or not.
I didn't do that because I'm not sure whether it is worth.
The other reason comfused me is compound page may not be thp. Of
course it can only be thp in the current callsites.
Maybe 'thp' is better than 'compound'.

Thanks
Yafang