Gregory Price <gregory.price@xxxxxxxxxxxx> writes: > On Fri, Jan 26, 2024 at 03:40:27PM +0800, Huang, Ying wrote: >> Gregory Price <gourry.memverge@xxxxxxxxx> writes: >> >> > Two special observations: >> > - if the weight is non-zero, cur_il_weight must *always* have a >> > valid node number, e.g. it cannot be NUMA_NO_NODE (-1). >> >> IIUC, we don't need that, "MAX_NUMNODES-1" is used instead. >> > > Correct, I just thought it pertinent to call this out explicitly since > I'm stealing the top byte, but the node value has traditionally been a > full integer. > > This may be relevant should anyone try to carry, a random node value > into this field. For example, if someone tried to copy policy->home_node > into cur_il_weight for whatever reason. > > It's worth breaking out a function to defend against this - plus to hide > the bit operations directly as you recommend below. > >> > /* Weighted interleave settings */ >> > - u8 cur_il_weight; >> > + atomic_t cur_il_weight; >> >> If we use this field for node and weight, why not change the field name? >> For example, cur_wil_node_weight. >> > > ack. > >> > + if (cweight & 0xFF) >> > + *policy = cweight >> 8; >> >> Please define some helper functions or macros instead of operate on bits >> directly. >> > > ack. > >> > else >> > *policy = next_node_in(current->il_prev, >> > pol->nodes); >> >> If we record current node in pol->cur_il_weight, why do we still need >> curren->il_prev. Can we only use pol->cur_il_weight? And if so, we can >> even make current->il_prev a union. >> > > I just realized that there's a problem here for shared memory policies. > > from weighted_interleave_nodes, I do this: > > cur_weight = atomic_read(&policy->cur_il_weight); > ... > weight--; > ... > atomic_set(&policy->cur_il_weight, cur_weight); > > On a shared memory policy, this is a race condition. > > > I don't think we can combine il_prev and cur_wil_node_weight because > the task policy may be different than the current policy. > > i.e. it's totally valid to do the following: > > 1) set_mempolicy(MPOL_INTERLEAVE) > 2) mbind(..., MPOL_WEIGHTED_INTERLEAVE) > > Using current->il_prev between these two policies, is just plain incorrect, > so I will need to rethink this, and the existing code will need to be > updated such that weighted_interleave does not use current->il_prev. IIUC, weighted_interleave_nodes() is only used for mempolicy of tasks (set_mempolicy()), as in the following code. + *nid = (ilx == NO_INTERLEAVE_INDEX) ? + weighted_interleave_nodes(pol) : + weighted_interleave_nid(pol, ilx); 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. -- Best Regards, Huang, Ying
