Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   Joel Fernandes <joel@joelfernandes.org>
Mime-Version: 1.0 (1.0)
Subject: Re: [RFC 0/2] srcu: Remove pre-flip memory barrier
Date:   Tue, 20 Dec 2022 08:44:40 -0500
Message-Id: <D358FF0F-6F7C-413A-8CA6-7D36E9A64DB6@joelfernandes.org>
References: <20221220124033.GA22763@lothringen>
Cc:     linux-kernel@vger.kernel.org,
        Josh Triplett <josh@joshtriplett.org>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        "Paul E. McKenney" <paulmck@kernel.org>, rcu@vger.kernel.org,
        Steven Rostedt <rostedt@goodmis.org>
In-Reply-To: <20221220124033.GA22763@lothringen>
To:     Frederic Weisbecker <frederic@kernel.org>
Precedence: bulk


> On Dec 20, 2022, at 7:40 AM, Frederic Weisbecker <frederic@kernel.org> wro=
te:
>=20
> =EF=BB=BFOn Tue, Dec 20, 2022 at 01:34:43PM +0100, Frederic Weisbecker wro=
te:
>>> 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:
>>>>=20
>>>> Hello, I believe the pre-flip memory barrier is not required. The only r=
eason 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 anyw=
ay.
>>>>=20
>>>> For this reason, please consider these patches as "informational", than=
 a
>>>> "please merge". :-) Though, feel free to consider merging if you agree!=

>>>>=20
>>>> All SRCU scenarios pass with these, with 6 hours of testing.
>>>>=20
>>>> thanks,
>>>>=20
>>>> - Joel
>>>>=20
>>>> Joel Fernandes (Google) (2):
>>>> srcu: Remove comment about prior read lock counts
>>>> srcu: Remove memory barrier "E" as it is not required
>>>=20
>>> 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?
>>> *)
>>>=20
>>> {}
>>>=20
>>> P0(int *lockcount, int *srcu_idx) // updater
>>> {
>>>        int r0;
>>>        r0 =3D READ_ONCE(*lockcount);
>>>        smp_mb();       // E
>>>        WRITE_ONCE(*srcu_idx, 1);
>>> }
>>>=20
>>> P1(int *lockcount, int *srcu_idx) // reader
>>> {
>>>        int r0;
>>>        WRITE_ONCE(*lockcount, 1); // previous reader
>>>        smp_mb();       // B+C
>>>        r0 =3D READ_ONCE(*srcu_idx); // new reader
>>> }
>>> exists (0:r0=3D0 /\ 1:r0=3D1) (* Bad outcome. *)
>>>=20
>>> Test 2:
>>> C mbe2
>>>=20
>>> (*
>>> * Result: sometimes
>>> * If updater saw reader's lock count, was that reader using the old idx?=

>>> *)
>>>=20
>>> {}
>>>=20
>>> P0(int *lockcount, int *srcu_idx) // updater
>>> {
>>>        int r0;
>>>        r0 =3D READ_ONCE(*lockcount);
>>>        smp_mb();       // E
>>>        WRITE_ONCE(*srcu_idx, 1);
>>> }
>>>=20
>>> P1(int *lockcount, int *srcu_idx) // reader
>>> {
>>>        int r0;
>>>        int r1;
>>>        r1 =3D READ_ONCE(*srcu_idx); // previous reader
>>>        WRITE_ONCE(*lockcount, 1); // previous reader
>>>        smp_mb();       // B+C
>>>        r0 =3D READ_ONCE(*srcu_idx); // new reader
>>> }
>>> exists (0:r0=3D1 /\ 1:r1=3D1) (* Bad outcome. *)
>>=20
>> 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 =3D=3D 0 (note the & in the beginning), it
>> could be modelized that way (I'm eluding the unlock part to simplify):
>>=20
>> ---
>> C w-depend-r
>>=20
>> {
>>    PLOCK=3DLOCK0;
>> }
>>=20
>> // updater
>> P0(int *LOCK0, int *LOCK1, int **PLOCK)
>> {
>>    int lock1;
>>=20
>>    lock1 =3D READ_ONCE(*LOCK1); // READ from inactive idx
>>    smp_mb();
>>    WRITE_ONCE(*PLOCK, LOCK1); // Flip idx
>> }
>>=20
>> // reader
>> P1(int **PLOCK)
>> {
>>    int *plock;
>>=20
>>    plock =3D READ_ONCE(*PLOCK);    // Read active idx
>>    WRITE_ONCE(*plock, 1); // Write to active idx
>> }
>>=20
>> exists (0:lock0=3D1) // never happens
>=20
> That's lock1=3D1, lemme do it again:
>=20
> C w-depend-r
>=20
> {
>    PLOCK=3DLOCK0;
> }
>=20
> // updater
> P0(int *LOCK1, int **PLOCK)
> {
>    int lock1;
>=20
>    lock1 =3D READ_ONCE(*LOCK1); // READ from inactive idx
>    smp_mb();
>    WRITE_ONCE(*PLOCK, LOCK1); // Flip idx
> }
>=20
> // reader
> P1(int **PLOCK)
> {
>    int *plock;
>=20
>    plock =3D READ_ONCE(*PLOCK);    // Read active idx
>    WRITE_ONCE(*plock, 1); // Write to active idx

I am a bit lost here, why would the reader want to write to the active idx? T=
he reader does not update the idx, only the lock count.

Further, the comment does not talk about implicit memory ordering, it=E2=80=99=
s talking about explicit ordering due to B+C on one side, and E on the other=
.

Thanks!

- Joel


> }
>=20
> exists (0:lock1=3D1) (* never *)