Re: [RFC 0/2] srcu: Remove pre-flip memory barrier

Frederic Weisbecker
2022-12-20T12:44:47Z

On Tue, Dec 20, 2022 at 01:34:43PM +0100, Frederic Weisbecker wrote:
> On Mon, Dec 19, 2022 at 11:07:17PM -0500, Joel Fernandes wrote:
> > On Sun, Dec 18, 2022 at 2:13 PM Joel Fernandes (Google)
> > <joel@joelfernandes.org> wrote:
> > >
> > > Hello, I believe the pre-flip memory barrier is not required. The only reason I
> > > can say to remove it, other than the possibility that it is unnecessary, is to
> > > not have extra code that does not help. However, since we are issuing a fully
> > > memory-barrier after the flip, I cannot say that it hurts to do it anyway.
> > >
> > > For this reason, please consider these patches as "informational", than a
> > > "please merge". :-) Though, feel free to consider merging if you agree!
> > >
> > > All SRCU scenarios pass with these, with 6 hours of testing.
> > >
> > > thanks,
> > >
> > > - Joel
> > >
> > > Joel Fernandes (Google) (2):
> > > srcu: Remove comment about prior read lock counts
> > > srcu: Remove memory barrier "E" as it is not required
> >
> > And litmus tests confirm that "E" does not really do what the comments
> > say, PTAL:
> > Test 1:
> > C mbe
> > (*
> > * Result: sometimes
> > * Does previous scan see old reader's lock count, if a new reader saw
> > the new srcu_idx?
> > *)
> >
> > {}
> >
> > P0(int *lockcount, int *srcu_idx) // updater
> > {
> > int r0;
> > r0 = READ_ONCE(*lockcount);
> > smp_mb(); // E
> > WRITE_ONCE(*srcu_idx, 1);
> > }
> >
> > P1(int *lockcount, int *srcu_idx) // reader
> > {
> > int r0;
> > WRITE_ONCE(*lockcount, 1); // previous reader
> > smp_mb(); // B+C
> > r0 = READ_ONCE(*srcu_idx); // new reader
> > }
> > exists (0:r0=0 /\ 1:r0=1) (* Bad outcome. *)
> >
> > Test 2:
> > C mbe2
> >
> > (*
> > * Result: sometimes
> > * If updater saw reader's lock count, was that reader using the old idx?
> > *)
> >
> > {}
> >
> > P0(int *lockcount, int *srcu_idx) // updater
> > {
> > int r0;
> > r0 = READ_ONCE(*lockcount);
> > smp_mb(); // E
> > WRITE_ONCE(*srcu_idx, 1);
> > }
> >
> > P1(int *lockcount, int *srcu_idx) // reader
> > {
> > int r0;
> > int r1;
> > r1 = READ_ONCE(*srcu_idx); // previous reader
> > WRITE_ONCE(*lockcount, 1); // previous reader
> > smp_mb(); // B+C
> > r0 = READ_ONCE(*srcu_idx); // new reader
> > }
> > exists (0:r0=1 /\ 1:r1=1) (* Bad outcome. *)
>
> Actually, starring at this some more, there is some form of dependency
> on the idx in order to build the address where the reader must write the
> lockcount to. Litmus doesn't support arrays but assuming that
> &ssp->sda->srcu_lock_count == 0 (note the & in the beginning), it
> could be modelized that way (I'm eluding the unlock part to simplify):
>
> ---
> C w-depend-r
>
> {
> PLOCK=LOCK0;
> }
>
> // updater
> P0(int *LOCK0, int *LOCK1, int **PLOCK)
> {
> int lock1;
>
> lock1 = READ_ONCE(*LOCK1); // READ from inactive idx
> smp_mb();
> WRITE_ONCE(*PLOCK, LOCK1); // Flip idx
> }
>
> // reader
> P1(int **PLOCK)
> {
> int *plock;
>
> plock = READ_ONCE(*PLOCK); // Read active idx
> WRITE_ONCE(*plock, 1); // Write to active idx
> }
>
> exists (0:lock0=1) // never happens

That's lock1=1, lemme do it again:

C w-depend-r

{
PLOCK=LOCK0;
}

// updater
P0(int *LOCK1, int **PLOCK)
{
int lock1;

lock1 = READ_ONCE(*LOCK1); // READ from inactive idx
smp_mb();
WRITE_ONCE(*PLOCK, LOCK1); // Flip idx
}

// reader
P1(int **PLOCK)
{
int *plock;

plock = READ_ONCE(*PLOCK); // Read active idx
WRITE_ONCE(*plock, 1); // Write to active idx
}

exists (0:lock1=1) (* never *)