linux-kernel - Re: locking/rwsem: RT throttling issue due to RT task hogging the cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4a4952a8-3d1b-8197-4cb6-b1de7eecf21f@quicinc.com>
Date:   Mon, 26 Sep 2022 17:16:34 +0530
From:   Mukesh Ojha <quic_mojha@...cinc.com>
To:     Peter Zijlstra <peterz@...radead.org>, <mingo@...hat.com>,
        <will@...nel.org>
CC:     <longman@...hat.com>, <linux-kernel@...r.kernel.org>,
        <"<boqun.feng"@gmail.com>
Subject: Re: locking/rwsem: RT throttling issue due to RT task hogging the cpu

Hi,

Any comments on this issue would be helpful.

Thanks,
Mukesh

On 9/20/2022 9:49 PM, Mukesh Ojha wrote:
> Hi,
> 
> We are observing one issue where, sem->owner is not set and sem->count=6 
> [1] which means both RWSEM_FLAG_WAITERS and RWSEM_FLAG_HANDOFF bits are 
> set. And if unfold the sem->wait_list we see the following order of 
> process waiting  [2] where [a] is waiting for write, while [b],[c] are 
> waiting for read and [d] is the RT task for which 
> waiter.handoff_set=true and it is continuously running on cpu7 and not 
> letting the first write waiter [a] on cpu7.
> 
> [1]
> 
>    sem = 0xFFFFFFD57DDC6680 -> (
>      count = (counter = 6),
>      owner = (counter = 0),
> 
> [2]
> 
> [a] kworker/7:0 pid: 32516 ==> [b] iptables-restor pid: 18625 ==> 
> [c]HwBinder:1544_3  pid: 2024 ==> [d] RenderEngine pid: 2032 cpu: 7 
> prio:97 (RT task)
> 
> 
> Sometime back, Waiman has suggested this which could help in RT task
> leaving the cpu.
> 
> https://lore.kernel.org/all/8c33f989-8870-08c6-db12-521de634b34e@redhat.com/ 
> 
> 
> --------------------------------->O----------------------------
> 
>  From c6493edd7a5e4f597ea55ff0eb3f1d763b335dfc Mon Sep 17 00:00:00 2001
>    2 From: Waiman Long <longman@...hat.com>
>    3 Date: Tue, 20 Sep 2022 20:50:45 +0530
>    4 Subject: [PATCH] locking/rwsem: Yield the cpu after doing handoff 
> optimistic
>    5  spinning
>    6
>    7 It is possible the new lock owner (writer) can be preempted before 
> setting
>    8 the owner field and if the current(e.g RT task) waiter is the task 
> that
>    9 preempts the new lock owner, it will hand_off spin loop for a long 
> time.
>   10 Avoid wasting cpu time and delaying the release of the lock by 
> yielding
>   11 the cpu if handoff optimistic spinning has been done multiple times 
> with
>   12 NULL owner.
>   13
>   14 Signed-off-by: Waiman Long <longman@...hat.com>
>   15 Signed-off-by: Mukesh Ojha <quic_mojha@...cinc.com>
>   16 ---
>   17  kernel/locking/rwsem.c | 15 ++++++++++++++-
>   18  1 file changed, 14 insertions(+), 1 deletion(-)
>   19
>   20 diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
>   21 index 65f0262..a875758 100644
>   22 --- a/kernel/locking/rwsem.c
>   23 +++ b/kernel/locking/rwsem.c
>   24 @@ -361,6 +361,8 @@ enum rwsem_wake_type {
>   25   */
>   26  #define MAX_READERS_WAKEUP     0x100
>   27
>   28 +#define MAX_HANDOFF_SPIN       10
>   29 +
>   30  static inline void
>   31  rwsem_add_waiter(struct rw_semaphore *sem, struct rwsem_waiter 
> *waiter)
>   32  {
>   33 @@ -1109,6 +1111,7 @@ rwsem_down_write_slowpath(struct rw_semaphore 
> *sem, int state)
>   34  {
>   35         struct rwsem_waiter waiter;
>   36         DEFINE_WAKE_Q(wake_q);
>   37 +       int handoff_spins = 0;
>   38
>   39         /* do optimistic spinning and steal lock if possible */
>   40         if (rwsem_can_spin_on_owner(sem) && 
> rwsem_optimistic_spin(sem)) {
>   41 @@ -1167,6 +1170,14 @@ rwsem_down_write_slowpath(struct 
> rw_semaphore *sem, int state)
>   42                  * has just released the lock, OWNER_NULL will be 
> returned.
>   43                  * In this case, we attempt to acquire the lock again
>   44                  * without sleeping.
>   45 +                *
>   46 +                * It is possible the new lock owner (writer) can 
> be preempted
>   47 +                * before setting the owner field and if the 
> current(e.g RT task)
>   48 +                * waiter is the task that preempts the new lock 
> owner, it will
>   49 +                * spin in this loop for a long time. Avoid wasting 
> cpu time
>   50 +                * and delaying the release of the lock by yielding 
> the cpu if
>   51 +                * handoff optimistic spinning has been done 
> multiple times with
>   52 +                * NULL owner.
>   53                  */
>   54                 if (waiter.handoff_set) {
>   55                         enum owner_state owner_state;
>   56 @@ -1175,8 +1186,10 @@ rwsem_down_write_slowpath(struct 
> rw_semaphore *sem, int state)
>   57                         owner_state = rwsem_spin_on_owner(sem);
>   58                         preempt_enable();
>   59
>   60 -                       if (owner_state == OWNER_NULL)
>   61 +                       if ((owner_state == OWNER_NULL) && 
> (handoff_spins < MAX_HANDOFF_SPIN)) {
>   62 +                               handoff_spins++;
>   63                                 goto trylock_again;
>   64 +                       }
>   65                 }
>   66
>   67                 schedule();
>   68 --
>   69 2.7.4
>   70
> 
> 
> -Mukesh