Received-SPF: pass (google.com: domain of linux-kernel+bounces-47526-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 31 Jan 2024 21:01:07 -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,
	Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>
Subject: Re: [PATCH v4 3/3] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE
 for weighted interleaving
Message-ID: <Zbr7YzjFrrIWXomI@memverge.com>
References: <20240130182046.74278-1-gregory.price@memverge.com>
 <20240130182046.74278-4-gregory.price@memverge.com>
 <877cjqgfzz.fsf@yhuang6-desk2.ccr.corp.intel.com>
 <Zbn6FG3346jhrQga@memverge.com>
 <87y1c5g8qw.fsf@yhuang6-desk2.ccr.corp.intel.com>
 <ZbqDgaOsAeXnqRP2@memverge.com>
 <871q9xeyo4.fsf@yhuang6-desk2.ccr.corp.intel.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <871q9xeyo4.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?1usUWRG110xduUaJ6+CUUNdEDudc+icOMzI2tfTe/xSp2jaSfZeHmMEaPaCy?=
 =?us-ascii?Q?LnA12d+Zn/1LTqv2WmAHqeGrSaOXdjROxMhZ5/U0p0bYgKewUHDcIb9mxJVV?=
 =?us-ascii?Q?XWoH+u2MENy1NwMAhjn33R+7eMxlWrSvtG+w44SeFad3TRsLK9zpW2ylYPGq?=
 =?us-ascii?Q?f3C7mfP3SWji6g04CUlTurMNeyczSHEVNH6Rd/Z0thpLzuLXBH4BFJvLsljh?=
 =?us-ascii?Q?xvXucyfxpTYaTHmum27PCYHRoEpX9i9+i8htDi4fi4iKZN70wQawTsy4xMEn?=
 =?us-ascii?Q?cAWNv0vvfxX4DlBIENuGe1YtjiTxA30mKRp0yPpnPRbvoGQWyAled5+OvXYd?=
 =?us-ascii?Q?yhQOedo5ArxsolahPGA5LSGKB+QydtApaZLtCIPW+owAq+PQdUog8Q3GjfLM?=
 =?us-ascii?Q?78W7Lkdivstk+GBUh1TQJChwx3y41kVlHovVHS46bXwneEDcB9Qg/RuC5LjQ?=
 =?us-ascii?Q?fy9Fr7wHdYMH1QTSRsrNcWzQX98JNjyazJdcrZi/F61zMwbev8gHhawCo9ZG?=
 =?us-ascii?Q?YgQDLsHh2ieS1IDckUrFevAixv/F3BGut7iFDj5TKIF28aqupJiR98DCDCeM?=
 =?us-ascii?Q?cd4w4Z0cBVXyuowVy/FnMAuNp6WKuhDu9a9A4+gY7/ih+ZHmG4vPI/l2ydds?=
 =?us-ascii?Q?xC1LS1SvKKeWzg0TY4wNFw6w2Uho8J6X3Xxdm5l+5/YZ1O1rNHQBGGtkQX+E?=
 =?us-ascii?Q?xrldUF9D4d8JDPYwdLFbtJUSkBs+dwfieZlKwMHgzqCMYf/Pe/TzwoN6nNCB?=
 =?us-ascii?Q?TvdGpqsJCMuu0mvUAY8iS8aqQ9uBM4rgwF05lTKPT9xeCeLKTMDeNmk/soa6?=
 =?us-ascii?Q?YFim75jLmhk5pCi/WMyFaawzv5awyKbicjIgxhoiZmxGQTHDzzuSBiKOsS3u?=
 =?us-ascii?Q?M3R9C/ofRqAwCTH3HjZDAZQcMIGeKpbAn5Sqo5k+fLLHMRxdno6lmVUw0h+Y?=
 =?us-ascii?Q?SCl2fp9+MFOo9v1wFqURtUR/XgXXxnwaQ1ueKZWZnid7WQibX7OLrAqPn2Y3?=
 =?us-ascii?Q?SQAbTJzlWFO9vKbPY0xFBtEZpw0ZpOQht1KAs3MJXLBClZkrpmzNTc/2SaUX?=
 =?us-ascii?Q?88Vs+yS9rMUIisHOvLc7FjZpoRwbM2HWAbsstMlkj/ANwidumnobKKoi2FAD?=
 =?us-ascii?Q?su+EpWrb/itFun63Gfge09iB9lmd9+r9MOLxaLT1LdRN27Kkj7Rl35kjWHRm?=
 =?us-ascii?Q?2fYGiWZ59nBy8kk0L5w565nWbJPD9Q6OwJA4wcxd/8QjnqBqBUvp6q/eIswT?=
 =?us-ascii?Q?EoP2CRRE+9WL/eNH1P+FJPqXHFQAGONyhluM4s54VfU+G7iYPjJmaqFGinp2?=
 =?us-ascii?Q?tLBoczCSLn6HMr+b/jkLI5pQcFHYJRJLQwBS/EGsxE/BDZDoqc/9JJnEfNrW?=
 =?us-ascii?Q?AFC713MNtjek5zmey8/dUOLOonlsE9TJgVXMfDMthIvefNTmSmzRi3hpVL+1?=
 =?us-ascii?Q?F5IILDJ5yaaIqyujJEziCC97BW+/OlOK/L+ns1W3yxEKX7jXktOfkeoI2dx3?=
 =?us-ascii?Q?cIx7+ncFAHZncT+vXL7Mtk2lgLNi7+V3xPDCEIW81pdhbe6A8VFRpBK+a43D?=
 =?us-ascii?Q?VioHhed7Nc5k69WjSeHBdEa0srWJGVL6HXqbcTajAt7LUXJM/PiaIHGl6o4b?=
 =?us-ascii?Q?eg=3D=3D?=
X-OriginatorOrg: memverge.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e981c151-c0e2-4185-5402-08dc22c9aa86
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR17MB5512.namprd17.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Feb 2024 02:01:12.5523
 (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: 85Tl/0z3LQwgYidFQ/8172QgbvMADVkERUsl543WBFFHpgkgDPCqK2+0n1OJsBR5VKikkINBwQlJJSmSDoF3S7LPaJ60CwA+TTvsMcD6gYo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR17MB4217

On Thu, Feb 01, 2024 at 09:55:07AM +0800, Huang, Ying wrote:
> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > index b1437396c357..dfd097009606 100644
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -2391,7 +2391,7 @@ static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
> >         unsigned long nr_allocated = 0;
> >         unsigned long rounds;
> >         unsigned long node_pages, delta;
> > -       u8 __rcu *table, *weights, weight;
> > +       u8 __rcu *table, __rcu *weights, weight;
> 
> The __rcu usage can be checked with `sparse` directly.  For example,
> 
> make C=1 mm/mempolicy.o
> 
> More details can be found in
> 
> https://www.kernel.org/doc/html/latest/dev-tools/sparse.html
> 
> Per my understanding, we shouldn't use "__rcu" here.  Please search
> "__rcu" in the following document.
> 
> https://www.kernel.org/doc/html/latest/RCU/checklist.html
> 

Thanks for this, I will sort this out and respond here with changes
before v5.

> > @@ -2460,17 +2454,10 @@ static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
> >                         node_pages += weight;
> >                         delta -= weight;
> >                 } else if (delta) {
> > +                       /* when delta is deleted, resume from that node */
>                                            ~~~~~~~
>                                            depleted?

ack.

> > +retry:
> > +       /* to prevent miscount use tsk->mems_allowed_seq to detect rebind */
> > +       cpuset_mems_cookie = read_mems_allowed_begin();
> >         if (!current->il_weight || !node_isset(node, policy->nodes)) {
> >                 node = next_node_in(node, policy->nodes);
> 
> node will be changed in the loop.  So we need to change the logic here.
> 

Good catch, stupid mistake. ack.

> > @@ -2388,10 +2401,17 @@ static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
> >         int prev_node;
> >         int i;
> >
> > +
> 
> Change by accident?
>

ack.

~Gregory