linux-kernel - Re: [PATCH v3 4/5] locking/rwsem: Enable direct rwsem lock handoff

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] [day] [month] [year] [list]

Message-ID: <ef17f410-1cc2-7159-5e4c-f146a9e4aad1@redhat.com>
Date:   Tue, 18 Oct 2022 13:37:20 -0400
From:   Waiman Long <longman@...hat.com>
To:     Mukesh Ojha <quic_mojha@...cinc.com>,
        Hillf Danton <hdanton@...a.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Boqun Feng <boqun.feng@...il.com>,
        linux-kernel@...r.kernel.org, john.p.donnelly@...cle.com,
        Ting11 Wang 王婷 <wangting11@...omi.com>
Subject: Re: [PATCH v3 4/5] locking/rwsem: Enable direct rwsem lock handoff

On 10/18/22 10:13, Mukesh Ojha wrote:
> Hi,
>
> On 10/18/2022 4:44 PM, Hillf Danton wrote:
>> On 17 Oct 2022 17:13:55 -0400 Waiman Long <longman@...hat.com>
>>> @@ -1067,13 +1119,33 @@ rwsem_down_read_slowpath(struct rw_semaphore 
>>> *sem, long count, unsigned int stat
>>>               return sem;
>>>           }
>>>           adjustment += RWSEM_FLAG_WAITERS;
>>> +    } else if ((count & RWSEM_FLAG_HANDOFF) &&
>>> +          ((count & RWSEM_LOCK_MASK) == RWSEM_READER_BIAS)) {
>>
>> Could a couple of CPUs go read slow path in parallel?
This is under wait_lock protection. So no parallel execution is possible.
>>
>>> +        /*
>>> +         * If the waiter to be handed off is a reader, this reader
>>> +         * can piggyback on top of top of that.
>>> +         */
>>> +        if (rwsem_first_waiter(sem)->type == RWSEM_WAITING_FOR_READ)
>>> +            adjustment = 0;
>>> +        rwsem_handoff(sem, adjustment, &wake_q);
>>> +
>>> +        if (!adjustment) {
>>> +            raw_spin_unlock_irq(&sem->wait_lock);
>>> +            wake_up_q(&wake_q);
>>> +            return sem;
>>> +        }
>>> +        adjustment = 0;
>>>       }
>>>       rwsem_add_waiter(sem, &waiter);
>>
>> Why can this acquirer pigyback without becoming a waiter?
The idea is to have as much reader parallelism as possible without 
writer starvation. In other word, a continuous stream of readers is not 
allowed to block out writer. However, there are places where allow one 
more reader to get the lock won't cause writer starvation.
>>
>>>   -    /* we're now waiting on the lock, but no longer actively 
>>> locking */
>>> -    count = atomic_long_add_return(adjustment, &sem->count);
>>> -
>>> -    rwsem_cond_wake_waiter(sem, count, &wake_q);
>>> +    if (adjustment) {
>>> +        /*
>>> +         * We are now waiting on the lock with no handoff, but no
>>> +         * longer actively locking.
>>> +         */
>>> +        count = atomic_long_add_return(adjustment, &sem->count);
>>> +        rwsem_cond_wake_waiter(sem, count, &wake_q);
>>> +    }
>>>       raw_spin_unlock_irq(&sem->wait_lock);
>>>         if (!wake_q_empty(&wake_q))
>>> @@ -1120,7 +1192,6 @@ static struct rw_semaphore __sched *
>>>   rwsem_down_write_slowpath(struct rw_semaphore *sem, int state)
>>>   {
>>>       struct rwsem_waiter waiter;
>>> -    int null_owner_retries;
>>
>> This reverts 2/5 and feel free to drop it directly.
>
> I think, he intents to tag the first two patches to go to stable 
> branches.

This patch is too disruptive to go to the stable branches. Yes, I do 
intend the first 2 patches to go into stable branches.

Cheers,
Longman