Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 4/4] locking/locktorture: Fix incorrect use of
 ww_acquire_ctx in ww_mutex test
To:     Davidlohr Bueso <dave@stgolabs.net>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
        Boqun Feng <boqun.feng@gmail.com>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        linux-kernel@vger.kernel.org, Juri Lelli <juri.lelli@redhat.com>
References: <20210316153119.13802-1-longman@redhat.com>
 <20210316153119.13802-5-longman@redhat.com>
 <20210317051605.popetodgwbr47ha2@offworld>
From:   Waiman Long <longman@redhat.com>
Organization: Red Hat
Message-ID: <ae20c4a1-1591-4b09-6de2-e55c30297d24@redhat.com>
Date:   Wed, 17 Mar 2021 09:21:50 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.0
MIME-Version: 1.0
In-Reply-To: <20210317051605.popetodgwbr47ha2@offworld>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

On 3/17/21 1:16 AM, Davidlohr Bueso wrote:
> On Tue, 16 Mar 2021, Waiman Long wrote:
>
>> The ww_acquire_ctx structure for ww_mutex needs to persist for a 
>> complete
>> lock/unlock cycle. In the ww_mutex test in locktorture, however, both
>> ww_acquire_init() and ww_acquire_fini() are called within the lock
>> function only. This causes a lockdep splat of "WARNING: Nested lock
>> was not taken" when lockdep is enabled in the kernel.
>>
>> To fix this problem, we need to move the ww_acquire_fini() after the
>> ww_mutex_unlock() in torture_ww_mutex_unlock(). In other word, we need
>> to pass state information from the lock function to the unlock function.
>
> Right, and afaict this _is_ the way ww_acquire_fini() should be called:
>
>  * Releases a w/w acquire context. This must be called _after_ all 
> acquired w/w
>  * mutexes have been released with ww_mutex_unlock.
>
>> Change the writelock and writeunlock function prototypes to allow that
>> and change the torture_ww_mutex_lock() and torture_ww_mutex_unlock()
>> accordingly.
>
> But wouldn't just making ctx a global variable be enough instead? That 
> way
> we don't deal with memory allocation for every lock/unlock operation 
> (yuck).
> Plus the ENOMEM would need to be handled/propagated accordingly - the 
> code
> really doesn't expect any failure from ->writelock().

The ctx should be per-thread to track potential locking conflict. Since 
there are as many locking threads as the number of cpus, we can't use 
one global variable to do that. I was thinking about using per-cpu 
variable but locktorture kthreads are cpu-bound. That led me to use the 
current scheme of allocation at lock and free at unlock.

Another alternative is to add a per-thread init/fini methods to allow 
setting up per-thread context that is passed to the locking functions. 
By doing that, we only need one kmalloc/kfree pair per running 
locktorture kthread.

Cheers,
Longman