Re: [PATCH 2/2] locking/rwbase_rt: Lockless reader waking up a writer

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, Sep 13 2021 at 14:20, Thomas Gleixner wrote:

> On Wed, Sep 01 2021 at 15:28, Davidlohr Bueso wrote:
>> diff --git a/kernel/locking/rwbase_rt.c b/kernel/locking/rwbase_rt.c
>> index 4ba15088e640..3444bc709973 100644
>> --- a/kernel/locking/rwbase_rt.c
>> +++ b/kernel/locking/rwbase_rt.c
>> @@ -141,6 +141,7 @@ static void __sched __rwbase_read_unlock(struct rwbase_rt *rwb,
>>  {
>>  	struct rt_mutex_base *rtm = &rwb->rtmutex;
>>  	struct task_struct *owner;
>> +	DEFINE_WAKE_Q(wake_q);
>>  
>>  	raw_spin_lock_irq(&rtm->wait_lock);
>>  	/*
>> @@ -151,9 +152,10 @@ static void __sched __rwbase_read_unlock(struct rwbase_rt *rwb,
>>  	 */
>>  	owner = rt_mutex_owner(rtm);
>>  	if (owner)
>> -		wake_up_state(owner, state);
>> +		wake_q_add(&wake_q, owner);
>
> That's broken for rw_locks. See commit 456cfbc65cd072f4f53936ee5a37eb1447a7d3ba.

Something like the untested below should work.

Thanks,

        tglx
---
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 6bb116c559b4..9e04bca0c11e 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -459,6 +459,20 @@ static __always_inline void rt_mutex_wake_q_add(struct rt_wake_q_head *wqh,
 	}
 }
 
+static __always_inline void rt_mutex_wake_q_add_task(struct rt_wake_q_head *wqh,
+						     struct task_struct *task,
+						     unsigned int wake_state)
+{
+	if (IS_ENABLED(CONFIG_PREEMPT_RT) && wake_state != TASK_NORMAL) {
+		if (IS_ENABLED(CONFIG_PROVE_LOCKING))
+			WARN_ON_ONCE(wqh->rtlock_task);
+		get_task_struct(task);
+		wqh->rtlock_task = task;
+	} else {
+		wake_q_add(&wqh->head, task);
+	}
+}
+
 static __always_inline void rt_mutex_wake_up_q(struct rt_wake_q_head *wqh)
 {
 	if (IS_ENABLED(CONFIG_PREEMPT_RT) && wqh->rtlock_task) {
diff --git a/kernel/locking/rwbase_rt.c b/kernel/locking/rwbase_rt.c
index 4ba15088e640..e011b347a2c5 100644
--- a/kernel/locking/rwbase_rt.c
+++ b/kernel/locking/rwbase_rt.c
@@ -141,8 +141,10 @@ static void __sched __rwbase_read_unlock(struct rwbase_rt *rwb,
 {
 	struct rt_mutex_base *rtm = &rwb->rtmutex;
 	struct task_struct *owner;
+	DEFINE_RT_WAKE_Q(wqh);
 
 	raw_spin_lock_irq(&rtm->wait_lock);
+
 	/*
 	 * Wake the writer, i.e. the rtmutex owner. It might release the
 	 * rtmutex concurrently in the fast path (due to a signal), but to
@@ -151,9 +153,12 @@ static void __sched __rwbase_read_unlock(struct rwbase_rt *rwb,
 	 */
 	owner = rt_mutex_owner(rtm);
 	if (owner)
-		wake_up_state(owner, state);
+		rt_mutex_wake_q_add_task(&wqh, owner, state);
 
+	/* Pairs with the preempt_enable in rt_mutex_wake_up_q() */
+	preempt_disable();
 	raw_spin_unlock_irq(&rtm->wait_lock);
+	rt_mutex_wake_up_q(&wqh);
 }
 
 static __always_inline void rwbase_read_unlock(struct rwbase_rt *rwb,