Message-ID: <87h6jwdvxn.fsf@yhuang6-desk2.ccr.corp.intel.com>
Date: Tue, 02 Jan 2024 15:41:08 +0800
From: "Huang, Ying" <ying.huang@...el.com>
To: Gregory Price <gregory.price@...verge.com>
Cc: Gregory Price <gourry.memverge@...il.com>,  <linux-mm@...ck.org>,
  <linux-doc@...r.kernel.org>,  <linux-fsdevel@...r.kernel.org>,
  <linux-kernel@...r.kernel.org>,  <linux-api@...r.kernel.org>,
  <x86@...nel.org>,  <akpm@...ux-foundation.org>,  <arnd@...db.de>,
  <tglx@...utronix.de>,  <luto@...nel.org>,  <mingo@...hat.com>,
  <bp@...en8.de>,  <dave.hansen@...ux.intel.com>,  <hpa@...or.com>,
  <mhocko@...nel.org>,  <tj@...nel.org>,  <corbet@....net>,
  <rakie.kim@...com>,  <hyeongtak.ji@...com>,  <honggyu.kim@...com>,
  <vtavarespetr@...ron.com>,  <peterz@...radead.org>,
  <jgroves@...ron.com>,  <ravis.opensrc@...ron.com>,
  <sthanneeru@...ron.com>,  <emirakhur@...ron.com>,  <Hasan.Maruf@....com>,
  <seungjun.ha@...sung.com>
Subject: Re: [PATCH v5 01/11] mm/mempolicy: implement the sysfs-based
 weighted_interleave interface

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

> On Wed, Dec 27, 2023 at 02:42:15PM +0800, Huang, Ying wrote:
>> Gregory Price <gourry.memverge@...il.com> writes:
>> 
>> > +		These weights only affect new allocations, and changes at runtime
>> > +		will not cause migrations on already allocated pages.
>> > +
>> > +		Writing an empty string resets the weight value to 1.
>> 
>> I still think that it's a good idea to provide some better default
>> weight value with HMAT or CDAT if available.  So, better not to make "1"
>> as part of ABI?
>> 
>
> That's the eventual goal,

Good to know this.

> but this is just the initial mechanism.
>
> My current thought is that the CXL driver will apply weights as the
> system iterates through devices and creates numa nodes.  In the
> meantime, you have to give the "possible" nodes a default value to
> prevent nodes onlined after boot from showing up with 0-value.
>
> Not allowing 0-value weights is simply easier in many respects.
>
>> > +
>> > +		Minimum weight: 1
>> 
>> Can weight be "0"?  Do we need a way to specify that a node don't want
>> to participate weighted interleave?
>> 
>
> In this code, weight cannot be 0.  My thoguht is that removing the node
> from the nodemask is the way to denote 0.
>
> The problem with 0 is hotplug, migration, and cpusets.mems_allowed.  
>
> Example issue:  Use set local weights to [1,0,1,0] for nodes [0-3],
> and has a cpusets.mems_allowed mask of (0, 2).
>
> Lets say the user migrates the task via cgroups from nodes (0,2) to
> (1,3).
>
> The task will instantly crash as basically OOM because weights of
> [1,0,1,0] will prevent memory from being allocations.
>
> Not allowing nodes weights of 0 is defensive.  Instead, simply removing
> the node from the nodemask and/or mems_allowed is both equivalent to and
> the preferred way to apply a weight of 0.

It sounds reasonable to set minimum weight to 1.  But "1" may be not the
default weight, so, I don't think it's a good idea to make "1" as
default in ABI.

>> > +		Maximum weight: 255
>> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>> > index 10a590ee1c89..0e77633b07a5 100644
>> > --- a/mm/mempolicy.c
>> > +++ b/mm/mempolicy.c
>> > @@ -131,6 +131,8 @@ static struct mempolicy default_policy = {
>> >  
>> >  static struct mempolicy preferred_node_policy[MAX_NUMNODES];
>> >  
>> > +static char iw_table[MAX_NUMNODES];
>> > +
>> 
>> It's kind of obscure whether "char" is "signed" or "unsigned".  Given
>> the max weight is 255 above, it's better to use "u8"?
>>
>
> bah, stupid mistake.  I will switch this to u8.
>
>> And, we may need a way to specify whether the weight has been overridden
>> by the user.
>> A special value (such as 255) can be used for that.  If
>> so, the maximum weight should be 254 instead of 255.  As a user space
>> interface, is it better to use 100 as the maximum value?
>> 
>
> There's global weights and local weights.  These are the global weights.
>
> Local weights are stored in task->mempolicy.wil.il_weights.
>
> (policy->mode_flags & MPOL_F_GWEIGHT) denotes the override.
> This is set if (mempolicy_args->il_weights) was provided.
>
> This simplifies the interface.
>
> (note: local weights are not introduced until the last patch 11/11)

The global weight is writable via sysfs too, right?  Then, for global
weights, we have 2 sets of values,

iw_table_default[], and iw_table[].

iw_table_default[] is set to "1" now, and will be set to some other
values after we have enabled HMAT/CDAT based default value.

iw_table[] is initialized with a special value (for example, "0", if "1"
is the minimal weight).  And users can change it via sysfs.  Then the
actual global weight for a node becomes

    iw_table[node] ? : iw_table_default[node]


As for global weight and local weight, we may need a way to copy from
the global weights to the local weights to simplify the memory policy
setup.  For example, if users want to use the global weights of node 0,
1, 2 and override the weight of node 3.  They can specify some special
value, for example, 0, in mempolicy_args->il_weights[0], [1], [2] to
copy from the global values, and override [3] via some other value.


Think about the default weight value via HMAT/CDAT again.  It may be not
a good idea to use "1" as default even for now.

For example,

- The memory bandwidth of DRAM is 100GB, whose default weight is "1".

- We hot-plug CXL.mem A with memory bandwidth 20GB.  So, we change the
  weight of DRAM to 5, and use "1" as the weight of CXL.mem A.

- We hot-plug CXL.mem B with memory bandwidth 10GB.  So, we change the
  weight of DRAM to 10, the weight of CXL.mem A to 2, and use "1" as the
  weight of CXL.mem B.

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?

>> > +
>> > +static void sysfs_mempolicy_release(struct kobject *mempolicy_kobj)
>> > +{
>> > +	int i;
>> > +
>> > +	for (i = 0; i < MAX_NUMNODES; i++)
>> > +		sysfs_wi_node_release(node_attrs[i], mempolicy_kobj);
>> 
>> IIUC, if this is called in error path (such as, in
>> add_weighted_interleave_group()), some node_attrs[] element may be
>> "NULL"?
>> 
>
> The null check is present in sysfs_wi_node_release
>
> if (!node_attr)
> 	return;

This works.  Sorry for noise.

> Is it preferable to pull this out? Seemed more defensive to put it
> inside the function.

Both are OK for me.

--
Best Regards,
Huang, Ying

Powered by blists - more mailing lists