Re: [PATCH v2] kernel/locking: Add context to ww_mutex_trylock.

[Maarten Lankhorst]

Op 14-09-2021 om 15:54 schreef Daniel Vetter:
> On Tue, Sep 14, 2021 at 02:43:02PM +0200, Maarten Lankhorst wrote:
>> Op 14-09-2021 om 08:50 schreef Peter Zijlstra:
>>> On Mon, Sep 13, 2021 at 10:42:36AM +0200, Maarten Lankhorst wrote:
>>>
>>>>> +/**
>>>>> + * ww_mutex_trylock - tries to acquire the w/w mutex with optional acquire context
>>>>> + * @ww: mutex to lock
>>>>> + * @ww_ctx: optional w/w acquire context
>>>>> + *
>>>>> + * Trylocks a mutex with the optional acquire context; no deadlock detection is
>>>>> + * possible. Returns 1 if the mutex has been acquired successfully, 0 otherwise.
>>>>> + *
>>>>> + * Unlike ww_mutex_lock, no deadlock handling is performed. However, if a @ctx is
>>>>> + * specified, -EALREADY handling may happen in calls to ww_mutex_trylock.
>>>>> + *
>>>>> + * A mutex acquired with this function must be released with ww_mutex_unlock.
>>>>> + */
>>>>> +int ww_mutex_trylock(struct ww_mutex *ww, struct ww_acquire_ctx *ww_ctx)
>>>>> +{
>>>>> +	if (!ww_ctx)
>>>>> +		return mutex_trylock(&ww->base);
>>>>> +
>>>>> +	MUTEX_WARN_ON(ww->base.magic != &ww->base);
>>>>> +
>>>>> +	if (unlikely(ww_ctx == READ_ONCE(ww->ctx)))
>>>>> +		return -EALREADY;
>>>> I'm not 100% sure this is a good idea, because it would make the
>>>> trylock weird.  For i915 I checked manually, because I didn't want to
>>>> change the function signature. This is probably the other extreme.
>>>>
>>>> "if (ww_mutex_trylock())" would look correct, but actually be wrong
>>>> and lead to double unlock without adjustments.  Maybe we could make a
>>>> ww_mutex_trylock_ctx_err, which would return -EALREADY or -EBUSY on
>>>> failure, and 0 on success?  We could keep ww_mutex_trylock without
>>>> ctx, probably just #define as (!ww_mutex_trylock_ctx_err(lock, NULL))
>>> Urgh, yeah. Also, I suppose that if we already own it, we'll just fail
>>> the trylock anyway. Let me take this out.
>>>
>>>>> +	/*
>>>>> +	 * Reset the wounded flag after a kill. No other process can
>>>>> +	 * race and wound us here, since they can't have a valid owner
>>>>> +	 * pointer if we don't have any locks held.
>>>>> +	 */
>>>>> +	if (ww_ctx->acquired == 0)
>>>>> +		ww_ctx->wounded = 0;
>>>> Yeah I guess this needs fixing too. Not completely sure since trylock
>>>> wouldn't do the whole ww dance, but since it's our first lock,
>>>> probably best to do so regardless so other users don't trip over it.
>>> This is actually critical, because if this trylock is the first lock
>>> acquisition for the context, there won't be any other opportunity to
>>> reset this value.
>>>
>>>>> +
>>>>> +	if (__mutex_trylock(&ww->base)) {
>>>>> +		ww_mutex_set_context_fastpath(ww, ww_ctx);
>>>>> +		mutex_acquire_nest(&ww->base.dep_map, 0, 1, &ww_ctx->dep_map, _RET_IP_);
>>>>> +		return 1;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL(ww_mutex_trylock);
>>> Updated version below...
>>>
>>> ---
>>> Subject: kernel/locking: Add context to ww_mutex_trylock()
>>> From: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>>> Date: Thu, 9 Sep 2021 11:32:18 +0200
>>>
>>> From: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>>>
>>> i915 will soon gain an eviction path that trylock a whole lot of locks
>>> for eviction, getting dmesg failures like below:
>>>
>>>   BUG: MAX_LOCK_DEPTH too low!
>>>   turning off the locking correctness validator.
>>>   depth: 48  max: 48!
>>>   48 locks held by i915_selftest/5776:
>>>    #0: ffff888101a79240 (&dev->mutex){....}-{3:3}, at: __driver_attach+0x88/0x160
>>>    #1: ffffc900009778c0 (reservation_ww_class_acquire){+.+.}-{0:0}, at: i915_vma_pin.constprop.63+0x39/0x1b0 [i915]
>>>    #2: ffff88800cf74de8 (reservation_ww_class_mutex){+.+.}-{3:3}, at: i915_vma_pin.constprop.63+0x5f/0x1b0 [i915]
>>>    #3: ffff88810c7f9e38 (&vm->mutex/1){+.+.}-{3:3}, at: i915_vma_pin_ww+0x1c4/0x9d0 [i915]
>>>    #4: ffff88810bad5768 (reservation_ww_class_mutex){+.+.}-{3:3}, at: i915_gem_evict_something+0x110/0x860 [i915]
>>>    #5: ffff88810bad60e8 (reservation_ww_class_mutex){+.+.}-{3:3}, at: i915_gem_evict_something+0x110/0x860 [i915]
>>>   ...
>>>    #46: ffff88811964d768 (reservation_ww_class_mutex){+.+.}-{3:3}, at: i915_gem_evict_something+0x110/0x860 [i915]
>>>    #47: ffff88811964e0e8 (reservation_ww_class_mutex){+.+.}-{3:3}, at: i915_gem_evict_something+0x110/0x860 [i915]
>>>   INFO: lockdep is turned off.
>>>
>>> Fixing eviction to nest into ww_class_acquire is a high priority, but
>>> it requires a rework of the entire driver, which can only be done one
>>> step at a time.
>>>
>>> As an intermediate solution, add an acquire context to
>>> ww_mutex_trylock, which allows us to do proper nesting annotations on
>>> the trylocks, making the above lockdep splat disappear.
>>>
>>> This is also useful in regulator_lock_nested, which may avoid dropping
>>> regulator_nesting_mutex in the uncontended path, so use it there.
>>>
>>> TTM may be another user for this, where we could lock a buffer in a
>>> fastpath with list locks held, without dropping all locks we hold.
>>>
>>> [peterz: rework actual ww_mutex_trylock() implementations]
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
>>> ---
>> My original patch series with this patch in place still passes i915 selftests, looks good to me. :)
> For merge logistics, can we pls have a stable branch? I expect that the
> i915 patches will be ready for 5.16.
>
> Or send it in for -rc2 so that the interface change doesn't cause needless
> conflicts, whatever you think is best.
> -Daniel
Yeah, some central branch drm could pull from, would make upstreaming patches that depends on it easier. :)