Re: [PATCH v10 5/8] mm/demotion: Build demotion targets based on explicit memory tiers

From: "Huang, Ying" <ying.huang@intel.com>
To: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
Cc: linux-mm@kvack.org,  akpm@linux-foundation.org,
	 Wei Xu <weixugc@google.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>,
	 Alistair Popple <apopple@nvidia.com>,
	 Dan Williams <dan.j.williams@intel.com>,
	 Johannes Weiner <hannes@cmpxchg.org>,
	 jvgediya.oss@gmail.com
Subject: Re: [PATCH v10 5/8] mm/demotion: Build demotion targets based on explicit memory tiers
Date: Thu, 28 Jul 2022 14:51:34 +0800	[thread overview]
Message-ID: <87y1wdn30p.fsf@yhuang6-desk2.ccr.corp.intel.com> (raw)
In-Reply-To: <87k07zrx3t.fsf@linux.ibm.com> (Aneesh Kumar K. V.'s message of "Wed, 27 Jul 2022 10:05:50 +0530")

"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:

> "Huang, Ying" <ying.huang@intel.com> writes:
>
>> Aneesh Kumar K V <aneesh.kumar@linux.ibm.com> writes:
>>
>>> On 7/26/22 1:14 PM, Huang, Ying wrote:
>>>> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>>>> 
>
> ....
>
>>>> + */
>>>>> +int next_demotion_node(int node)
>>>>> +{
>>>>> +	struct demotion_nodes *nd;
>>>>> +	int target;
>>>>> +
>>>>> +	if (!node_demotion)
>>>>> +		return NUMA_NO_NODE;
>>>>> +
>>>>> +	nd = &node_demotion[node];
>>>>> +
>>>>> +	/*
>>>>> +	 * node_demotion[] is updated without excluding this
>>>>> +	 * function from running.
>>>>> +	 *
>>>>> +	 * Make sure to use RCU over entire code blocks if
>>>>> +	 * node_demotion[] reads need to be consistent.
>>>>> +	 */
>>>>> +	rcu_read_lock();
>>>>> +	/*
>>>>> +	 * If there are multiple target nodes, just select one
>>>>> +	 * target node randomly.
>>>>> +	 *
>>>>> +	 * In addition, we can also use round-robin to select
>>>>> +	 * target node, but we should introduce another variable
>>>>> +	 * for node_demotion[] to record last selected target node,
>>>>> +	 * that may cause cache ping-pong due to the changing of
>>>>> +	 * last target node. Or introducing per-cpu data to avoid
>>>>> +	 * caching issue, which seems more complicated. So selecting
>>>>> +	 * target node randomly seems better until now.
>>>>> +	 */
>>>>> +	target = node_random(&nd->preferred);
>>>> 
>>>> In one of the most common cases, nodes_weight(&nd->preferred) == 1.
>>>> Where, get_random_int() in node_random() just wastes CPU cycles and
>>>> random entropy.  So the original struct demotion_nodes implementation
>>>> appears better.
>>>> 
>>>>   struct demotion_nodes {
>>>>          unsigned short nr;
>>>>          short nodes[DEMOTION_TARGET_NODES];
>>>>   };
>>>> 
>>>
>>>
>>> Is that measurable difference? using nodemask_t makes it much easier with respect to
>>> implementation. IMHO if we observe the usage of node_random() to have performance impact
>>> with nodes_weight() == 1 we should fix node_random() to handle that? If you strongly
>>> feel we should fix this, i can opencode node_random to special case node_weight() == 1?
>>
>> If there's no much difference, why not just use the existing code?
>> IMHO, it's your responsibility to prove your new implementation is
>> better via numbers, for example, reduced code lines, with better or same
>> performance.
>>
>> Another policy is just to use the existing code in the first version.
>> Then change it based on measurement.
>
> One of the reason I switched to nodemask_t is to make code simpler.
> demotion target is essentially a node mask. 
>
>>
>> In general, I care more about the most common cases, that is, 0 or 1
>> demotion target.
>
> How about I switch to the below opencoded version. That should take care
> of the above concern. 

Per my estimation, the performance for 0 or 1 demotion target should be
OK.

And I think that you can change node_random() implementation directly.
Because it will not hurt other users too.

Best Regards,
Huang, Ying

>>
>>> -	target = node_random(&nd->preferred);
>>> +	node_weight = nodes_weight(nd->preferred);
>>> +	switch (node_weight) {
>>> +	case 0:
>>> +		target = NUMA_NO_NODE;
>>> +		break;
>>> +	case 1:
>>> +		target = first_node(nd->preferred);
>>> +		break;
>>> +	default:
>>> +		target = bitmap_ord_to_pos(nd->preferred.bits,
>>> +					   get_random_int() % node_weight, MAX_NUMNODES);
>>> +		break;
>>> +	}
>>>  
>>>