Received: by 2002:a25:ef43:0:0:0:0:0 with SMTP id w3csp266531ybm; Thu, 28 May 2020 22:53:20 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxgxY2mbeYeMFPfcV9inQRiZU1GEPi3pOshHq0qX+7SLqx3pAm2mxOJIEODH8di0xo6aKML X-Received: by 2002:a17:906:f115:: with SMTP id gv21mr519012ejb.340.1590731600001; Thu, 28 May 2020 22:53:20 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1590731599; cv=none; d=google.com; s=arc-20160816; b=NqYJmrImY6hp/jur/NUZ5EXDnvhpTqPemkdYcF7FFQyeYENOuq4wNKsDoGJO8FjqdE yQ3WAMAGaljvCI3O7n+zK7i6d40EE+hXNHtoRHc1wDpcqI2SYIedCN1TbCOvIH/m+P6C o1VObgidOfSmpJIv4v2QNZuJzEToWx513ICflsky7l6wEGKtn3bSMYIZxSFUsizTwgH2 1mkV6MJBJHp2Med6gKiOcQrR4KD3R7xY53p+RhoX6mY5LPYgrWoIkhx9hz8+E8rOq5wC MJ0Kz0anb0rLBnHEhkGDE3h14y4x5bX03fhmVSo/XWblEX7FiMfbzuNk650fz1/Mveyz rP7g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:cc:to:subject :message-id:date:from:in-reply-to:references:mime-version :dkim-signature; bh=GwPTcodFrxyo6ma4eVz3wTAE48j9kX0xKI9NPcHg3+Y=; b=NT5VyeOCgpi2drBt3slyySrcO6MgYxL2CPw36DTzFX1/S3Rk304gNxHpwSbzgSPZef ElXEeqbsrpbkQtiz1JN9X5CtccvKa4Onc+d0oEkf+7hghcXsxNkA0iHwL+qdg0f6WiQw RHVh1zVMtAT3N0/ltadetMpYs9s2Zgrss6FdBz6y60ngNvVmQNkW/f408CKl/IaWY56O p0PeIEz3p8omkzou4tGpa9ee6/9AIauot5Z6PWKbc7yvTz1jxUTtGEhv7tUIPpdlEyAw YzKvZNmX3U9YypdwmF26yC1sqldv9ATferCcewD3hVjPwMbGu0/TIMrCE0HVVgjbaUFz nqHg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ffwll.ch header.s=google header.b=g7c8phrQ; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id k13si5068592edv.394.2020.05.28.22.52.57; Thu, 28 May 2020 22:53:19 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@ffwll.ch header.s=google header.b=g7c8phrQ; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725854AbgE2FtP (ORCPT + 99 others); Fri, 29 May 2020 01:49:15 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44260 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725562AbgE2FtO (ORCPT ); Fri, 29 May 2020 01:49:14 -0400 Received: from mail-oi1-x242.google.com (mail-oi1-x242.google.com [IPv6:2607:f8b0:4864:20::242]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6A18BC08C5C9 for ; Thu, 28 May 2020 22:49:14 -0700 (PDT) Received: by mail-oi1-x242.google.com with SMTP id w4so1619425oia.1 for ; Thu, 28 May 2020 22:49:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=GwPTcodFrxyo6ma4eVz3wTAE48j9kX0xKI9NPcHg3+Y=; b=g7c8phrQOHUbjmhEiH1WaiZ1ynwv7l8bGG0INitQ4WPavylOnmjSOorKtp00skaFkC OznTnDNw77kWfXlyzgGTJy7zf94suVZrOnm3RVNMArQ0jgcN/N11ZPOz4POFpT2QoIIf GCXS4yH+9IzvvLeppOxZjpO4kWcwbINiDyDo8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=GwPTcodFrxyo6ma4eVz3wTAE48j9kX0xKI9NPcHg3+Y=; b=NdLOaQL+pxo0cbBhkBVMTTGCsh8nyH+9poTSKCwYc6IiBxoNvqcG7+hVGJ1ptdhUiK TSSqfaMoA7nMnAtvWXacXPzpVIFYKEAZl7yjCDR5KpPcdu7wSIq9FX7VGwKKvQ082jhX mjJ7I2MPwoUXdZ0BVjQRMJlSrPqKAHthldOLbpWq54AtC++3siNNudVFqrfkJdLTRM+h IpEJINU3bz8v1Zy1j62XHcIUPsFVq3hY8oMnvKkQO06e5iFemuedyy4j03HnueTOuu32 D1f2R6Nv4ISH7q7bIYzVMTIDebTg4RDROrQnilbmk5CNoIySbe9Ju1/bXxArqRYNKrax fOxw== X-Gm-Message-State: AOAM532budMM7F3iJ6EXeSkGC+BSqX+gf7P7h+yStC5/jt71iLUXAKEY ilqoVDcUMweb7DivIkHA+q6KFkfgpczAAqHh7rEErQ== X-Received: by 2002:a05:6808:282:: with SMTP id z2mr4257884oic.101.1590731353651; Thu, 28 May 2020 22:49:13 -0700 (PDT) MIME-Version: 1.0 References: <20200512085944.222637-1-daniel.vetter@ffwll.ch> <20200512085944.222637-3-daniel.vetter@ffwll.ch> <190e5572-fc29-612d-87e0-a4f0151abcc6@amd.com> In-Reply-To: <190e5572-fc29-612d-87e0-a4f0151abcc6@amd.com> From: Daniel Vetter Date: Fri, 29 May 2020 07:49:02 +0200 Message-ID: Subject: Re: [RFC 02/17] dma-fence: basic lockdep annotations To: Luben Tuikov Cc: DRI Development , linux-rdma , intel-gfx , Maarten Lankhorst , LKML , amd-gfx list , Chris Wilson , "moderated list:DMA BUFFER SHARING FRAMEWORK" , Daniel Vetter , =?UTF-8?Q?Christian_K=C3=B6nig?= , "open list:DMA BUFFER SHARING FRAMEWORK" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 28, 2020 at 11:54 PM Luben Tuikov wrote: > > On 2020-05-12 4:59 a.m., Daniel Vetter wrote: > > Design is similar to the lockdep annotations for workers, but with > > some twists: > > > > - We use a read-lock for the execution/worker/completion side, so that > > this explicit annotation can be more liberally sprinkled around. > > With read locks lockdep isn't going to complain if the read-side > > isn't nested the same way under all circumstances, so ABBA deadlocks > > are ok. Which they are, since this is an annotation only. > > > > - We're using non-recursive lockdep read lock mode, since in recursive > > read lock mode lockdep does not catch read side hazards. And we > > _very_ much want read side hazards to be caught. For full details of > > this limitation see > > > > commit e91498589746065e3ae95d9a00b068e525eec34f > > Author: Peter Zijlstra > > Date: Wed Aug 23 13:13:11 2017 +0200 > > > > locking/lockdep/selftests: Add mixed read-write ABBA tests > > > > - To allow nesting of the read-side explicit annotations we explicitly > > keep track of the nesting. lock_is_held() allows us to do that. > > > > - The wait-side annotation is a write lock, and entirely done within > > dma_fence_wait() for everyone by default. > > > > - To be able to freely annotate helper functions I want to make it ok > > to call dma_fence_begin/end_signalling from soft/hardirq context. > > First attempt was using the hardirq locking context for the write > > side in lockdep, but this forces all normal spinlocks nested within > > dma_fence_begin/end_signalling to be spinlocks. That bollocks. > > > > The approach now is to simple check in_atomic(), and for these cases > > entirely rely on the might_sleep() check in dma_fence_wait(). That > > will catch any wrong nesting against spinlocks from soft/hardirq > > contexts. > > > > The idea here is that every code path that's critical for eventually > > signalling a dma_fence should be annotated with > > dma_fence_begin/end_signalling. The annotation ideally starts right > > after a dma_fence is published (added to a dma_resv, exposed as a > > sync_file fd, attached to a drm_syncobj fd, or anything else that > > makes the dma_fence visible to other kernel threads), up to and > > including the dma_fence_wait(). Examples are irq handlers, the > > scheduler rt threads, the tail of execbuf (after the corresponding > > fences are visible), any workers that end up signalling dma_fences and > > really anything else. Not annotated should be code paths that only > > complete fences opportunistically as the gpu progresses, like e.g. > > shrinker/eviction code. > > > > The main class of deadlocks this is supposed to catch are: > > > > Thread A: > > > > mutex_lock(A); > > mutex_unlock(A); > > > > dma_fence_signal(); > > > > Thread B: > > > > mutex_lock(A); > > dma_fence_wait(); > > mutex_unlock(A); > > > > Thread B is blocked on A signalling the fence, but A never gets around > > to that because it cannot acquire the lock A. > > > > Note that dma_fence_wait() is allowed to be nested within > > dma_fence_begin/end_signalling sections. To allow this to happen the > > read lock needs to be upgraded to a write lock, which means that any > > other lock is acquired between the dma_fence_begin_signalling() call an= d > > the call to dma_fence_wait(), and still held, this will result in an > > immediate lockdep complaint. The only other option would be to not > > annotate such calls, defeating the point. Therefore these annotations > > cannot be sprinkled over the code entirely mindless to avoid false > > positives. > > > > v2: handle soft/hardirq ctx better against write side and dont forget > > EXPORT_SYMBOL, drivers can't use this otherwise. > > > > Cc: linux-media@vger.kernel.org > > Cc: linaro-mm-sig@lists.linaro.org > > Cc: linux-rdma@vger.kernel.org > > Cc: amd-gfx@lists.freedesktop.org > > Cc: intel-gfx@lists.freedesktop.org > > Cc: Chris Wilson > > Cc: Maarten Lankhorst > > Cc: Christian K=C3=B6nig > > Signed-off-by: Daniel Vetter > > --- > > drivers/dma-buf/dma-fence.c | 53 +++++++++++++++++++++++++++++++++++++ > > include/linux/dma-fence.h | 12 +++++++++ > > 2 files changed, 65 insertions(+) > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > index 6802125349fb..d5c0fd2efc70 100644 > > --- a/drivers/dma-buf/dma-fence.c > > +++ b/drivers/dma-buf/dma-fence.c > > @@ -110,6 +110,52 @@ u64 dma_fence_context_alloc(unsigned num) > > } > > EXPORT_SYMBOL(dma_fence_context_alloc); > > > > +#ifdef CONFIG_LOCKDEP > > +struct lockdep_map dma_fence_lockdep_map =3D { > > + .name =3D "dma_fence_map" > > +}; > > + > > +bool dma_fence_begin_signalling(void) > > +{ > > + /* explicitly nesting ... */ > > + if (lock_is_held_type(&dma_fence_lockdep_map, 1)) > > + return true; > > + > > + /* rely on might_sleep check for soft/hardirq locks */ > > + if (in_atomic()) > > + return true; > > + > > + /* ... and non-recursive readlock */ > > + lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _RET_IP_); > > + > > + return false; > > +} > > +EXPORT_SYMBOL(dma_fence_begin_signalling); > > Hi Daniel, > > This is great work and could help a lot. > > If you invert the result of dma_fence_begin_signalling() > then it would naturally mean "locked", i.e. whether we need to > later release "dma_fence_lockedep_map". Then, > in dma_fence_end_signalling(), you can call the "cookie" > argument "locked" and simply do: > > void dma_fence_end_signalling(bool locked) > { > if (locked) > lock_release(&dma_fence_lockdep_map, _RET_IP_); > } > EXPORT_SYMBOL(dma_fence_end_signalling); > > It'll be more natural to understand as well. It's intentionally called cookie so callers don't start doing funny stuff with it. The thing is, after begin_signalling you are _always_ in the locked state. It's just that because of limitations with lockdep we need to play a few tricks, and in some cases we do not take the lockdep map. There's 2 cases: - lockdep map already taken - we want recursive readlock semantics for this, but lockdep does not correctly check recursive read locks. Hence we only use readlock, and make sure we do not actually nest upon ourselves with this explicit check. - when we're in atomic sections - lockdep gets pissed at us if we take the read lock in hard/softirq sections because of hard/softirq ctx mismatch (lockdep thinks it's a real lock, but we don't treat it as one). Simplest fix was to rely on the might_sleep check in patch 1 (already merged) The commit message mentions this already a bit, but I'll try to explain this implementation detail tersely in the kerneldoc too in the next round. Thanks, Daniel > > Regards, > Luben > > > + > > +void dma_fence_end_signalling(bool cookie) > > +{ > > + if (cookie) > > + return; > > + > > + lock_release(&dma_fence_lockdep_map, _RET_IP_); > > +} > > +EXPORT_SYMBOL(dma_fence_end_signalling); > > + > > +void __dma_fence_might_wait(void) > > +{ > > + bool tmp; > > + > > + tmp =3D lock_is_held_type(&dma_fence_lockdep_map, 1); > > + if (tmp) > > + lock_release(&dma_fence_lockdep_map, _THIS_IP_); > > + lock_map_acquire(&dma_fence_lockdep_map); > > + lock_map_release(&dma_fence_lockdep_map); > > + if (tmp) > > + lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _T= HIS_IP_); > > +} > > +#endif > > + > > + > > /** > > * dma_fence_signal_locked - signal completion of a fence > > * @fence: the fence to signal > > @@ -170,14 +216,19 @@ int dma_fence_signal(struct dma_fence *fence) > > { > > unsigned long flags; > > int ret; > > + bool tmp; > > > > if (!fence) > > return -EINVAL; > > > > + tmp =3D dma_fence_begin_signalling(); > > + > > spin_lock_irqsave(fence->lock, flags); > > ret =3D dma_fence_signal_locked(fence); > > spin_unlock_irqrestore(fence->lock, flags); > > > > + dma_fence_end_signalling(tmp); > > + > > return ret; > > } > > EXPORT_SYMBOL(dma_fence_signal); > > @@ -211,6 +262,8 @@ dma_fence_wait_timeout(struct dma_fence *fence, boo= l intr, signed long timeout) > > if (timeout > 0) > > might_sleep(); > > > > + __dma_fence_might_wait(); > > + > > trace_dma_fence_wait_start(fence); > > if (fence->ops->wait) > > ret =3D fence->ops->wait(fence, intr, timeout); > > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > > index 3347c54f3a87..3f288f7db2ef 100644 > > --- a/include/linux/dma-fence.h > > +++ b/include/linux/dma-fence.h > > @@ -357,6 +357,18 @@ dma_fence_get_rcu_safe(struct dma_fence __rcu **fe= ncep) > > } while (1); > > } > > > > +#ifdef CONFIG_LOCKDEP > > +bool dma_fence_begin_signalling(void); > > +void dma_fence_end_signalling(bool cookie); > > +#else > > +static inline bool dma_fence_begin_signalling(void) > > +{ > > + return true; > > +} > > +static inline void dma_fence_end_signalling(bool cookie) {} > > +static inline void __dma_fence_might_wait(void) {} > > +#endif > > + > > int dma_fence_signal(struct dma_fence *fence); > > int dma_fence_signal_locked(struct dma_fence *fence); > > signed long dma_fence_default_wait(struct dma_fence *fence, > > > --=20 Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch