Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <767ce2df-6f9b-40a3-b40b-e9e7a593e3b2@redhat.com>
Date:   Thu, 16 Nov 2023 20:50:45 -0500
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 1/4] locking: Add rwsem_assert_held() and
 rwsem_assert_held_write()
Content-Language: en-US
To:     Matthew Wilcox <willy@infradead.org>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Chandan Babu R <chandan.babu@oracle.com>,
        "Darrick J . Wong" <djwong@kernel.org>, linux-xfs@vger.kernel.org,
        Mateusz Guzik <mjguzik@gmail.com>
References: <20231110204119.3692023-1-willy@infradead.org>
 <20231110204119.3692023-2-willy@infradead.org>
 <52f481a3-bf4f-85ae-9ae6-10a23b48c7c5@redhat.com>
 <ZVPmCoLVXyShSrkN@casper.infradead.org>
 <72dced0f-6d49-4522-beeb-1a398d8f2557@redhat.com>
 <ZVY/cAMFbkuKJF/Y@casper.infradead.org>
From:   Waiman Long <longman@redhat.com>
In-Reply-To: <ZVY/cAMFbkuKJF/Y@casper.infradead.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 11/16/23 11:12, Matthew Wilcox wrote:
> On Tue, Nov 14, 2023 at 08:17:32PM -0500, Waiman Long wrote:
>>>> There are some inconsistency in the use of WARN_ON() and BUG_ON() in the
>>>> assertions. For PREEMPT_RT, held_write is a BUG_ON. For non-PREEMPT_RT, held
>>>> is a BUG_ON. It is not clear why one is BUG_ON and other one is WARN_ON. Is
>>>> there a rationale for that?
>>> I'll fix that up.
>> The check for write lock ownership is accurate. OTOH, the locked check can
>> have false positive and so is less reliable.
> When you say 'false positive', do you mean it might report the lock as
> being held, when it actually isn't, or report the lock as not being held
> when it actually is?  The differing polarities of assert and BUG_ON
> make this confusing as usual.
It means there may be no active lock owner even though the count isn't 
zero. If there is one or more owners, the count will always be non-zero.
>
> Obviously, for an assert, we're OK with it reporting that the lock is
> held when actually it's not.  The caller is expected to hold the lock,
> so failing to trip the assert when the caller doesn't hold the lock
> isn't great, but we can live with it.  OTOH, if the assert fires when
> the caller does hold the lock, that is not tolerable.

The second case shouldn't happen. So the assert should be OK.

Cheers,
Longman