On Tue, Sep 22, 2020 at 11:42:43AM -0700, Paul E. McKenney wrote:
On Tue, Sep 22, 2020 at 09:51:36AM +1000, Herbert Xu wrote:
On Mon, Sep 21, 2020 at 04:26:39PM -0700, Paul E. McKenney wrote:
But this reasoning could apply to any data structure that contains a spin lock, in particular ones that are dereferenced through RCU.
I lost you on this one. What is special about a spin lock?
I don't know, that was Eric's concern. He is inferring that spin locks through lockdep debugging may trigger dependencies that require smp_load_acquire.
Anyway, my point is if it applies to crng_node_pool then it would equally apply to RCU in general.
Referring to the patch you call out below...
Huh. The old cmpxchg() primitive is fully ordered, so the old mb() preceding it must have been for correctly interacting with hardware on !SMP systems. If that is the case, then the use of cmpxchg_release() is incorrect. This is not the purview of the memory model, but rather of device-driver semantics. Or does crng not (or no longer, as the case might be) interact with hardware RNGs?
No hardware involved here. The mb() is just unnecessary, as I noted in my patch https://lore.kernel.org/lkml/20200916233042.51634-1-ebiggers@kernel.org/.
What prevents either the old or the new code from kfree()ing the old state out from under another CPU that just now picked up a pointer to the old state? The combination of cmpxchg_release() and smp_load_acquire() won't do anything to prevent this from happening. This is after all not a memory-ordering issue, but instead an object-lifetime issue. But maybe you have a lock or something that provides the needed protection. I don't see how this can be the case and still require the cmpxchg_release() and smp_load_acquire(), but perhaps this is a failure of imagination on my part.
crng_node_pool is initialized only once, and never freed.
- Eric