Messages in this thread Patch in this message | | | Date | Wed, 8 Sep 2021 16:41:10 +0200 | From | Peter Zijlstra <> | Subject | Re: [RFC] locking: rwbase: Take care of ordering guarantee for fastpath reader |
| |
On Wed, Sep 08, 2021 at 09:08:52PM +0800, Boqun Feng wrote: > On Wed, Sep 08, 2021 at 01:51:24PM +0200, Peter Zijlstra wrote: > [...] > > @@ -201,23 +207,30 @@ static int __sched rwbase_write_lock(struct rwbase_rt *rwb, > > { > > struct rt_mutex_base *rtm = &rwb->rtmutex; > > unsigned long flags; > > + int readers; > > > > /* Take the rtmutex as a first step */ > > if (rwbase_rtmutex_lock_state(rtm, state)) > > return -EINTR; > > > > /* Force readers into slow path */ > > - atomic_sub(READER_BIAS, &rwb->readers); > > + readers = atomic_sub_return_relaxed(READER_BIAS, &rwb->readers); > > > > - raw_spin_lock_irqsave(&rtm->wait_lock, flags); > > /* > > * set_current_state() for rw_semaphore > > * current_save_and_set_rtlock_wait_state() for rwlock > > */ > > rwbase_set_and_save_current_state(state); > > rwbase_set_and_save_current_state() may eventually call > current_save_and_set_rtlock_wait_state(), which requires being called > with irq-off, while rwbase_write_lock() may be called with irq-on. I > guess we can change the raw_spin_lock() to raw_spin_lock_irqsave() in > current_save_and_set_rtlock_wait_state() to solve this.
Oh right... that's actually something I pointed out to Thomas during review, and I suppose we both forgot about it, or figured it didn't matter enough.
Oooh, Thomas added that lockdep_assert.. still lemme change that to match set_special_state().
Also,...
--- Subject: sched/wakeup: Strengthen current_save_and_set_rtlock_wait_state()
While looking at current_save_and_set_rtlock_wait_state() I'm thinking it really ought to use smp_store_mb(), because something like:
current_save_and_set_rtlock_wait_state(); for (;;) { if (try_lock()) break; raw_spin_unlock_irq(&lock->wait_lock); if (!cond) schedule(); raw_spin_lock_irq(&lock->wait_lock); set_current_state(TASK_RTLOCK_WAIT); } current_restore_rtlock_saved_state();
which is very close to the advertised usage in the comment, is actually broken I think:
- try_lock() doesn't need to provide any ordering on failure; - raw_spin_unlock() only needs to provide RELEASE ordering;
which gives that the above turns into something like:
WRITE_ONCE(current->__state, TASK_RTLOCK_WAIT); raw_spin_unlock(¤t->pi_lock); raw_spin_unlock(&lock->wait_lock); if (!cond)
and the load of @cond is then allowed to speculate right before the __state store, and we've got a missed wakeup -> BAD(tm).
Fixes: 5f220be21418 ("sched/wakeup: Prepare for RT sleeping spin/rwlocks") Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- diff --git a/include/linux/sched.h b/include/linux/sched.h index 1780260f237b..3d3246d7e87d 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -245,7 +245,8 @@ struct task_group; * if (try_lock()) * break; * raw_spin_unlock_irq(&lock->wait_lock); - * schedule_rtlock(); + * if (!cond) + * schedule_rtlock(); * raw_spin_lock_irq(&lock->wait_lock); * set_current_state(TASK_RTLOCK_WAIT); * } @@ -253,22 +254,24 @@ struct task_group; */ #define current_save_and_set_rtlock_wait_state() \ do { \ - lockdep_assert_irqs_disabled(); \ - raw_spin_lock(¤t->pi_lock); \ + unsigned long flags; /* may shadow */ \ + \ + raw_spin_lock_irqsave(¤t->pi_lock, flags); \ current->saved_state = current->__state; \ debug_rtlock_wait_set_state(); \ - WRITE_ONCE(current->__state, TASK_RTLOCK_WAIT); \ - raw_spin_unlock(¤t->pi_lock); \ + smp_store_mb(current->__state, TASK_RTLOCK_WAIT); \ + raw_spin_unlock_irqrestore(¤t->pi_lock, flags); \ } while (0); #define current_restore_rtlock_saved_state() \ do { \ - lockdep_assert_irqs_disabled(); \ - raw_spin_lock(¤t->pi_lock); \ + unsigned long flags; /* may shadow */ \ + \ + raw_spin_lock_irqsave(¤t->pi_lock, flags); \ debug_rtlock_wait_restore_state(); \ WRITE_ONCE(current->__state, current->saved_state); \ current->saved_state = TASK_RUNNING; \ - raw_spin_unlock(¤t->pi_lock); \ + raw_spin_unlock_irqrestore(¤t->pi_lock, flags); \ } while (0); #define get_current_state() READ_ONCE(current->__state)
| |