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

[Petr Mladek <pmladek@xxxxxxxx>]

On Fri 2023-06-23 12:11:11, Sebastian Andrzej Siewior wrote:
> On 2023-06-23 18:51:24 [+0900], Tetsuo Handa wrote:
> > My understanding is that zonelist_iter_begin() accesses only zonelist_update_seq.seqcount .
> >  From where is the zonelist_update_seq.lock accessed when zonelist_iter_begin() is called?
> 
> zonelist_iter_begin()
>  -> read_seqbegin()
>    -> read_seqcount_begin()
>      -> raw_read_seqcount_begin()
>        -> __read_seqcount_begin()
> 
> Now. The inner part looks like
>          while ((__seq = seqprop_sequence(s)) & 1)
>                  cpu_relax();
> 
> but with RT enabled (in-tree, not out-of-tree, in-tree) seqprop_sequence()
> goes from:

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?

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

Best Regards,
Petr


> | 	}
> | 	return seq;
> | }