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

From: Yafang Shao <laoar.shao@gmail.com>
To: Chris Down <chris@chrisdown.name>
Cc: Michal Hocko <mhocko@suse.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	 Linux MM <linux-mm@kvack.org>,
	shaoyafang@didiglobal.com
Subject: Re: [PATCH] mm/memcontrol: avoid unnecessary PageTransHuge() when counting compound page
Date: Tue, 7 May 2019 23:00:36 +0800	[thread overview]
Message-ID: <CALOAHbDGbEKxGgTEr4wJRVCp_Tw7H0ziUTsALbniPp6u1Qx2jQ@mail.gmail.com> (raw)
In-Reply-To: <20190507142148.GA55122@chrisdown.name>

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