Received-SPF: pass (google.com: domain of linux-kernel+bounces-43946-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
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>
Subject: Re: [PATCH v3 4/4] mm/mempolicy: change cur_il_weight to atomic and
 carry the node with it
In-Reply-To: <ZbhuJTBp68e8eLRv@memverge.com> (Gregory Price's message of "Mon,
	29 Jan 2024 22:33:57 -0500")
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>
	<ZbhuJTBp68e8eLRv@memverge.com>
Date: Tue, 30 Jan 2024 13:18:30 +0800
Message-ID: <871q9ziel5.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 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.

So, I added a check as follows,

node_isset(current->il_prev, policy->nodes)

If prev node is removed from nodemask, allocation will proceed to the
next node.  Otherwise, it's safe to use current->il_weight.  

--
Best Regards,
Huang, Ying

> 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