Re: [PATCH v7 09/12] mm/demotion: Demote pages according to allocation fallback order

From: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
To: Alistair Popple <apopple@nvidia.com>
Cc: linux-mm@kvack.org, akpm@linux-foundation.org,
	Wei Xu <weixugc@google.com>, Huang Ying <ying.huang@intel.com>,
	Yang Shi <shy828301@gmail.com>,
	Davidlohr Bueso <dave@stgolabs.net>,
	Tim C Chen <tim.c.chen@intel.com>,
	Michal Hocko <mhocko@kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Hesham Almatary <hesham.almatary@huawei.com>,
	Dave Hansen <dave.hansen@intel.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Jagdish Gediya <jvgediya@linux.ibm.com>
Subject: Re: [PATCH v7 09/12] mm/demotion: Demote pages according to allocation fallback order
Date: Mon, 27 Jun 2022 09:24:31 +0530	[thread overview]
Message-ID: <87czeuzrpk.fsf@linux.ibm.com> (raw)
In-Reply-To: <87czf06q1i.fsf@nvdebian.thelocal>

Alistair Popple <apopple@nvidia.com> writes:

> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>
>> From: Jagdish Gediya <jvgediya@linux.ibm.com>
>
> [...]
>
>> -static struct page *alloc_demote_page(struct page *page, unsigned long node)
>> +static struct page *alloc_demote_page(struct page *page, unsigned long private)
>>  {
>> -	struct migration_target_control mtc = {
>> -		/*
>> -		 * Allocate from 'node', or fail quickly and quietly.
>> -		 * When this happens, 'page' will likely just be discarded
>> -		 * instead of migrated.
>> -		 */
>> -		.gfp_mask = (GFP_HIGHUSER_MOVABLE & ~__GFP_RECLAIM) |
>> -			    __GFP_THISNODE  | __GFP_NOWARN |
>> -			    __GFP_NOMEMALLOC | GFP_NOWAIT,
>> -		.nid = node
>> -	};
>> +	struct page *target_page;
>> +	nodemask_t *allowed_mask;
>> +	struct migration_target_control *mtc;
>> +
>> +	mtc = (struct migration_target_control *)private;
>> +
>> +	allowed_mask = mtc->nmask;
>> +	/*
>> +	 * make sure we allocate from the target node first also trying to
>> +	 * reclaim pages from the target node via kswapd if we are low on
>> +	 * free memory on target node. If we don't do this and if we have low
>> +	 * free memory on the target memtier, we would start allocating pages
>> +	 * from higher memory tiers without even forcing a demotion of cold
>> +	 * pages from the target memtier. This can result in the kernel placing
>> +	 * hotpages in higher memory tiers.
>> +	 */
>> +	mtc->nmask = NULL;
>> +	mtc->gfp_mask |= __GFP_THISNODE;
>> +	target_page = alloc_migration_target(page, (unsigned long)&mtc);
>
> I finally managed to get a system setup to start testing some of this
> out. However it quickly crashed due to the bad pointer in the above call
> - you need mtc not &mtc here.

I remember fixing that during earlier testing. I guess I missed to copy
the change from test to my development. Thanks for testing this. I have
now also tested the complete series with the above-suggested changes and did
make sure we are indeed doing demotion by looking at
/proc/vmstat:pgdemote_kswapd/pgdemote_direct 

>
>> +	if (target_page)
>> +		return target_page;
>> +
>> +	mtc->gfp_mask &= ~__GFP_THISNODE;
>> +	mtc->nmask = allowed_mask;
>>
>>  	return alloc_migration_target(page, (unsigned long)&mtc);
>
> And here.
>

I will fold this changes in and send a v8 after waiting for review
feedback from others.

-aneesh