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. ~Gregory
