kernel/locking/rwsem.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
From: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
Make the region inside the rwsem_write_trylock non preemptible.
We observe RT task is hogging CPU when trying to acquire rwsem lock
which was acquired by a kworker task but before the rwsem owner was set.
Here is the scenario:
1. CFS task (affined to a particular CPU) takes rwsem lock.
2. CFS task gets preempted by a RT task before setting owner.
3. RT task (FIFO) is trying to acquire the lock, but spinning until
RT throttling happens for the lock as the lock was taken by CFS task.
This patch attempts to fix the above issue by disabling preemption
until owner is set for the lock. while at it also fix this issue
at the place where owner being set/cleared.
Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
kernel/locking/rwsem.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 65f0262..3b4b32e 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -251,13 +251,16 @@ static inline bool rwsem_read_trylock(struct rw_semaphore *sem, long *cntp)
static inline bool rwsem_write_trylock(struct rw_semaphore *sem)
{
long tmp = RWSEM_UNLOCKED_VALUE;
+ bool ret = false;
+ preempt_disable();
if (atomic_long_try_cmpxchg_acquire(&sem->count, &tmp, RWSEM_WRITER_LOCKED)) {
rwsem_set_owner(sem);
- return true;
+ ret = true;
}
- return false;
+ preempt_enable();
+ return ret;
}
/*
@@ -686,16 +689,21 @@ enum owner_state {
static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
{
long count = atomic_long_read(&sem->count);
+ bool ret = false;
+ preempt_disable();
while (!(count & (RWSEM_LOCK_MASK|RWSEM_FLAG_HANDOFF))) {
if (atomic_long_try_cmpxchg_acquire(&sem->count, &count,
count | RWSEM_WRITER_LOCKED)) {
rwsem_set_owner(sem);
lockevent_inc(rwsem_opt_lock);
- return true;
+ ret = true;
+ break;
}
}
- return false;
+
+ preempt_enable();
+ return ret;
}
static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
@@ -1352,8 +1360,10 @@ static inline void __up_write(struct rw_semaphore *sem)
DEBUG_RWSEMS_WARN_ON((rwsem_owner(sem) != current) &&
!rwsem_test_oflags(sem, RWSEM_NONSPINNABLE), sem);
+ preempt_disable();
rwsem_clear_owner(sem);
tmp = atomic_long_fetch_add_release(-RWSEM_WRITER_LOCKED, &sem->count);
+ preempt_enable();
if (unlikely(tmp & RWSEM_FLAG_WAITERS))
rwsem_wake(sem);
}
--
2.7.4On Thu, Sep 01, 2022 at 03:58:10PM +0530, Mukesh Ojha wrote:
> From: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
>
> Make the region inside the rwsem_write_trylock non preemptible.
>
> We observe RT task is hogging CPU when trying to acquire rwsem lock
> which was acquired by a kworker task but before the rwsem owner was set.
>
> Here is the scenario:
> 1. CFS task (affined to a particular CPU) takes rwsem lock.
>
> 2. CFS task gets preempted by a RT task before setting owner.
>
> 3. RT task (FIFO) is trying to acquire the lock, but spinning until
> RT throttling happens for the lock as the lock was taken by CFS task.
>
> This patch attempts to fix the above issue by disabling preemption
> until owner is set for the lock. while at it also fix this issue
> at the place where owner being set/cleared.
>
> Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
This is not a valid SoB chain.
> ---
> kernel/locking/rwsem.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
> index 65f0262..3b4b32e 100644
> --- a/kernel/locking/rwsem.c
> +++ b/kernel/locking/rwsem.c
> @@ -251,13 +251,16 @@ static inline bool rwsem_read_trylock(struct rw_semaphore *sem, long *cntp)
> static inline bool rwsem_write_trylock(struct rw_semaphore *sem)
> {
> long tmp = RWSEM_UNLOCKED_VALUE;
> + bool ret = false;
>
> + preempt_disable();
> if (atomic_long_try_cmpxchg_acquire(&sem->count, &tmp, RWSEM_WRITER_LOCKED)) {
> rwsem_set_owner(sem);
> - return true;
> + ret = true;
> }
>
> - return false;
> + preempt_enable();
> + return ret;
> }
>
> /*
Yes, this part looks ok.
> @@ -686,16 +689,21 @@ enum owner_state {
> static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
> {
> long count = atomic_long_read(&sem->count);
> + bool ret = false;
>
> + preempt_disable();
> while (!(count & (RWSEM_LOCK_MASK|RWSEM_FLAG_HANDOFF))) {
> if (atomic_long_try_cmpxchg_acquire(&sem->count, &count,
> count | RWSEM_WRITER_LOCKED)) {
> rwsem_set_owner(sem);
> lockevent_inc(rwsem_opt_lock);
> - return true;
> + ret = true;
> + break;
> }
> }
> - return false;
> +
> + preempt_enable();
> + return ret;
> }
>
This one I can't follow; afaict this is only called with preemption
already disabled.
> static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
> @@ -1352,8 +1360,10 @@ static inline void __up_write(struct rw_semaphore *sem)
> DEBUG_RWSEMS_WARN_ON((rwsem_owner(sem) != current) &&
> !rwsem_test_oflags(sem, RWSEM_NONSPINNABLE), sem);
>
> + preempt_disable();
> rwsem_clear_owner(sem);
> tmp = atomic_long_fetch_add_release(-RWSEM_WRITER_LOCKED, &sem->count);
> + preempt_enable();
> if (unlikely(tmp & RWSEM_FLAG_WAITERS))
> rwsem_wake(sem);
> }
Yep, that looks good again.
Perhaps the thing to do would be to add:
lockdep_assert_preemption_disabled()
to rwsem_{set,clear}_owner() and expand the comment there to explain
that these functions should be in the same preempt-disable section as
the atomic op that changes sem->count.
Hi Peter,
Thanks for your time in reviewing this patch.
On 9/8/2022 8:02 PM, Peter Zijlstra wrote:
> On Thu, Sep 01, 2022 at 03:58:10PM +0530, Mukesh Ojha wrote:
>> From: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
>>
>> Make the region inside the rwsem_write_trylock non preemptible.
>>
>> We observe RT task is hogging CPU when trying to acquire rwsem lock
>> which was acquired by a kworker task but before the rwsem owner was set.
>>
>> Here is the scenario:
>> 1. CFS task (affined to a particular CPU) takes rwsem lock.
>>
>> 2. CFS task gets preempted by a RT task before setting owner.
>>
>> 3. RT task (FIFO) is trying to acquire the lock, but spinning until
>> RT throttling happens for the lock as the lock was taken by CFS task.
>>
>> This patch attempts to fix the above issue by disabling preemption
>> until owner is set for the lock. while at it also fix this issue
>> at the place where owner being set/cleared.
>>
>> Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>
> This is not a valid SoB chain.
Since this patch of adding preempt disable() at rwsem_write_trylock() is
originated from Gokul.
Would be adding him in
Original-patch-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
Convert myself to the author/SoB.
Would that be fine ? please suggest.
>
>> ---
>> kernel/locking/rwsem.c | 18 ++++++++++++++----
>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
>> index 65f0262..3b4b32e 100644
>> --- a/kernel/locking/rwsem.c
>> +++ b/kernel/locking/rwsem.c
>> @@ -251,13 +251,16 @@ static inline bool rwsem_read_trylock(struct rw_semaphore *sem, long *cntp)
>> static inline bool rwsem_write_trylock(struct rw_semaphore *sem)
>> {
>> long tmp = RWSEM_UNLOCKED_VALUE;
>> + bool ret = false;
>>
>> + preempt_disable();
>> if (atomic_long_try_cmpxchg_acquire(&sem->count, &tmp, RWSEM_WRITER_LOCKED)) {
>> rwsem_set_owner(sem);
>> - return true;
>> + ret = true;
>> }
>>
>> - return false;
>> + preempt_enable();
>> + return ret;
>> }
>>
>> /*
>
> Yes, this part looks ok.
>
>> @@ -686,16 +689,21 @@ enum owner_state {
>> static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
>> {
>> long count = atomic_long_read(&sem->count);
>> + bool ret = false;
>>
>> + preempt_disable();
>> while (!(count & (RWSEM_LOCK_MASK|RWSEM_FLAG_HANDOFF))) {
>> if (atomic_long_try_cmpxchg_acquire(&sem->count, &count,
>> count | RWSEM_WRITER_LOCKED)) {
>> rwsem_set_owner(sem);
>> lockevent_inc(rwsem_opt_lock);
>> - return true;
>> + ret = true;
>> + break;
>> }
>> }
>> - return false;
>> +
>> + preempt_enable();
>> + return ret;
>> }
>>
>
> This one I can't follow; afaict this is only called with preemption
> already disabled.
Agreed. Will remove it in v2.
>
>> static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
>> @@ -1352,8 +1360,10 @@ static inline void __up_write(struct rw_semaphore *sem)
>> DEBUG_RWSEMS_WARN_ON((rwsem_owner(sem) != current) &&
>> !rwsem_test_oflags(sem, RWSEM_NONSPINNABLE), sem);
>>
>> + preempt_disable();
>> rwsem_clear_owner(sem);
>> tmp = atomic_long_fetch_add_release(-RWSEM_WRITER_LOCKED, &sem->count);
>> + preempt_enable();
>> if (unlikely(tmp & RWSEM_FLAG_WAITERS))
>> rwsem_wake(sem);
>> }
>
> Yep, that looks good again.
>
> Perhaps the thing to do would be to add:
>
> lockdep_assert_preemption_disabled()
>
> to rwsem_{set,clear}_owner() and expand the comment there to explain
> that these functions should be in the same preempt-disable section as
> the atomic op that changes sem->count.
Thanks for the suggestion, will add it in v2.
-Mukesh
On Thu, Sep 08, 2022 at 09:18:29PM +0530, Mukesh Ojha wrote: > Hi Peter, > > Thanks for your time in reviewing this patch. > > On 9/8/2022 8:02 PM, Peter Zijlstra wrote: > > On Thu, Sep 01, 2022 at 03:58:10PM +0530, Mukesh Ojha wrote: > > > From: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com> > > > > > > Make the region inside the rwsem_write_trylock non preemptible. > > > > > > We observe RT task is hogging CPU when trying to acquire rwsem lock > > > which was acquired by a kworker task but before the rwsem owner was set. > > > > > > Here is the scenario: > > > 1. CFS task (affined to a particular CPU) takes rwsem lock. > > > > > > 2. CFS task gets preempted by a RT task before setting owner. > > > > > > 3. RT task (FIFO) is trying to acquire the lock, but spinning until > > > RT throttling happens for the lock as the lock was taken by CFS task. > > > > > > This patch attempts to fix the above issue by disabling preemption > > > until owner is set for the lock. while at it also fix this issue > > > at the place where owner being set/cleared. > > > > > > Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com> > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > > > > This is not a valid SoB chain. > > Since this patch of adding preempt disable() at rwsem_write_trylock() is > originated from Gokul. > > Would be adding him in > Original-patch-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com> > > Convert myself to the author/SoB. > Oh, my bad, I missed From is actually Gokul. So yeah, all good, ignore that.
On 9/1/22 06:28, Mukesh Ojha wrote:
> From: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
>
> Make the region inside the rwsem_write_trylock non preemptible.
>
> We observe RT task is hogging CPU when trying to acquire rwsem lock
> which was acquired by a kworker task but before the rwsem owner was set.
>
> Here is the scenario:
> 1. CFS task (affined to a particular CPU) takes rwsem lock.
>
> 2. CFS task gets preempted by a RT task before setting owner.
>
> 3. RT task (FIFO) is trying to acquire the lock, but spinning until
> RT throttling happens for the lock as the lock was taken by CFS task.
Note that the spinning is likely caused by the following code in
rwsem_down_write_slowpath():
1163 /*
1164 * After setting the handoff bit and failing to acquire
1165 * the lock, attempt to spin on owner to accelerate
lock
1166 * transfer. If the previous owner is a on-cpu
writer and it
1167 * has just released the lock, OWNER_NULL will be
returned.
1168 * In this case, we attempt to acquire the lock again
1169 * without sleeping.
1170 */
1171 if (waiter.handoff_set) {
1172 enum owner_state owner_state;
1173
1174 preempt_disable();
1175 owner_state = rwsem_spin_on_owner(sem);
1176 preempt_enable();
1177
1178 if (owner_state == OWNER_NULL)
1179 goto trylock_again;
1180 }
rwsem_optimistic_spin() limits RT task one additional attempt if
OWNER_NULL is returned. There is no such limitation in this loop. So an
alternative will be to put a limit on the number of times an OWNER_NULL
return values will be allowed to continue spinning without sleeping.
That put the burden on the slowpath instead of in the fastpath.
Other than the slight overhead in the fastpath, the patch should work too.
Acked-by: Waiman Long <longman@redhat.com>
Cheers,
Longman
Hi,
On 9/3/2022 2:25 AM, Waiman Long wrote:
>
> On 9/1/22 06:28, Mukesh Ojha wrote:
>> From: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
>>
>> Make the region inside the rwsem_write_trylock non preemptible.
>>
>> We observe RT task is hogging CPU when trying to acquire rwsem lock
>> which was acquired by a kworker task but before the rwsem owner was set.
>>
>> Here is the scenario:
>> 1. CFS task (affined to a particular CPU) takes rwsem lock.
>>
>> 2. CFS task gets preempted by a RT task before setting owner.
>>
>> 3. RT task (FIFO) is trying to acquire the lock, but spinning until
>> RT throttling happens for the lock as the lock was taken by CFS task.
>
> Note that the spinning is likely caused by the following code in
> rwsem_down_write_slowpath():
>
> 1163 /*
> 1164 * After setting the handoff bit and failing to
> acquire
> 1165 * the lock, attempt to spin on owner to accelerate
> lock
> 1166 * transfer. If the previous owner is a on-cpu
> writer and it
> 1167 * has just released the lock, OWNER_NULL will be
> returned.
> 1168 * In this case, we attempt to acquire the lock again
> 1169 * without sleeping.
> 1170 */
> 1171 if (waiter.handoff_set) {
> 1172 enum owner_state owner_state;
> 1173
> 1174 preempt_disable();
> 1175 owner_state = rwsem_spin_on_owner(sem);
> 1176 preempt_enable();
> 1177
> 1178 if (owner_state == OWNER_NULL)
> 1179 goto trylock_again;
> 1180 }
>
> rwsem_optimistic_spin() limits RT task one additional attempt if
> OWNER_NULL is returned. There is no such limitation in this loop. So an
> alternative will be to put a limit on the number of times an OWNER_NULL
> return values will be allowed to continue spinning without sleeping.
> That put the burden on the slowpath instead of in the fastpath.
>
> Other than the slight overhead in the fastpath, the patch should work too.
>
> Acked-by: Waiman Long <longman@redhat.com>
Thanks Waiman for your time and suggestion.
Would like to take others opinion as well.
-Mukesh
>
> Cheers,
> Longman
>
© 2016 - 2026 Red Hat, Inc.