Re: [PATCH v2 2/2] mm/memcg: set memcg when split page

From: Matthew Wilcox <willy@infradead.org>
To: Michal Hocko <mhocko@suse.com>
Cc: Zhou Guanghui <zhouguanghui1@huawei.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	akpm@linux-foundation.org, hannes@cmpxchg.org, hughd@google.com,
	kirill.shutemov@linux.intel.com, npiggin@gmail.com,
	ziy@nvidia.com, wangkefeng.wang@huawei.com, guohanjun@huawei.com,
	dingtianhong@huawei.com, chenweilong@huawei.com,
	rui.xiang@huawei.com
Subject: Re: [PATCH v2 2/2] mm/memcg: set memcg when split page
Date: Tue, 9 Mar 2021 12:32:55 +0000	[thread overview]
Message-ID: <20210309123255.GI3479805@casper.infradead.org> (raw)
In-Reply-To: <YEc5iI+ZP7dWr2fC@dhcp22.suse.cz>

On Tue, Mar 09, 2021 at 10:02:00AM +0100, Michal Hocko wrote:
> On Mon 08-03-21 21:02:25, Matthew Wilcox wrote:
> > On Thu, Mar 04, 2021 at 07:40:53AM +0000, Zhou Guanghui wrote:
> > > For example, when alloc_pages_exact is used to allocate 1MB continuous
> > > physical memory, 2MB is charged(kmemcg is enabled and __GFP_ACCOUNT is
> > > set). When make_alloc_exact free the unused 1MB and free_pages_exact
> > > free the applied 1MB, actually, only 4KB(one page) is uncharged.

> > @@ -5081,9 +5081,15 @@ void __free_pages(struct page *page, unsigned int order)
> >  {
> >         if (put_page_testzero(page))
> >                 free_the_page(page, order);
> > -       else if (!PageHead(page))
> > -               while (order-- > 0)
> > -                       free_the_page(page + (1 << order), order);
> > +       else if (!PageHead(page)) {
> > +               while (order-- > 0) {
> > +                       struct page *tail = page + (1 << order);
> > +#ifdef CONFIG_MEMCG
> > +                       tail->memcg_data = page->memcg_data;
> > +#endif
> > +                       free_the_page(tail, order);
> > +               }
> > +       }
> >  }
> >  EXPORT_SYMBOL(__free_pages);
> 
> Hmm, I was not aware of this code. This is really a tricky code.

Yes.  I only added it recently.  I don't see a better way to solve this
problem.  We could turn the non-compound page into a compound page at
this point, but I'm not sure that's really less tricky.

> > I wonder if we shouldn't initialise memcg_data on all subsequent pages
> > of non-compound allocations instead?  Because I'm not sure this is the
> > only place that needs to be fixed.
> 
> That would be safer for sure. Do you mean this as a replacement to the
> original patch?
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 913c2b9e5c72..d44dea2b8d22 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3135,8 +3135,21 @@ int __memcg_kmem_charge_page(struct page *page, gfp_t gfp, int order)
>  	if (memcg && !mem_cgroup_is_root(memcg)) {
>  		ret = __memcg_kmem_charge(memcg, gfp, 1 << order);
>  		if (!ret) {
> +			int nr_pages = 1 << order;
>  			page->memcg_data = (unsigned long)memcg |
>  				MEMCG_DATA_KMEM;
> +			
> +			/*
> +			 * Compound pages are normally split or freed
> +			 * via their head pages so memcg_data in in the
> +			 * head page should be sufficient but there
> +			 * are exceptions to the rule (see __free_pages).
> +			 * Non compound pages would need to copy memcg anyway.
> +			 */
> +			for (i = 1; i < nr_pages; i++) {
> +				struct page * p = page + i;
> +				p->memcg_data = page->memcg_data
> +			}
>  			return 0;

I would condition this loop on if (!(gfp & __GFP_COMP)), but yes, something
along these lines.  I might phrase the comment a little differently ...

			/*
			 * Compound pages are treated as a single unit,
			 * but non-compound pages can be freed individually
			 * so each page needs to have its memcg set to get
			 * the accounting right.
			 */