On Wed, Oct 11, 2017 at 04:17:25PM +0100, David Howells wrote: > Will Deacon <will.deacon@xxxxxxx> wrote: > > > FWIW, that's exactly what my patches do, this fixup looks a bit weird > > because it removes a prior barrier which suggests that either (a) it's in > > the wrong place to start with, or (b) we're annotating the wrong load. > > There is a loop involved. The barrier is against the read in the previous > iteration of the loop. IIRC, the reason I did it this way is to avoid the > need for the barrier if there's nothing on the 'after-side' - ie. we examine > the pointer and see that it's NULL or a leaf. However, I'm not sure that's a > particularly necessary optimisation. Given that smp_read_barrier_depends() is nothingness on anything other than DEC Alpha, I would argue that this optimization is not necessary. > So if READ_ONCE() issues a smp_read_barrier_depends() after the read, then > I've no problem with the removal of these explicit barriers. Very good! > I will, however, quibble with the appropriateness of the name READ_ONCE()... > I still think it's not sufficiently obvious that this is a barrier and the > barrier is after. Maybe READ_AND_BARRIER()? Linus was unhappy with READ_ONCE_CTRL() to tag control dependencies, but indicated that he might consider it if it helped code-analysis tools. Adding Dmitry Vyukov for his thoughts on whether tagging READ_ONCE() for dependencies would help. Me, I would suggest READ_ONCE_DEP(), but let's figure out if the bikeshed needs to be painted before arguing over the color. ;-) > Also, does WRITE_ONCE() imply a preceding barrier? It does not. In most cases, the barriered version would be smp_store_release(). Thanx, Paul
