Gregory Price <gregory.price@xxxxxxxxxxxx> writes: > On Wed, Dec 27, 2023 at 04:32:37PM +0800, Huang, Ying wrote: >> Gregory Price <gourry.memverge@xxxxxxxxx> writes: >> >> > +static unsigned int weighted_interleave_nid(struct mempolicy *pol, pgoff_t ilx) >> > +{ >> > + nodemask_t nodemask = pol->nodes; >> > + unsigned int target, weight_total = 0; >> > + int nid; >> > + unsigned char weights[MAX_NUMNODES]; >> >> MAX_NUMNODSE could be as large as 1024. 1KB stack space may be too >> large? >> > > I've been struggling with a good solution to this. We need a local copy > of weights to prevent weights from changing out from under us during > allocation (which may take quite some time), but it seemed unwise to > to allocate 1KB heap in this particular path. > > Is my concern unfounded? If so, I can go ahead and add the allocation > code. Please take a look at NODEMASK_ALLOC(). >> > + unsigned char weight; >> > + >> > + barrier(); >> >> Memory barrier needs comments. >> > > Barrier is to stabilize nodemask on the stack, but yes i'll carry the > comment from interleave_nid into this barrier as well. Please see below. >> > + >> > + /* first ensure we have a valid nodemask */ >> > + nid = first_node(nodemask); >> > + if (nid == MAX_NUMNODES) >> > + return nid; >> >> It appears that this isn't necessary, because we can check whether >> weight_total == 0 after the next loop. >> > > fair, will snip. > >> > + >> > + /* Then collect weights on stack and calculate totals */ >> > + for_each_node_mask(nid, nodemask) { >> > + weight = iw_table[nid]; >> > + weight_total += weight; >> > + weights[nid] = weight; >> > + } >> > + >> > + /* Finally, calculate the node offset based on totals */ >> > + target = (unsigned int)ilx % weight_total; >> >> Why use type casting? >> > > Artifact of old prototypes, snipped. > >> > + >> > + /* Stabilize the nodemask on the stack */ >> > + barrier(); >> >> I don't think barrier() is needed to wait for memory operations for >> stack. It's usually used for cross-processor memory order. >> > > This is present in the old interleave code. To the best of my > understanding, the concern is for mempolicy->nodemask rebinding that can > occur when cgroups.cpusets.mems_allowed changes. > > so we can't iterate over (mempolicy->nodemask), we have to take a local > copy. > > My *best* understanding of the barrier here is to prevent the compiler > from reordering operations such that it attempts to optimize out the > local copy (or do lazy-fetch). > > It is present in the original interleave code, so I pulled it forward to > this, but I have not tested whether this is a bit paranoid or not. > > from `interleave_nid`: > > /* > * The barrier will stabilize the nodemask in a register or on > * the stack so that it will stop changing under the code. > * > * Between first_node() and next_node(), pol->nodes could be changed > * by other threads. So we put pol->nodes in a local stack. > */ > barrier(); Got it. This is kind of READ_ONCE() for nodemask. To avoid to add comments all over the place. Can we implement a wrapper for it? For example, memcpy_once(). __read_once_size() in tools/include/linux/compiler.h can be used as reference. Because node_weights[] may be changed simultaneously too. We may need to consider similar issue for it too. But RCU seems more appropriate for node_weights[]. >> > + /* Otherwise we adjust nr_pages down, and continue from there */ >> > + rem_pages -= pol->wil.cur_weight; >> > + pol->wil.cur_weight = 0; >> > + prev_node = node; >> >> If pol->wil.cur_weight == 0, prev_node will be used without being >> initialized below. >> > > pol->wil.cur_weight is not used below. > >> > + } >> > + >> > + /* Now we can continue allocating as if from 0 instead of an offset */ >> > + rounds = rem_pages / weight_total; >> > + delta = rem_pages % weight_total; >> > + for (i = 0; i < nnodes; i++) { >> > + node = next_node_in(prev_node, nodes); >> > + weight = weights[node]; >> > + node_pages = weight * rounds; >> > + if (delta) { >> > + if (delta > weight) { >> > + node_pages += weight; >> > + delta -= weight; >> > + } else { >> > + node_pages += delta; >> > + delta = 0; >> > + } >> > + } >> > + /* We may not make it all the way around */ >> > + if (!node_pages) >> > + break; >> > + /* If an over-allocation would occur, floor it */ >> > + if (node_pages + total_allocated > nr_pages) { >> >> Why is this possible? >> > > this may have been a paranoid artifact from an early prototype, will > snip and validate. -- Best Regards, Huang, Ying