On Mon, Jan 29, 2024 at 04:17:46PM +0800, Huang, Ying wrote:
> Gregory Price <gregory.price@xxxxxxxxxxxx> writes:
>
> > 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);
>
Was digging through this the past couple of days. It does look like
this is true - because if (pol) comes from a vma, ilx will not be
NO_INTERLEAVE_INDEX. If this changes in the future, however,
weighted_interleave_nodes may begin to miscount under heavy contention.
It may be worth documenting this explicitly, because this is incredibly
non-obvious. I will add a comment to this chunk here.
> 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.
>
Given all of this, I think is reasonably. That is effectively what is
happening anyway for anyone that just uses `numactl -w --interleave=...`
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?
The latter involves additional code updates in mempolicy.c for the union
name (current->___.il_prev) but it lets us add documentation to mempolicy.h
~Gregory
