From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 31623C433FE for ; Wed, 12 Oct 2022 13:16:52 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B07B56B0071; Wed, 12 Oct 2022 09:16:51 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A8F856B0073; Wed, 12 Oct 2022 09:16:51 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8E320900002; Wed, 12 Oct 2022 09:16:51 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 76E546B0071 for ; Wed, 12 Oct 2022 09:16:51 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 424E6C0A20 for ; Wed, 12 Oct 2022 13:16:51 +0000 (UTC) X-FDA: 80012347422.08.78AA9EB Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf22.hostedemail.com (Postfix) with ESMTP id BA7A8C0037 for ; Wed, 12 Oct 2022 13:16:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1665580610; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=G1SlhP8YtCDLXQxqiYxHrawoN/1E0Lo/8a/OQeBjWs8=; b=X1hq7BAg+ZJiFBTIZGkTNbBnbwEFYHVdMH3gfwayUS7BhRRqO/s1lXfrQLmK5FPDWISt1M h74k60VvDDtiTxyJS2fIhxuNnTBeD3bcCExApDTcibXsoaRA2XmSVhE3Xc4Azmj2Lt2xir tuqBD+ZhY8XYTPnV3TNZhnIdibeuqw4= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-427-CeUP5aDlOrCUhp0CWW6c4w-1; Wed, 12 Oct 2022 09:16:47 -0400 X-MC-Unique: CeUP5aDlOrCUhp0CWW6c4w-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 8EDBC811E84; Wed, 12 Oct 2022 13:16:46 +0000 (UTC) Received: from [10.22.33.120] (unknown [10.22.33.120]) by smtp.corp.redhat.com (Postfix) with ESMTP id E311F4EA5F; Wed, 12 Oct 2022 13:16:45 +0000 (UTC) Message-ID: <41871fa1-897d-c43e-671b-1ad927c0b9da@redhat.com> Date: Wed, 12 Oct 2022 09:16:45 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.12.0 Subject: Re: [PATCH] locking/rwsem: Prevent non-first waiter from spinning in down_write() slowpath Content-Language: en-US To: Mukesh Ojha , Hillf Danton Cc: linux-kernel@vger.kernel.org, john.p.donnelly@oracle.com, linux-mm@kvack.org, Peter Zijlstra , Will Deacon , Boqun Feng , Ingo Molnar References: <20221011104621.231-1-hdanton@sina.com> <3967aca6-3403-655d-d8eb-34312c2bb1b9@quicinc.com> From: Waiman Long In-Reply-To: <3967aca6-3403-655d-d8eb-34312c2bb1b9@quicinc.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1665580610; a=rsa-sha256; cv=none; b=ODxiRmrNZEVTLSA7ahlm6ZqaYSpJa2gXth7SnjrFjMTCPr7iMhJWNYsfW3KA9GWtyB616Q TnBsixJ8clpewVzM480FVusDG9aOpIG228+NrAigzxLZ49GHnPc/TjM5+isMxxiuE88sQp JpJ5afEhd9vC6ww5h5bxs9HaIo30tew= ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=X1hq7BAg; spf=pass (imf22.hostedemail.com: domain of longman@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=longman@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1665580610; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=G1SlhP8YtCDLXQxqiYxHrawoN/1E0Lo/8a/OQeBjWs8=; b=b7WF1LdVxjwBalUIxQofm0hxmYxVKkEvLK87Q2IvukU3gtgCBQ2s7Fv2Kjku8FLhlGIp1K cJUnoJfNPELP/Cw+iUfKdv4ul9nmv4XoF3eymt65Ce1qil6kmNzMaKAqrLUVWhZ9MYjcG0 H1tq2KAIda487YiHfB8gsM3y+OOmEes= X-Stat-Signature: o1nig5y7fbpcxndmh4ernxnm7hoonc89 X-Rspamd-Queue-Id: BA7A8C0037 X-Rspam-User: X-Rspamd-Server: rspam08 Authentication-Results: imf22.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=X1hq7BAg; spf=pass (imf22.hostedemail.com: domain of longman@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=longman@redhat.com; dmarc=pass (policy=none) header.from=redhat.com X-HE-Tag: 1665580610-163462 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 10/11/22 09:16, Mukesh Ojha wrote: > Hi @Hilf, > > Thanks for looking into this issue. > > On 10/11/2022 4:16 PM, Hillf Danton wrote: >> On 10/10/22 06:24 Mukesh Ojha >>> Hi Waiman, >>> >>> On 9/29/2022 11:36 PM, Waiman Long wrote: >>>> On 9/29/22 14:04, Waiman Long wrote: >>>>> A non-first waiter can potentially spin in the for loop of >>>>> rwsem_down_write_slowpath() without sleeping but fail to acquire the >>>>> lock even if the rwsem is free if the following sequence happens: >>>>> >>>>>     Non-first waiter       First waiter      Lock holder >>>>>     ----------------       ------------      ----------- >>>>>     Acquire wait_lock >>>>>     rwsem_try_write_lock(): >>>>>       Set handoff bit if RT or >>>>>         wait too long >>>>>       Set waiter->handoff_set >>>>>     Release wait_lock >>>>>                            Acquire wait_lock >>>>>                            Inherit waiter->handoff_set >>>>>                            Release wait_lock >>>>>                         Clear owner >>>>>                                              Release lock >>>>>     if (waiter.handoff_set) { >>>>>       rwsem_spin_on_owner((); >>>>>       if (OWNER_NULL) >>>>>         goto trylock_again; >>>>>     } >>>>>     trylock_again: >>>>>     Acquire wait_lock >>>>>     rwsem_try_write_lock(): >>>>>        if (first->handoff_set && (waiter != first)) >>>>>            return false; >>>>>     Release wait_lock >>>>> >>>>> It is especially problematic if the non-first waiter is an RT task >>>>> and >>>>> it is running on the same CPU as the first waiter as this can lead to >>>>> live lock. >>>>> >>>>> Fixes: d257cc8cb8d5 ("locking/rwsem: Make handoff bit handling more >>>>> consistent") >>>>> Signed-off-by: Waiman Long >>>>> --- >>>>>    kernel/locking/rwsem.c | 13 ++++++++++--- >>>>>    1 file changed, 10 insertions(+), 3 deletions(-) >>>> >>>> Mukesh, can you test if this patch can fix the RT task lockup problem? >>>> >>> >>> Looks like, There is still a window for a race. >>> >>> There is a chance when a reader who came first added it's BIAS and >>> goes to slowpath and before it gets added to wait list it got >>> preempted by RT task which  goes to slowpath as well and being the >>> first waiter gets its hand-off bit set and not able to get the lock >>> due to following condition in rwsem_try_write_lock() > > [] > >>> >>>   630                 if (count & RWSEM_LOCK_MASK) { ==> reader has >>> sets its bias >>> .. >>> ... >>> >>>   634 >>>   635                         new |= RWSEM_FLAG_HANDOFF; >>>   636                 } else { >>>   637                         new |= RWSEM_WRITER_LOCKED; >>> >>> >>> ---------------------->----------------------->------------------------- >>> >>> >>> First reader (1)          writer(2) RT task             Lock holder(3) >>> >>> It sets >>> RWSEM_READER_BIAS. >>> while it is going to >>> slowpath(as the lock >>> was held by (3)) and >>> before it got added >>> to the waiters list >>> it got preempted >>> by (2). >>>                         RT task also takes >>>                          the slowpath and add release the >>>                          itself into waiting list rwsem lock >>>              and since it is the first         clear the >>>                          it is the next one to get owner. >>>                          the lock but it can not >>>                          get the lock as (count & >>>                          RWSEM_LOCK_MASK) is set >>>                          as (1) has added it but >>>                          not able to remove its >>>              adjustment. > > [] > >>> >> Hey Mukesh, >> >> Can you test the diff if it makes sense to you? >> >> It simply prevents the first waiter from spinning any longer after >> detecting >> it barely makes any progress to spin without lock owner. >> >> Hillf >> >> --- mainline/kernel/locking/rwsem.c >> +++ b/kernel/locking/rwsem.c >> @@ -611,26 +611,15 @@ static inline bool rwsem_try_write_lock( >>       long count, new; >>         lockdep_assert_held(&sem->wait_lock); >> +    waiter->handoff_set = false; >>         count = atomic_long_read(&sem->count); >>       do { >>           bool has_handoff = !!(count & RWSEM_FLAG_HANDOFF); >>             if (has_handoff) { >> -            /* >> -             * Honor handoff bit and yield only when the first >> -             * waiter is the one that set it. Otherwisee, we >> -             * still try to acquire the rwsem. >> -             */ >> -            if (first->handoff_set && (waiter != first)) >> +            if (waiter != first) >>                   return false; > > you mean, you want to check and change waiter->handoff_set on every > run rwsem_try_write_lock(). > > But does it break optimistic spinning ? @waiman ? > As I said in my previous mail, this is equivalent to allow only one optimistic spinning attempt after setting the handoff bit. That will likely reduce the performance benefit provided by this feature. Cheers, Longman