Received-SPF: pass (google.com: domain of linux-kernel+bounces-46128-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: "Huang, Ying" <ying.huang@intel.com>
To: Gregory Price <gregory.price@memverge.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
In-Reply-To: <Zbn6FG3346jhrQga@memverge.com> (Gregory Price's message of "Wed,
	31 Jan 2024 02:43:16 -0500")
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>
Date: Wed, 31 Jan 2024 17:19:51 +0800
Message-ID: <87y1c5g8qw.fsf@yhuang6-desk2.ccr.corp.intel.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=ascii

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

> On Wed, Jan 31, 2024 at 02:43:12PM +0800, Huang, Ying wrote:
>> Gregory Price <gourry.memverge@gmail.com> writes:
>> >  
>> > +static unsigned int weighted_interleave_nodes(struct mempolicy *policy)
>> > +{
>> > +	unsigned int node = current->il_prev;
>> > +
>> > +	if (!current->il_weight || !node_isset(node, policy->nodes)) {
>> > +		node = next_node_in(node, policy->nodes);
>> > +		/* can only happen if nodemask is being rebound */
>> > +		if (node == MAX_NUMNODES)
>> > +			return node;
>> 
>> I feel a little unsafe to read policy->nodes at same time of writing in
>> rebound.  Is it better to use a seqlock to guarantee its consistency?
>> It's unnecessary to be a part of this series though.
>> 
>
> I think this is handled already? It is definitely an explicit race
> condition that is documented elsewhere:
>
> /*
>  * mpol_rebind_policy - Migrate a policy to a different set of nodes
>  *
>  * Per-vma policies are protected by mmap_lock. Allocations using per-task
>  * policies are protected by task->mems_allowed_seq to prevent a premature
>  * OOM/allocation failure due to parallel nodemask modification.
>  */

Thanks for pointing this out!

If we use task->mems_allowed_seq reader side in
weighted_interleave_nodes() we can guarantee the consistency of
policy->nodes.  That may be not deserved, because it's not a big deal to
allocate 1 page in a wrong node.

It makes more sense to do that in
alloc_pages_bulk_array_weighted_interleave(), because a lot of pages may
be allocated there.

> example from slub:
>
> do {
> 	cpuset_mems_cookie = read_mems_allowed_begin();
> 	zonelist = node_zonelist(mempolicy_slab_node(), pc->flags);
> 	...
> } while (read_mems_allowed_retry(cpuset_mems_cookie));
>
> quick perusal through other allocators, show similar checks.
>
> page_alloc.c  -  check_retry_cpusetset()
> filemap.c     -  filemap_alloc_folio()
>
> If we ever want mempolicy to be swappable from outside the current task
> context, this will have to change most likely - but that's another
> feature for another day.
>

--
Best Regards,
Huang, Ying