On Mon, Jan 29, 2024 at 04:17:46PM +0800, Huang, Ying wrote:

Gregory Price [off-list ref] writes:

quoted

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