Received-SPF: pass (google.com: domain of linux-kernel+bounces-40350-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 26 Jan 2024 11:38:01 -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 v3 4/4] mm/mempolicy: change cur_il_weight to atomic and
 carry the node with it
Message-ID: <ZbPf6d2cQykdl3Eb@memverge.com>
References: <20240125184345.47074-1-gregory.price@memverge.com>
 <20240125184345.47074-5-gregory.price@memverge.com>
 <87sf2klez8.fsf@yhuang6-desk2.ccr.corp.intel.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87sf2klez8.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?Z+WugfFt/IfMKlE48oWzkeEtFD3c73JgpgUGNff7+eGpKhwtHRXjpWajW73O?=
 =?us-ascii?Q?NANm9o/LnfC2mZF3N92Hf5XNA4J1xQfKkvogxwGfE8yDWcbxvh7kxLnJMYuh?=
 =?us-ascii?Q?uIqz1LGm+yXJj+TF9ZdgArTBXf8hD6GhyXq3dJT+V4xWhhv9WpppvyHvJ8rM?=
 =?us-ascii?Q?A1hs9sI5mnXPP7+8GyKnxX8BcG7ALGY6f92yz0RfGe+wm67WFoIGqr6aHj8T?=
 =?us-ascii?Q?gwRObPT8FT2mndhdqgd7Htzk9GyDmFIUXeMqd+xdcOyAQ+Ph2oetcl9fJnuF?=
 =?us-ascii?Q?8vQT+WlBNRbVfBSgkyKUyYfYiYHJAzv4WYcWXy4b9vFwoB9CJUr4fHhzAagP?=
 =?us-ascii?Q?U3pmZnZslE8FG9zHHkl3y6ZpAAPKqFXJgc0Llu/COGUt75vmGPDjqsDjuovl?=
 =?us-ascii?Q?IqIkJ3OeBs0Kai+KQ20rt7vx64T6+plFztptNC/grdpnXCsh8nVB4JbQ8nmX?=
 =?us-ascii?Q?39cceuxS9roVStwLbHqqFu30AhW+byzeO2wcTtdLg/ScgSns73DjsPor3H5h?=
 =?us-ascii?Q?uH+0LJ+TqP5iCjlOza3VarlM71kArvghS+9n6zYKCuJZYGu5NkrBeqxHzHqs?=
 =?us-ascii?Q?U+Y5FlpwicEJUC1g3OfJ8MtFEROTmIG1qAO6vjYOD++YqjtneXY3plI6bYn+?=
 =?us-ascii?Q?NFfwTWfIqQSU27wUE0oZi9xkpMMbLfA/GTIoVXiJA+F9lSnEmrPbhHUK4Bwq?=
 =?us-ascii?Q?egQ1DdycELrvVt3l64HfxwnA2mih0kIJchCWZQU5UuO3pQyWUGhnHp98kOH7?=
 =?us-ascii?Q?ncjxgHtvJUobjlkGEDGUAAKI3wqYn59bLzuktJODl1VPuQ0H36TDrG6qPFzS?=
 =?us-ascii?Q?5gAD8eyfOao79A8fWZWO4aMme2grSLm2w6N2nZCXGbjZnRdaeYiR4J2jC0Qc?=
 =?us-ascii?Q?3YSYXkspFHrRXNim310Hp0H2KNl/fy/vmagjM+qCicR0Djs7CJlf+O0HaIEH?=
 =?us-ascii?Q?/azu8Xnd5SEcMHvo99hwFc+JjVfqHhWxt3kpwd8zI4gA7WBcy7aa7qtIP0Hh?=
 =?us-ascii?Q?VBBJ5vinwZv0joLbJrjt0skXERUsbbmYzwhZLQWLoLFiLCDqwkt49j0noGVv?=
 =?us-ascii?Q?3+IpWS4djnaVz1/X/s+/Km4DAxjka9473JV86W/X31WyK/WffujKZMEtAMU7?=
 =?us-ascii?Q?LTamtTCdAIi4SzDoyemi28qkly4Voxxa/YLtRMd97VCkS5Wq1ijoiAgDFbVl?=
 =?us-ascii?Q?0G8hbqjqLNBmbVYctFTGxMuyTlzWWO59Zqgk8IeaiG9qEHd0SNM7AjXC/Izy?=
 =?us-ascii?Q?qZJB9V7YljVLclEfCQVg230h55I022ybpX6ZFB+yUFIKP6flwMCxp8qcLsHl?=
 =?us-ascii?Q?ZGnPRPupQJxLCZ3js0wP4Ts3BxrgfvT4aIBIEgse5FXz2Qj2+xI3TTx9HuBL?=
 =?us-ascii?Q?f7dATMb8Yu14wWTeiGZESfqWZHCuvCs99Rpdl7XDqdviQD31ROgrLCEiBPm9?=
 =?us-ascii?Q?Ld/4peZ0EN7428dOdaO81I4N74x3gJPex7EcZX1MXrAQJY3Knw3vwX3QeOsV?=
 =?us-ascii?Q?5UJWgB5eXTkNm4SjMjf9aFoW0o8/eJxQl4OSHzzg4zsNmVyiapqtaVwKb6ar?=
 =?us-ascii?Q?MahM/VLMuHio+jsNRxfEUXvyiBC3eCT8PbuErewAAMQdonhXMHKba6an/ULi?=
 =?us-ascii?Q?pg=3D=3D?=
X-OriginatorOrg: memverge.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 88dba959-1831-475e-58f7-08dc1e8d2f1e
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR17MB5512.namprd17.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jan 2024 16:38:11.0983
 (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: oe4gaOmOY38dGHbGizVvHnUEnlx5c5PGJs5QEk4eAB83HzK99r3R3oWxVRq/rA2PbLLxLWx3rYVWEwo7FkGsp9tKNDujiBPEZCSbiPmeOQo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR17MB4537

On Fri, Jan 26, 2024 at 03:40:27PM +0800, Huang, Ying wrote:
> Gregory Price <gourry.memverge@gmail.com> writes:
> 
> > Two special observations:
> > - if the weight is non-zero, cur_il_weight must *always* have a
> >   valid node number, e.g. it cannot be NUMA_NO_NODE (-1).
> 
> IIUC, we don't need that, "MAX_NUMNODES-1" is used instead.
> 

Correct, I just thought it pertinent to call this out explicitly since
I'm stealing the top byte, but the node value has traditionally been a
full integer.

This may be relevant should anyone try to carry, a random node value
into this field. For example, if someone tried to copy policy->home_node
into cur_il_weight for whatever reason.

It's worth breaking out a function to defend against this - plus to hide
the bit operations directly as you recommend below.

> >  	/* Weighted interleave settings */
> > -	u8 cur_il_weight;
> > +	atomic_t cur_il_weight;
> 
> If we use this field for node and weight, why not change the field name?
> For example, cur_wil_node_weight.
> 

ack.

> > +			if (cweight & 0xFF)
> > +				*policy = cweight >> 8;
> 
> Please define some helper functions or macros instead of operate on bits
> directly.
> 

ack.

> >  			else
> >  				*policy = next_node_in(current->il_prev,
> >  						       pol->nodes);
> 
> If we record current node in pol->cur_il_weight, why do we still need
> curren->il_prev.  Can we only use pol->cur_il_weight?  And if so, we can
> even make current->il_prev a union.
> 

I just realized that there's a problem here for shared memory policies.

from weighted_interleave_nodes, I do this:

cur_weight = atomic_read(&policy->cur_il_weight);
..
weight--;
..
atomic_set(&policy->cur_il_weight, cur_weight);

On a shared memory policy, this is a race condition.


I don't think we can combine il_prev and cur_wil_node_weight because
the task policy may be different than the current policy.

i.e. it's totally valid to do the following:

1) set_mempolicy(MPOL_INTERLEAVE)
2) mbind(..., MPOL_WEIGHTED_INTERLEAVE)

Using current->il_prev between these two policies, is just plain incorrect,
so I will need to rethink this, and the existing code will need to be
updated such that weighted_interleave does not use current->il_prev.

~Gregory