On Tue, Jan 30, 2024 at 11:15:35AM +0800, Huang, Ying wrote: > Gregory Price <gregory.price@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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