Hi Paul,
On Mon, Dec 20, 2021 at 8:00 PM Paul E. McKenney paulmck@kernel.org wrote:
This assumes that the various crng_node_pool[i] pointers never change while accessible to readers (and that some sort of synchronization applies to the values in the pointed-to structure). If these pointers do change, then there also needs to be a READ_ONCE(pool[nid]) in select_crng(), where the value returned from this READ_ONCE() is both tested and returned. (As in assign this value to a temporary.)
But if the various crng_node_pool[i] pointers really are constant while readers can access them, then the cmpxchg_release() suffices. The loads from pool[nid] are then data-race free, and because they are unmarked, the compiler is prohibited from hoisting them out from within the "if" statement. The address dependency prohibits the CPU from reordering them.
Right, this is just an initialization-time allocation and assignment, never updated or freed again after.
So READ_ONCE() should be just fine. Which answers Jason's question. ;-)
Great. So v2 of this patch can use READ_ONCE then. Thanks!
Looking at _extract_crng(), if this was my code, I would use READ_ONCE() in the checks, but that might be my misunderstanding boot-time constraints or some such. Without some sort of constraint, I don't see how the code avoids confusion from reloads of crng->init_time if two CPUs concurrently see the expiration of CRNG_RESEED_INTERVAL, but I could easily be missing something that makes this safe. (And this is irrelevant to this patch.)
Indeed init_time seems to race via the crng_reseed path, and READ_ONCE()ing that seems reasonable. The other setters of it -- initialize_{primary,secondary} -- are in the boot path.
Jason