Daniel Xu wrote:
> Hi Akira,
>
> Thanks for taking a look!
>
> On Tue, Dec 17, 2024, at 5:53 PM, Akira Yokosawa wrote:
>> Hello,
>>
>> Daniel Xu wrote:
>>> WRITE_ONCE() is needed here to prevent store tears and other unwanted
>>> compiler optimizations.
>>
>> That might be true if there were chances of these two accesses to
>> race with each other.
>>
>> I don't see any possibility of such races.
>>
>> Can you elaborate?
>
> My understanding is that read_seqbegin() and read_seqretry()
> can execute at any time. That means the read side access of
> the sequence number can occur doing an increment.
>
> To prevent the reader from reading a partially written value,
> we need the WRITE_ONCE() to ensure the relaxed atomic
> write.
OK, now I see the race there.
But see below.
> --- a/CodeSamples/defer/seqlock.h
> +++ b/CodeSamples/defer/seqlock.h
> @@ -60,14 +60,14 @@ static inline int read_seqretry(seqlock_t *slp, //\lnlbl{read_seqretry:b}
> static inline void write_seqlock(seqlock_t *slp) //\lnlbl{write_seqlock:b}
> {
> spin_lock(&slp->lock);
> - ++slp->seq;
> + WRITE_ONCE(slp->seq, READ_ONCE(slp->seq) + 1);
^^^^^^^^^
> smp_mb();
> } //\lnlbl{write_seqlock:e}
>
> static inline void write_sequnlock(seqlock_t *slp) //\lnlbl{write_sequnlock:b}
> {
> smp_mb(); //\lnlbl{write_sequnlock:mb}
> - ++slp->seq; //\lnlbl{write_sequnlock:inc}
> + WRITE_ONCE(slp->seq, READ_ONCE(slp->seq) + 1); //\lnlbl{write_sequnlock:inc}
^^^^^^^^^
> spin_unlock(&slp->lock);
> } //\lnlbl{write_sequnlock:e}
> //\end{snippet}
Those two READ_ONCE()s don't have any concurrent writes.
So there seems to be no need of them, IIUC.
Thanks, Akira
