Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 2 Sep 2022 20:08:52 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Andrew Morton <akpm@linux-foundation.org>
Cc:     "zhaoyang.huang" <zhaoyang.huang@unisoc.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Zhaoyang Huang <huangzhaoyang@gmail.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, ke.wang@unisoc.com
Subject: Re: [Resend RFC PATCH] mm: introduce __GFP_TRACKLEAK to track
 in-kernel allocation
Message-ID: <YxJUxDn5v2MqmjKM@casper.infradead.org>
References: <1662116347-17649-1-git-send-email-zhaoyang.huang@unisoc.com>
 <20220902115839.1e3fafd159e42d4e7dae90af@linux-foundation.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220902115839.1e3fafd159e42d4e7dae90af@linux-foundation.org>
Precedence: bulk

On Fri, Sep 02, 2022 at 11:58:39AM -0700, Andrew Morton wrote:
> Cc willy for page-flags changes.

Thanks.  This is probably OK.  The biggest problem is that it won't
work for drivers which allocate memory and then map it to userspace.
If they try, they'll get a nice splat, but it may limit the usefulness
of this option.  We should probably document that limitation in this
patch.

> On Fri, 2 Sep 2022 18:59:07 +0800 "zhaoyang.huang" <zhaoyang.huang@unisoc.com> wrote:
> > +++ b/mm/page_alloc.c
> > @@ -1361,6 +1361,8 @@ static __always_inline bool free_pages_prepare(struct page *page,
> >  		page->mapping = NULL;
> >  	if (memcg_kmem_enabled() && PageMemcgKmem(page))
> >  		__memcg_kmem_uncharge_page(page, order);
> > +	if (PageTrackleak(page))
> > +		kmemleak_free(page);

Don't we also need to __ClearPageTrackleak()?

> > +	if (gfp & __GFP_TRACKLEAK) {
> 
> And we'd want __GFP_TRACKLEAK to evaluate to zero at compile time if
> CONFIG_HAVE_DEBUG_KMEMLEAK=n.
> 
> > +		kmemleak_alloc(page_address(page), PAGE_SIZE << order, 1, gfp & ~__GFP_TRACKLEAK);
> > +		__SetPageTrackleak(page);
> > +	}

We only set this on the first page we allocate.  I think there's a
problem for multi-page, non-compound allocations, no?  Particularly
when you consider the problem fixed in e320d3012d25.

I'm not opposed to this tracking, it just needs a bit more thought and
awareness of some of the corner cases of the VM.  A few test cases would
be nice; they could demonstrate that this works for both compound and
non-compound high-order allocations.