On Wed, Mar 13, 2024 at 2:07 AM Wang, Li <li.w...@windriver.com> wrote:
>
>
> On 3/13/2024 10:55, Bruce Ashfield wrote:
> > In message: [linux-yocto][v5.15/standard/base][PATCH] locking/rwsem:
> > Disable preemption while trying for rwsem lock
> > on 10/03/2024 Li Wang via lists.yoctoproject.org wrote:
> >
> >> From: Gokul krishna Krishnakumar <quic_gokuk...@quicinc.com>
> >>
> >> commit 48dfb5d2560d36fb16c7d430c229d1604ea7d185 upstream
> >>
> >> 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 the issues
> >> at the places where rwsem_{set,clear}_owner() are called.
> >>
> >> This also adds lockdep annotation of preemption disable in
> >> rwsem_{set,clear}_owner() on Peter Z. suggestion.
> > Any thoughts on why this hasn't been picked up by -stable ?
>
> submitter want the patch into linux stable 5.10, but miss the receiver
> mail of stable.
>
> we will wait the reply from the maintainer of stable:
>
> https://lore.kernel.org/all/b92644e5-529b-4403-aba7-d316262cc...@redhat.com/
>
Perfect!
I'll do the merge now as well, since any conflicts will be easy to
handle. I just
wanted to be sure that we let -stable know (we've done our best, whether it
makes -stable or not :)
Bruce
> Thanks,
> LiWang.
>
> >
> > Bruce
> >
> >> Signed-off-by: Gokul krishna Krishnakumar <quic_gokuk...@quicinc.com>
> >> Signed-off-by: Mukesh Ojha <quic_mo...@quicinc.com>
> >> Signed-off-by: Peter Zijlstra (Intel) <pet...@infradead.org>
> >> Reviewed-by: Waiman Long <long...@redhat.com>
> >> Link:
> >> https://lore.kernel.org/r/1662661467-24203-1-git-send-email-quic_mo...@quicinc.com
> >> Signed-off-by: Beniamin Sandu <beniamin.sa...@windriver.com>
> >> Signed-off-by: Li Wang <li.w...@windriver.com>
> >> ---
> >> kernel/locking/rwsem.c | 14 ++++++++++++--
> >> 1 file changed, 12 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
> >> index f0287a16b4ec..4a38d32b89fa 100644
> >> --- a/kernel/locking/rwsem.c
> >> +++ b/kernel/locking/rwsem.c
> >> @@ -133,14 +133,19 @@
> >> * the owner value concurrently without lock. Read from owner, however,
> >> * may not need READ_ONCE() as long as the pointer value is only used
> >> * for comparison and isn't being dereferenced.
> >> + *
> >> + * Both rwsem_{set,clear}_owner() functions should be in the same
> >> + * preempt disable section as the atomic op that changes sem->count.
> >> */
> >> static inline void rwsem_set_owner(struct rw_semaphore *sem)
> >> {
> >> + lockdep_assert_preemption_disabled();
> >> atomic_long_set(&sem->owner, (long)current);
> >> }
> >>
> >> static inline void rwsem_clear_owner(struct rw_semaphore *sem)
> >> {
> >> + lockdep_assert_preemption_disabled();
> >> atomic_long_set(&sem->owner, 0);
> >> }
> >>
> >> @@ -251,13 +256,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;
> >> }
> >>
> >> /*
> >> @@ -1341,8 +1349,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.25.1
> >>
> >>
> >>
--
- Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end
- "Use the force Harry" - Gandalf, Star Trek II
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#13683):
https://lists.yoctoproject.org/g/linux-yocto/message/13683
Mute This Topic: https://lists.yoctoproject.org/mt/104857955/21656
Group Owner: linux-yocto+ow...@lists.yoctoproject.org
Unsubscribe: https://lists.yoctoproject.org/g/linux-yocto/unsub
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
