Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: quoted-printable
From:   Qian Cai <cai@lca.pw>
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH -next v2] locking/osq_lock: annotate a data race in osq_lock
Date:   Mon, 11 May 2020 12:44:26 -0400
Message-Id: <D8F8F41A-F785-4E17-83CE-4101137ADC5B@lca.pw>
References: <20200511155812.GB22270@willie-the-truck>
Cc:     "Paul E. McKenney" <paulmck@kernel.org>,
        Elver Marco <elver@google.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Ingo Molnar <mingo@redhat.com>,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>
In-Reply-To: <20200511155812.GB22270@willie-the-truck>
To:     Will Deacon <will@kernel.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On May 11, 2020, at 11:58 AM, Will Deacon <will@kernel.org> wrote:
>=20
> I'm fine with the data_race() placement, but I don't find the comment
> very helpful. We assign the result of a READ_ONCE() to 'prev' in the
> loop, so I don't think that the cpu_relax() is really relevant.
>=20
> The reason we don't need READ_ONCE() here is because if we race with
> the writer then either we'll go round the loop again after accidentally
> thinking prev->next !=3D node, or we'll erroneously attempt the cmpxchg()
> because we thought they were equal and that will fail.
>=20
> Make sense?

I think the significant concern from the previous reviews was if compilers c=
ould prove that prev->next =3D=3D node was always true because it had no kno=
wledge of the concurrency, and then took out the whole if statement away res=
ulting in an infinite loop.

The comment tried to explain that the cpu_relax() would save us from the inf=
inite loop in theory here.=