lkml.org 
[lkml]   [2022]   [Sep]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] locking/rwsem: Disable preemption while trying for rwsem lock
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.

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

\
 
 \ /
  Last update: 2022-09-08 16:33    [W:0.110 / U:0.248 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site