Received-SPF: pass (google.com: domain of linux-kernel+bounces-43900-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 29 Jan 2024 22:33:57 -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: <ZbhuJTBp68e8eLRv@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>
 <ZbPf6d2cQykdl3Eb@memverge.com>
 <877cjsk0yd.fsf@yhuang6-desk2.ccr.corp.intel.com>
 <ZbfI3+nhgQlNKMPG@memverge.com>
 <ZbfqVHA9+38/j3Mq@memverge.com>
 <875xzbika0.fsf@yhuang6-desk2.ccr.corp.intel.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <875xzbika0.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?XSO7zTeGpvlrIKx37QqonTKuabk25VHFh2C1zsfK9RaHkAnSPb6DPN4etKQa?=
 =?us-ascii?Q?Ynmo+T2erlx+yOc8mt7NzCoVbD5Z/kHjXJ6br1TqKCuTvQQYx5JaP+Lsqg03?=
 =?us-ascii?Q?hGyLif0j2sz/Ko1zCD2urxJ6tdRFJwavtGfMAem8zjBad5hd7eRlTQ0swrul?=
 =?us-ascii?Q?0iDEjTGbrGhXmucuKw/miglTxvhe9OsVIdo5FNP524IHMXPqMnY56uFuOq3v?=
 =?us-ascii?Q?YaPL8IMuYLaWcQgw0APdC+ZFCNk7EUZpUeX7jaD06Ta5CvQytVHR8MNq9+8G?=
 =?us-ascii?Q?0y5C7SYbYP9IIRQTa0GU9bb7/L5GwlFwsZpv7GrTi9GEtQmW8cnACbXBSRf0?=
 =?us-ascii?Q?LngumgJD5HbOSfNUl4646P3XW60uaT9C83SpF49Lcewiy972Xw+vqMCGmW4e?=
 =?us-ascii?Q?Tsjy3OYCXANfyeSH1jqoFsZ7rYPSCleUT7lqGHfMgdwHToSSZzlFjIiMEpXl?=
 =?us-ascii?Q?4ysBG5/2YQMW3P/kDlsXPZUnpGDkt+D0bW/w836rTpQx1iiaIQVeHVphsF6N?=
 =?us-ascii?Q?3phvgjRRJbvcIlXjyuzzRh5OoUKPtUXqsr8WRtn9fJoiwx1MhjPNvw5PRUYk?=
 =?us-ascii?Q?8oAvkjSS45RHtKfPrxHVvTFC8LRqVkkBA7d06bAdQ6LsZ4HD0SStcIuHf/+a?=
 =?us-ascii?Q?RwRWsOrIsEBCZpdYmbrQRSj/wnQuZpgasvl2IA44QI728MBpVZInyZjmZi4L?=
 =?us-ascii?Q?l+hyhht5MKcJdgGOj6f967f+q3QOLBYi7A7OGi/ckvX75QP1VjQr2newsvV3?=
 =?us-ascii?Q?H4qd7UO9I5gTNZoYEYYAvetjY4m0Ct3+JbOu13dEgtm5hBH+byI4scGGEkvG?=
 =?us-ascii?Q?7AxdiaPj8rNImclBUpUlJoFSaGh2xnHifxTK4LOQBgs7OymHOlf73ihirOhC?=
 =?us-ascii?Q?cUtEbsC5ToJakVyJWE0+Sy8irEyp0P+wEaVlnveZx0Ix2bsbcYQ4uzNoQbq8?=
 =?us-ascii?Q?uYoJa/Dh9RS6SplUZ1zFQoSWJYE+TTAIBXL7tKgFbd3ybCxorUHp5rF6H7zW?=
 =?us-ascii?Q?hywhCPqmJxANburtglmVnP0k51f6E2toTlwNcF1AS5KuLbuxDu6m3y30/+uK?=
 =?us-ascii?Q?0eqXKIri4DBGuNMsSJulxvYc5znz9PdTUDqq+YKnoT8WxYiwP0r2Uo3PzJiB?=
 =?us-ascii?Q?b9tmO1smujNv8ssoIHcXzQzNhFxJ0/Wryd6mUvrZGrorWYJyn81Vq4mAVI5i?=
 =?us-ascii?Q?vTZLuB97zKAiHHyI8Pqgr3KqMMDDeLyFlcRNxblsrMu9aIoohntNGHQyAiJz?=
 =?us-ascii?Q?heN4MUcAM/nm4whPxt4sz3JAmUVQrDBoe5f10qbt5O0G5M8AVKG8QPiGCJpg?=
 =?us-ascii?Q?3Jv2CTj2i9NUt6a8QsxJtzdLlDXqa5B5yXJq3FKxTs8W49hhfR6GeSw0TGH8?=
 =?us-ascii?Q?2/10Zecxe5I1ZrUN1aqt8xx6/x/Po1Ss1ziCubBI8uoCbO/b4mPnUm61CJJ9?=
 =?us-ascii?Q?FwPwqAzCZdk+lGnGdl9J0vew+3/gs7+TeObnHgfQjH8bmfA471mGQgXEX0Fe?=
 =?us-ascii?Q?YgwcJ0z9QJbzUCE8pAuVNH/bDuulmGowzKdQklPRKVj2F7hdKjts/zAFFVZT?=
 =?us-ascii?Q?MbMKB/lFiiiyGr7UKJDfydBvNWEZBu4d8luUHegOTHUAWi9j0b1VmbRrBnBX?=
 =?us-ascii?Q?3w=3D=3D?=
X-OriginatorOrg: memverge.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 598d13bb-a308-46e1-2bbf-08dc21444dc2
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR17MB5512.namprd17.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Jan 2024 03:34:02.7626
 (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: x7Px/sInCVRCI+J9ch4Rtq6sGIq1Ga7bKGpdSloKOnkEduwQaOI2NleoRfnEEIgRGa7dQfuIAu3UopJcEeBqKLxJzQ/gIxL1IdOtCoUVISc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR17MB4282

On Tue, Jan 30, 2024 at 11:15:35AM +0800, Huang, Ying wrote:
> Gregory Price <gregory.price@memverge.com> writes:
> 
> > On Mon, Jan 29, 2024 at 10:48:47AM -0500, Gregory Price wrote:
> >> On Mon, Jan 29, 2024 at 04:17:46PM +0800, Huang, Ying wrote:
> >> > Gregory Price <gregory.price@memverge.com> writes:
> >> > 
> >> > But, in contrast, it's bad to put task-local "current weight" in
> >> > mempolicy.  So, I think that it's better to move cur_il_weight to
> >> > task_struct.  And maybe combine it with current->il_prev.
> >> > 
> >> Style question: is it preferable add an anonymous union into task_struct:
> >> 
> >> union {
> >>     short il_prev;
> >>     atomic_t wil_node_weight;
> >> };
> >> 
> >> Or should I break out that union explicitly in mempolicy.h?
> >> 
> >
> > Having attempted this, it looks like including mempolicy.h into sched.h
> > is a non-starter.  There are build issues likely associated from the
> > nested include of uapi/linux/mempolicy.h
> >
> > So I went ahead and did the following.  Style-wise If it's better to just
> > integrate this as an anonymous union in task_struct, let me know, but it
> > seemed better to add some documentation here.
> >
> > I also added static get/set functions to mempolicy.c to touch these
> > values accordingly.
> >
> > As suggested, I changed things to allow 0-weight in il_prev.node_weight
> > adjusted the logic accordingly. Will be testing this for a day or so
> > before sending out new patches.
> >
> 
> Thanks about this again.  It seems that we don't need to touch
> task->il_prev and task->il_weight during rebinding for weighted
> interleave too.
> 

It's not clear to me this is the case.  cpusets takes the task_lock to
change mems_allowed and rebind task->mempolicy, but I do not see the
task lock access blocking allocations.

Comments from cpusets suggest allocations can happen in parallel.

/*
 * cpuset_change_task_nodemask - change task's mems_allowed and mempolicy
 * @tsk: the task to change
 * @newmems: new nodes that the task will be set
 *
 * We use the mems_allowed_seq seqlock to safely update both tsk->mems_allowed
 * and rebind an eventual tasks' mempolicy. If the task is allocating in
 * parallel, it might temporarily see an empty intersection, which results in
 * a seqlock check and retry before OOM or allocation failure.
 */


For normal interleave, this isn't an issue because it always proceeds to
the next node. The same is not true of weighted interleave, which may
have a hanging weight in task->il_weight.

That is why I looked to combine the two, so at least node/weight were
carried together.

> unsigned int weighted_interleave_nodes(struct mempolicy *policy)
> {
>         unsigned int nid;
>         struct task_struct *me = current;
> 
>         nid = me->il_prev;
>         if (!me->il_weight || !node_isset(nid, policy->nodes)) {
>                 nid = next_node_in(...);
>                 me->il_prev = nid;
>                 me->il_weight = weights[nid];
>         }
>         me->il_weight--;
> 
>         return nid;
> }

I ended up with this:

static unsigned int weighted_interleave_nodes(struct mempolicy *policy)
{
       unsigned int node;
       u8 weight;

       get_wil_prev(&node, &weight);
       /* If nodemask was rebound, just fetch the next node */
       if (!weight) {
               node = next_node_in(node, policy->nodes);
               /* can only happen if nodemask has become invalid */
               if (node == MAX_NUMNODES)
                       return node;
               weight = get_il_weight(node);
       }
       weight--;
       set_wil_prev(node, weight);
       return node;
}

~Gregory