On Fri 2023-06-23 15:31:27, Sebastian Andrzej Siewior wrote:
> On 2023-06-23 12:53:57 [+0200], Petr Mladek wrote:
> > BTW: I see a possible race in the below code.
> >
> > > | unsigned __seqprop_spinlock_sequence(const seqcount_spinlock_t *s)
> > > | {
> > > | unsigned seq = READ_ONCE(s->seqcount.sequence);
> > > |
> > > | if (unlikely(seq & 1)) {
> > > | spin_lock(s->lock);
> > > | spin_unlock(s->lock);
> >
> > This guarantees the reader waits for the writer. But does this
> > guarantee that another writer could not bump the sequence
> > before we read it below?
>
> A second writer can bump the seq after the first is done, correct.
>
> > > | seq = READ_ONCE(s->seqcount.sequence);
> >
> > IMHO, it should be:
> >
> > if (unlikely(seq & 1)) {
> > spin_lock(s->lock);
> > seq = READ_ONCE(s->seqcount.sequence);
> > spin_unlock(s->lock);
> >
> > IMHO, only this would guarantee that returned seq couldn't be odd.
>
> This doesn't buy us anything.
Sure?
> If the writer increments the counter,
> after the reader unlocks, then the reader observers the odd seq number
> and locks immediately (within read_seqbegin()) on the lock without
> spending cycles within the read section (outside of read_seqbegin()) and
> learning about the seq increment in read_seqretry().
I am afraid that it will not get caugh:
static inline int do___read_seqcount_retry(const seqcount_t *s, unsigned start)
{
kcsan_atomic_next(0);
return unlikely(READ_ONCE(s->sequence) != start);
}
It checks if the sequence is the same. It does not check if it is odd.
But it might be odd if you read "start" outsize of the lock and do
not check if it is odd.
I do not feel to be expert but a code:
spin_lock(s->lock);
spin_unlock(s->lock);
s = locked_variable;
just triggers many bells in my head. It is like from a book
"Classic locking mistakes".
With the code:
spin_lock(s->lock);
seq s->seqcount.sequence;
spin_unlock(s->lock);
you will always get the right value without any extra cost. The code
took the lock anyway. And the single assignment is negligible.
And RT could schedule inside the spin_lock anyway.
Or do I miss anything?
Best Regards,
Petr
