Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <a7d3829e-8bc5-d7a8-5e9e-a7943bb50740@linux.ibm.com>
Date:   Mon, 6 Jun 2022 09:37:26 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [RFC PATCH v4 7/7] mm/demotion: Demote pages according to
 allocation fallback order
Content-Language: en-US
To:     Ying Huang <ying.huang@intel.com>, linux-mm@kvack.org,
        akpm@linux-foundation.org
Cc:     Greg Thelen <gthelen@google.com>, Yang Shi <shy828301@gmail.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Tim C Chen <tim.c.chen@intel.com>,
        Brice Goglin <brice.goglin@gmail.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>,
        Alistair Popple <apopple@nvidia.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Feng Tang <feng.tang@intel.com>,
        Jagdish Gediya <jvgediya@linux.ibm.com>,
        Baolin Wang <baolin.wang@linux.alibaba.com>,
        David Rientjes <rientjes@google.com>
References: <CAAPL-u-dFp7PwPH6DfbYdnY8xaGsHz3tRQ0CPGVkiqURvdN8=A@mail.gmail.com>
 <20220527122528.129445-1-aneesh.kumar@linux.ibm.com>
 <20220527122528.129445-8-aneesh.kumar@linux.ibm.com>
 <b102d5773bffd6391283773044f756e810c1f044.camel@intel.com>
 <046c373a-f30b-091d-47a1-e28bfb7e9394@linux.ibm.com>
 <9f6e60cc8be3cbde4871458c612c5c31d2a9e056.camel@intel.com>
From:   Aneesh Kumar K V <aneesh.kumar@linux.ibm.com>
In-Reply-To: <9f6e60cc8be3cbde4871458c612c5c31d2a9e056.camel@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
Precedence: bulk

On 6/6/22 6:13 AM, Ying Huang wrote:
> On Fri, 2022-06-03 at 20:39 +0530, Aneesh Kumar K V wrote:
>> On 6/2/22 1:05 PM, Ying Huang wrote:
>>> On Fri, 2022-05-27 at 17:55 +0530, Aneesh Kumar K.V wrote:
>>>> From: Jagdish Gediya <jvgediya@linux.ibm.com>
>>>>
>>>> currently, a higher tier node can only be demoted to selected
>>>> nodes on the next lower tier as defined by the demotion path,
>>>> not any other node from any lower tier.  This strict, hard-coded
>>>> demotion order does not work in all use cases (e.g. some use cases
>>>> may want to allow cross-socket demotion to another node in the same
>>>> demotion tier as a fallback when the preferred demotion node is out
>>>> of space). This demotion order is also inconsistent with the page
>>>> allocation fallback order when all the nodes in a higher tier are
>>>> out of space: The page allocation can fall back to any node from any
>>>> lower tier, whereas the demotion order doesn't allow that currently.
>>>>
>>>> This patch adds support to get all the allowed demotion targets mask
>>>> for node, also demote_page_list() function is modified to utilize this
>>>> allowed node mask by filling it in migration_target_control structure
>>>> before passing it to migrate_pages().
>>>
>>
>> ...
>>
>>>>     * Take pages on @demote_list and attempt to demote them to
>>>>     * another node.  Pages which are not demoted are left on
>>>> @@ -1481,6 +1464,19 @@ static unsigned int demote_page_list(struct list_head *demote_pages,
>>>>    {
>>>>    	int target_nid = next_demotion_node(pgdat->node_id);
>>>>    	unsigned int nr_succeeded;
>>>> +	nodemask_t allowed_mask;
>>>> +
>>>> +	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_NOWARN |
>>>> +			__GFP_NOMEMALLOC | GFP_NOWAIT,
>>>> +		.nid = target_nid,
>>>> +		.nmask = &allowed_mask
>>>> +	};
>>>
>>> IMHO, we should try to allocate from preferred node firstly (which will
>>> kick kswapd of the preferred node if necessary).  If failed, we will
>>> fallback to all allowed node.
>>>
>>> As we discussed as follows,
>>>
>>> https://lore.kernel.org/lkml/69f2d063a15f8c4afb4688af7b7890f32af55391.camel@intel.com/
>>>
>>> That is, something like below,
>>>
>>> static struct page *alloc_demote_page(struct page *page, unsigned long node)
>>> {
>>> 	struct page *page;
>>> 	nodemask_t allowed_mask;
>>> 	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
>>> 	};
>>>
>>> 	page = alloc_migration_target(page, (unsigned long)&mtc);
>>> 	if (page)
>>> 		return page;
>>>
>>> 	mtc.gfp_mask &= ~__GFP_THISNODE;
>>> 	mtc.nmask = &allowed_mask;
>>>
>>> 	return alloc_migration_target(page, (unsigned long)&mtc);
>>> }
>>
>> I skipped doing this in v5 because I was not sure this is really what we
>> want.
> 
> I think so.  And this is the original behavior.  We should keep the
> original behavior as much as possible, then make changes if necessary.
> 

That is the reason I split the new page allocation as a separate patch. 
Previous discussion on this topic didn't conclude on whether we really 
need to do the above or not
https://lore.kernel.org/lkml/CAAPL-u9endrWf_aOnPENDPdvT-2-YhCAeJ7ONGckGnXErTLOfQ@mail.gmail.com/

Based on the above I looked at avoiding GFP_THISNODE allocation. If you 
have experiment results that suggest otherwise can you share? I could 
summarize that in the commit message for better description of why 
GFP_THISNODE enforcing is needed.

>> I guess we can do this as part of the change that is going to
>> introduce the usage of memory policy for the allocation?
> 
> Like the memory allocation policy, the default policy should be local
> preferred.  We shouldn't force users to use explicit memory policy for
> that.
> 
> And the added code isn't complex.
> 

-aneesh