Received-SPF: pass (google.com: domain of linux-kernel+bounces-15090-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
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-doc@vger.kernel.org>,  <linux-fsdevel@vger.kernel.org>,
  <linux-kernel@vger.kernel.org>,  <linux-api@vger.kernel.org>,
  <x86@kernel.org>,  <akpm@linux-foundation.org>,  <arnd@arndb.de>,
  <tglx@linutronix.de>,  <luto@kernel.org>,  <mingo@redhat.com>,
  <bp@alien8.de>,  <dave.hansen@linux.intel.com>,  <hpa@zytor.com>,
  <mhocko@kernel.org>,  <tj@kernel.org>,  <corbet@lwn.net>,
  <rakie.kim@sk.com>,  <hyeongtak.ji@sk.com>,  <honggyu.kim@sk.com>,
  <vtavarespetr@micron.com>,  <peterz@infradead.org>,
  <jgroves@micron.com>,  <ravis.opensrc@micron.com>,
  <sthanneeru@micron.com>,  <emirakhur@micron.com>,  <Hasan.Maruf@amd.com>,
  <seungjun.ha@samsung.com>
Subject: Re: [PATCH v5 01/11] mm/mempolicy: implement the sysfs-based
 weighted_interleave interface
In-Reply-To: <ZZRn04IiZhet8peu@memverge.com> (Gregory Price's message of "Tue,
	2 Jan 2024 14:45:23 -0500")
References: <20231223181101.1954-1-gregory.price@memverge.com>
	<20231223181101.1954-2-gregory.price@memverge.com>
	<877cl0f8oo.fsf@yhuang6-desk2.ccr.corp.intel.com>
	<ZYp3JbcCPQc4fUrB@memverge.com>
	<87h6jwdvxn.fsf@yhuang6-desk2.ccr.corp.intel.com>
	<ZZRn04IiZhet8peu@memverge.com>
Date: Wed, 03 Jan 2024 10:45:53 +0800
Message-ID: <87wmsrcexq.fsf@yhuang6-desk2.ccr.corp.intel.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=ascii

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

> On Tue, Jan 02, 2024 at 03:41:08PM +0800, Huang, Ying wrote:
>> Gregory Price <gregory.price@memverge.com> writes:
>> 
>> That is, if we use "1" as default weight, we need to change weights of
>> nodes frequently because we haven't a "base" weight.  The best candidate
>> base weight is the weight of DRAM node.  For example, if we set the
>> default weight of DRAM node to be "16" and use that as the base weight,
>> we don't need to change it in most cases.  The weight of other nodes can
>> be set according to the ratio of its memory bandwidth to that of DRAM.
>> 
>> This makes it easy to set the default weight via HMAT/CDAT too.
>> 
>> What do you think about that?
>> 
>
> You're getting a bit ahead of the patch set.  There is "what is a
> reasonable default weight" and "what is the minumum functionality".

I totally agree that we need the minimal functionality firstly.

> The minimum functionality is everything receiving a default weight of 1,
> such that weighted interleave's behavior defaults to round-robin
> interleave. This gets the system off the ground.

I don't think that we need to implement all functionalities now.  But,
we may need to consider more especially if it may impact the user space
interface.  The default base weight is something like that.  If we
change the default base weight from "1" to "16" later, users may be
surprised.  So, I think it's better to discuss it now.

> We can then expose an internal interface to drivers for them to set the
> default weight to some reasonable number during system and device
> initialization. The question at that point is what system is responsible
> for setting the default weights... node? cxl? anything? What happens on
> hotplug? etc.  That seems outside the scope of this patch set.
>
>
> If you want me to add the default_iw_table with special value 0 denoting
> "use default" at each layer, I can do that.
>
> The basic change is this snippet:
> ```
> if (pol->flags & MPOL_F_GWEIGHT)
> 	pol_weights = iw_table;
> else
> 	pol_weights = pol->wil.weights;
>
> for_each_node_mask(nid, nodemask) {
> 	weight = pol_weights[nid];
> 	weight_total += weight;
> 	weights[nid] = weight;
> }
> ```
>
> changes to:
> ```
> for_each_node_mask(nid, nodemask) {
> 	weight = pol->wil.weights[node]
> 	if (!weight)
> 		weight = iw_table[node]
> 	if (!weight)
> 		weight = default_iw_table[node]
> 	weight_total += weight;
> 	weights[nid] = weight
> }
> ```
>
> It's a bit ugly,

We can use a wrapper function to hide the logic.

> but it allows a 0 value to represent "use default",
> and default_iw_table just ends up being initialized to `1` for now.

Because the contents of default_iw_table are just "default weight" for
now.  We don't need it for now.  We can add it later.

> I think it also allows MPOL_F_GWEIGHT to be eliminated.

Do we need a way to distinguish whether to copy the global weights to
local weights when the memory policy is created?  That is, when the
global weights are changed later, will the changes be used?  One
possible solution is

- If no weights are specified in set_mempolicy2(), the global weights
  will be used always.

- If at least one weight is specified in set_mempolicy2(), it will be
  used, and the other weights in global weights will be copied to the
  local weights.  That is, changes to the global weights will not be
  used.

--
Best Regards,
Huang, Ying