Re: [PATCH] mm/page_alloc: Use write_seqlock_irqsave() instead write_seqlock() + local_irq_save().

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

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. 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().

> Best Regards,
> Petr

Sebastian