Re: [PATCH v2 3/3] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving

From: "Huang, Ying" <ying.huang@intel.com>
To: Gregory Price <gregory.price@memverge.com>
Cc: Gregory Price <gourry.memverge@gmail.com>,  <linux-mm@kvack.org>,
	<linux-kernel@vger.kernel.org>,  <linux-doc@vger.kernel.org>,
	<linux-fsdevel@vger.kernel.org>,  <linux-api@vger.kernel.org>,
	<corbet@lwn.net>,  <akpm@linux-foundation.org>,
	 <honggyu.kim@sk.com>, <rakie.kim@sk.com>,  <hyeongtak.ji@sk.com>,
	 <mhocko@kernel.org>, <vtavarespetr@micron.com>,
	 <jgroves@micron.com>, <ravis.opensrc@micron.com>,
	 <sthanneeru@micron.com>, <emirakhur@micron.com>,
	 <Hasan.Maruf@amd.com>, <seungjun.ha@samsung.com>,
	 <hannes@cmpxchg.org>, <dan.j.williams@intel.com>,
	 Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>
Subject: Re: [PATCH v2 3/3] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving
Date: Tue, 23 Jan 2024 16:13:43 +0800	[thread overview]
Message-ID: <87ede8v554.fsf@yhuang6-desk2.ccr.corp.intel.com> (raw)
In-Reply-To: <Za9GiqsZtcfKXc5m@memverge.com> (Gregory Price's message of "Mon, 22 Jan 2024 23:54:34 -0500")

Gregory Price <gregory.price@memverge.com> writes:

> On Tue, Jan 23, 2024 at 11:02:03AM +0800, Huang, Ying wrote:
>> Gregory Price <gourry.memverge@gmail.com> writes:
>> 
>> > +	int prev_node = NUMA_NO_NODE;
>> 
>> It appears that we should initialize prev_node with me->il_prev?
>> Details are as below.
>> 
>
> yeah good catch, was a rebase error from my tested code, where this is
> the case.  patching now.
>
>> > +		if (rem_pages <= pol->wil.cur_weight) {
>> > +			pol->wil.cur_weight -= rem_pages;
>> 
>> If "pol->wil.cur_weight == 0" here, we need to change me->il_prev?
>> 
> you are right, and also need to fetch the next cur_weight.  Seems I
> missed this specific case in my tests.  (had this tested with a single
> node but not 2, so it looked right).
>
> Added to my test suite.
>
>> We can replace "weight_nodes" with "i" and use a "for" loop?
>> 
>> > +	while (weight_nodes < nnodes) {
>> > +		node = next_node_in(prev_node, nodes);
>> 
>> IIUC, "node" will not change in the loop, so all "weight" below will be
>> the same value.  To keep it simple, I think we can just copy weights
>> from the global iw_table and consider the default value?
>> 
>
> another rebase error here from my tested code, this should have been
> node = prev_node;
> while (...)
>     node = next_node_in(node, nodes);
>
> I can change it to a for loop as suggested, but for more info on why I
> did it this way, see the chunk below
>
>> > +		} else if (!delta_depleted) {
>> > +			/* if there was no delta, track last allocated node */
>> > +			resume_node = node;
>> > +			resume_weight = i < (nnodes - 1) ? weights[i+1] :
>> > +							   weights[0];
>                         ^ this line acquires the weight of the *NEXT* node
> 			  another chunk prior to this does the same
> 			  thing.  I suppose i can use next_node_in()
> 			  instead and just copy the entire weigh array
> 			  though, if that is preferable.

Yes.  I think copy the entire weight array make code logic simpler.

--
Best Regards,
Huang, Ying