[I sent this reply earlier, but since it hasn't shown up in the mailing
list archives, I may have forgotten to include the proper CC's. At the
risk of repeating myself, here it is again.]
On Fri, Oct 16, 2020 at 11:19:41PM -0400, joel@xxxxxxxxxxxxxxxxx wrote:
> So I made a litmus test to show that smp_mb() is needed also after the update
> to length. Basically, otherwise it is possible the callback will see garbage
> that the module cleanup/unload did.
>
> C rcubarrier+ctrldep
>
> (*
> * Result: Never
> *
> * This litmus test shows that rcu_barrier (P1) prematurely
> * returning by reading len 0 can cause issues if P0 does
> * NOT have a smb_mb() after WRITE_ONCE(len, 1).
> * mod_data == 2 means module was unloaded (so data is garbage).
> *)
>
> { int len = 0; int enq = 0; }
>
> P0(int *len, int *mod_data, int *enq)
> {
> int r0;
>
> WRITE_ONCE(*len, 1);
> smp_mb(); /* Needed! */
> WRITE_ONCE(*enq, 1);
>
> r0 = READ_ONCE(*mod_data);
> }
>
> P1(int *len, int *mod_data, int *enq)
> {
> int r0;
> int r1;
>
> r1 = READ_ONCE(*enq);
>
> // barrier Just for test purpose ("exists" clause) to force the..
> // ..rcu_barrier() to see enq before len
> smp_mb();
> r0 = READ_ONCE(*len);
>
> // implicit memory barrier due to conditional */
> if (r0 == 0)
> WRITE_ONCE(*mod_data, 2);
> }
>
> // Did P0 read garbage?
> exists (0:r0=2 /\ 1:r0=0 /\ 1:r1=1)
Is this exists clause really what you meant? Not only can it not be
satisfied, it couldn't even be satisfied if you left out the 0:r0=2
part. And smp_mb() is stronger than neessary to enforce this.
However, some memory barrier is needed. If the smp_mb() in P1 were
omitted then P1 would be free to reorder its reads, and the exists
clause could be satisfied as follows:
P0 P1
------------------------------------------
Read len = 0
Write len = 1
smp_mb();
Write enq = 1
Read enq = 1
Write mod_data = 2
Read mod_data = 2
Alan
