Received-SPF: pass (google.com: domain of linux-kernel+bounces-28534-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 17 Jan 2024 00:24:41 -0500
From: Gregory Price <gregory.price@memverge.com>
To: "Huang, Ying" <ying.huang@intel.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
Subject: Re: [PATCH 1/3] mm/mempolicy: implement the sysfs-based
 weighted_interleave interface
Message-ID: <ZadkmWj3Rd483f68@memverge.com>
References: <20240112210834.8035-1-gregory.price@memverge.com>
 <20240112210834.8035-2-gregory.price@memverge.com>
 <87le8r1dzr.fsf@yhuang6-desk2.ccr.corp.intel.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87le8r1dzr.fsf@yhuang6-desk2.ccr.corp.intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?BQDbqw2BvFnX90OuMt0wZhakFUnNeQ2+OqAgtwkM3lCsU2VlKaIG/0QSUV3E?=
 =?us-ascii?Q?nqBku8CeV/dasfTzugXrIdtLT2UpDJ4iIVALx7SjjJ6hHdzD6eVqjAOMYbXI?=
 =?us-ascii?Q?athLR0tk1HO4wogA/LRoIcbHhGuyZQSMuINCr/n1aJlMVMnpW5ks1P6FxG1A?=
 =?us-ascii?Q?6dXaDRHuZCnEIl7E6PBpgNlSdIwiXe4rVYFq0vWXuMIsJeAU6GQfXBzdv26G?=
 =?us-ascii?Q?WCHPJLStNLB9x6OhRh82CDpu2mI48c6bC/hg16Oic9xkhgrsY+pQN6tQjvP5?=
 =?us-ascii?Q?hUEWRQ4iCktubhPdjDQskjCnCmkKeAdBXckPUhfyi3o2w8+eoHIt/kfnGIGR?=
 =?us-ascii?Q?cWJYawSQScO0Jl1R5z21NixNrMnqC26iSmObq9R1Y4ykKqrJ1SnuhRXgDnnb?=
 =?us-ascii?Q?ETT4cYWctHk3seg3kqZO7c7GbttqTq+kfBHaXeBxRUAjkwHzcz9ZqKn1PCHE?=
 =?us-ascii?Q?5bz0QuMv6mzUh22v6oDxgz+cKt77JZTw8XrcsqgCKfmuS53/H3dV1VqYB9Fn?=
 =?us-ascii?Q?Yc6OhlT3v7DxtmwqFcsAxFa4QTKc0W45Bumw4tQJ8VXvnJ3hkdb+Fl3qNN2e?=
 =?us-ascii?Q?p0IHqfoGE1lFfdDz5Hl35R/8d19owfVi9jp0zSkDMo6yPKH/dAuLWZ565w5m?=
 =?us-ascii?Q?TbhBzLEZB34jJJh+y7bl9pnIBxk54MjGwbSE5QqoL96JmJTUz0J4qQ30m7wV?=
 =?us-ascii?Q?rtMTYNWbWkHfI0BUlq79i073pWcbrYD0ElnMJEHB7brZJ/1n5bKyaFk0JH8b?=
 =?us-ascii?Q?lVv+adTiqF4jz5RXDXIlbGr4QORhbufzD906Wj038YfesM8bR5Dlxge1PZ/c?=
 =?us-ascii?Q?SlBfchJveFXKLOM44MYvnSYZq6hcyfkw0xoLrbPzyPWMpI+lYhS4imQ0EX9y?=
 =?us-ascii?Q?eOt/8bZmlVQ+u3rG+/DG5bgZt0l3ZGQZ/xHgSj0Wpiuqn2bxa/ZUImjHTBJt?=
 =?us-ascii?Q?bDoD2cYTCUcinMju+/Kgs5Ohq6EFBJNnOBfVMjN6NIqrBWYM0RITsPStH4tT?=
 =?us-ascii?Q?KsgZGfyjcHEQ7O97O3oEOz5OCCoZ3ZHNKdclC3Ky9fuFQfyvVjP6irQUWaEg?=
 =?us-ascii?Q?nc0pm2dPC3gx0+faGlWP1gdNU4tl2KsXX+LsYUPlvSms/SQH/3q1ABbQSSG1?=
 =?us-ascii?Q?7L0M8GheDUFFWXiplWY5gc5nFglyHd/79GU8HZwYBjqrdp7RwBThLMxgzva5?=
 =?us-ascii?Q?c2HqTlMEVjromU5lmDQeifo7MUGaEOWtu5/Km9sZSoAyR1INbz8PUi7/WxT6?=
 =?us-ascii?Q?1DXmmWrufonMAXRthnqOHj1KGeqZ5uZ8XoVoNsYwVn6Pp9WsQjIDBpXy2aaE?=
 =?us-ascii?Q?dY4Z1TUZN8R8583rjYhytsHgu3W1V2iTQLFKi6y3cEdCmAP23lG/O3tCMlxv?=
 =?us-ascii?Q?o8QMQ+g9gBQ1g8cTHT5djTnUwytkjArSc1wwEqxtMtrzFLHsBP1tyeB77O30?=
 =?us-ascii?Q?EhnCMh9c+/PSAGmuM+FWfAuXNsqoF5d05iREWBJDN/Kfz9WnTtHhm+WIl0Qn?=
 =?us-ascii?Q?2Qm0Pgw/PfWb2LckrNOSupye+nTnUOdDXwECYv4BzXVoCUaAexnZsS/EVbPF?=
 =?us-ascii?Q?pV0YVM1KEGoZfIwI84+Jm6fz1Sj467y5WvKKO32tMh1FlHlIB8+jC54Nkn2q?=
 =?us-ascii?Q?NQ=3D=3D?=
X-OriginatorOrg: memverge.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fad66eec-3654-4b87-bbb9-08dc171ca09e
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR17MB5512.namprd17.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jan 2024 05:24:50.2015
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 5c90cb59-37e7-4c81-9c07-00473d5fb682
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: NxtQDhuYmxARNWVJvKcwNpakCZRS6axH2jq0VIauLj7omHVoiz4lQ7xZSjvo4z2gOC01Wioz8wsiSAFStDi7IwsxiLBi0sSCNc591sL/+ps=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR17MB6514

On Mon, Jan 15, 2024 at 11:18:00AM +0800, Huang, Ying wrote:
> Gregory Price <gourry.memverge@gmail.com> writes:
> 
> > +static struct iw_table default_iw_table;
> > +/*
> > + * iw_table is the sysfs-set interleave weight table, a value of 0
> > + * denotes that the default_iw_table value should be used.
> > + *
> > + * iw_table is RCU protected
> > + */
> > +static struct iw_table __rcu *iw_table;
> > +static DEFINE_MUTEX(iw_table_mtx);
> 
> I greped "mtx" in kernel/*.c and mm/*.c and found nothing.  To following
> the existing coding convention, better to name this as iw_table_mutex or
> iw_table_lock?
> 

ack.

> And, I think this is used to protect both iw_table and default_iw_table?
> If so, it deserves some comments.
> 

Right now default_iw_table cannot be updated, and so it is neither
protected nor requires protection.

I planned to add the protection comment in the next patch series, which
would implement the kernel-side interface for updating the default
weights during boot/hotplug.

We haven't had the discussion on how/when this should happen yet,
though, and there's some research to be done.  (i.e. when should DRAM
weights be set? should the entire table be reweighted on hotplug? etc)

> > +static ssize_t node_store(struct kobject *kobj, struct kobj_attribute *attr,
> > +			  const char *buf, size_t count)
> > +{
> > +	struct iw_node_attr *node_attr;
> > +	struct iw_table __rcu *new;
> > +	struct iw_table __rcu *old;
> > +	u8 weight = 0;
> > +
> > +	node_attr = container_of(attr, struct iw_node_attr, kobj_attr);
> > +	if (count == 0 || sysfs_streq(buf, ""))
> > +		weight = 0;
> > +	else if (kstrtou8(buf, 0, &weight))
> > +		return -EINVAL;
> > +
> > +	new = kmalloc(sizeof(*new), GFP_KERNEL);
> > +	if (!new)
> > +		return -ENOMEM;
> > +
> > +	mutex_lock(&iw_table_mtx);
> > +	old = rcu_dereference_protected(iw_table,
> > +					lockdep_is_held(&iw_table_mtx));
> > +	/* If value is 0, revert to default weight */
> > +	weight = weight ? weight : default_iw_table.weights[node_attr->nid];
> 
> If we change the default weight in default_iw_table.weights[], how do we
> identify whether the weight has been customized by users via sysfs?  So,
> I suggest to use 0 in iw_table for not-customized weight.
> 
> And if so, we need to use RCU to access default_iw_table too.
>

Dumb simplification on my part, I'll walk this back and add the 

if (!weight) weight = default_iw_table[node]

logic back into the allocator paths accordinly.

> > +	memcpy(&new->weights, &old->weights, sizeof(new->weights));
> > +	new->weights[node_attr->nid] = weight;
> > +	rcu_assign_pointer(iw_table, new);
> > +	mutex_unlock(&iw_table_mtx);
> > +	kfree_rcu(old, rcu);
> 
> synchronize_rcu() should be OK here.  It's fast enough in this cold
> path.  This make it good to define iw_table as
> 
I'll take a look.

> u8 __rcu *iw_table;
> 
> Then, we only need to allocate nr_node_ids elements now.
> 

We need nr_possible_nodes to handle hotplug correctly.

I decided to simplify this down to MAX_NUMNODES *juuuuuust in case*
"true node hotplug" ever becomes a reality.  If that happens, then
only allocating space for possible nodes creates a much bigger
headache on hotplug.

For the sake of that simplification, it seemed better to just eat the
1KB.  If you really want me to do that, I will, but the MAX_NUMNODES
choice was an explicitly defensive choice.

> > +static int __init mempolicy_sysfs_init(void)
> > +{
> > +	/*
> > +	 * if sysfs is not enabled MPOL_WEIGHTED_INTERLEAVE defaults to
> > +	 * MPOL_INTERLEAVE behavior, but is still defined separately to
> > +	 * allow task-local weighted interleave and system-defaults to
> > +	 * operate as intended.
> > +	 *
> > +	 * In this scenario iw_table cannot (presently) change, so
> > +	 * there's no need to set up RCU / cleanup code.
> > +	 */
> > +	memset(&default_iw_table.weights, 1, sizeof(default_iw_table));
> 
> This depends on sizeof(default_iw_table.weights[0]) == 1, I think it's
> better to use explicit loop here to make the code more robust a little.
> 

oh hm, you're right.  rookie mistake on my part.

Thanks,
Gregory