Messages in this thread | | | From | Dmitry Vyukov <> | Date | Thu, 24 Oct 2019 13:51:20 +0200 | Subject | Re: [PATCH v6] taskstats: fix data-race |
| |
On Thu, Oct 24, 2019 at 1:32 PM Andrea Parri <parri.andrea@gmail.com> wrote: > > > How these later loads can be completely independent of the pointer > > value? They need to obtain the pointer value from somewhere. And this > > can only be done by loaded it. And if a thread loads a pointer and > > then dereferences that pointer, that's a data/address dependency and > > we assume this is now covered by READ_ONCE. > > The "dependency" I was considering here is a dependency _between the > load of sig->stats in taskstats_tgid_alloc() and the (program-order) > later loads of *(sig->stats) in taskstats_exit(). Roughly speaking, > such a dependency should correspond to a dependency chain at the asm > or registers level from the first load to the later loads; e.g., in: > > Thread [register r0 contains the address of sig->stats] > > A: LOAD r1,[r0] // LOAD_ACQUIRE sig->stats > ... > B: LOAD r2,[r0] // LOAD *(sig->stats) > C: LOAD r3,[r2] > > there would be no such dependency from A to C. Compare, e.g., with: > > Thread [register r0 contains the address of sig->stats] > > A: LOAD r1,[r0] // LOAD_ACQUIRE sig->stats > ... > C: LOAD r3,[r1] // LOAD *(sig->stats) > > AFAICT, there's no guarantee that the compilers will generate such a > dependency from the code under discussion.
Fixing this by making A ACQUIRE looks like somewhat weird code pattern to me (though correct). B is what loads the address used to read indirect data, so B ought to be ACQUIRE (or LOAD-DEPENDS which we get from READ_ONCE).
What you are suggesting is:
addr = ptr.load(memory_order_acquire); if (addr) { addr = ptr.load(memory_order_relaxed); data = *addr; }
whereas the canonical/non-convoluted form of this pattern is:
addr = ptr.load(memory_order_consume); if (addr) data = *addr;
Moreover the second load of ptr is not even atomic in our case, so it is a subject to another data race?
| |