From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout-p-102.mailbox.org (mout-p-102.mailbox.org [80.241.56.152]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6B5BD3D7D99 for ; Mon, 15 Jun 2026 10:04:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=80.241.56.152 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781517888; cv=none; b=JvYT8jGzhX7E78/TmZAhwhV8n4A8wtDGuWFzqEm/BWcy77zbpilhSMITUeJqVMyNl2gUQ1EgO5aIpRWstqjcOEA9sIggkcbcD837I2ClGOMCIJIgJawyP4UaNRo8NGQAguUQCGCNTX5RUtGqvtEWNeT55MTNdPiGWiZ8mRe9y4c= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781517888; c=relaxed/simple; bh=ZmwSTAZThB7f2650t5CaRPc7nEj2VGldsa+fq9k/ChI=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=PwubqEFD/Km0evraJYImUo8h8o38LYTNkA8kqBi8aLGn7jMGT7arlA8ZYGHZP93hpVTf+l7xMSyr4rHehCpjzNe67dcxbJIQ89v4mVDpMVMEhFmSl8k8YNpKMEKk0Gf4TgMvG37CvwHgsoolC83UmwjSLnwbhVnjTr5KZZ8yyvo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=mailbox.org; spf=pass smtp.mailfrom=mailbox.org; dkim=pass (2048-bit key) header.d=mailbox.org header.i=@mailbox.org header.b=lYXH3qjU; arc=none smtp.client-ip=80.241.56.152 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=mailbox.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=mailbox.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=mailbox.org header.i=@mailbox.org header.b="lYXH3qjU" Received: from smtp102.mailbox.org (smtp102.mailbox.org [IPv6:2001:67c:2050:b231:465::102]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-p-102.mailbox.org (Postfix) with ESMTPS id 4gf5M94sSVz9vD9; Mon, 15 Jun 2026 12:04:37 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mailbox.org; s=mail20150812; t=1781517877; h=from:from:reply-to: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=vcMfyWCmScqAXuIuZfOPShBN7wAdoe95v7+nL69H0sE=; b=lYXH3qjUvqrL/6mAF0LaPbLonPIQO3HtOnWoUVS4q0V4iT4LF7naeR9xtXNWv34jMPNRrS GflRTrWTsxB4rbS1HNfy7a4n2H4A7a1y7kW/hLEyTIwUUlH1kbKDTVeAkgRJn+EMXY7vwO 374k4z/edg1ACd4rI12mxcmuYlDd37FX/zs6NIxSHP96tK/FOBcQj212PNwv+19OZ4nEap nkLsMijRxqWTdQu+p9r+7VZYjAXwQaZbYPdaczNWMY2QkKQHbRFsoRq2cqlGQ62YjA9dvq E0XS9dSQ71rhGchKPZA2TsEV0x+GY7I11ZHA9S8BVyFOPuS1Vq6Fklk2yeipqQ== Message-ID: Subject: Re: [RFC PATCH] dma-buf/dma_fence: Make races for dma_fence_is_signaled() less likely From: Philipp Stanner Reply-To: phasta@kernel.org To: Christian =?ISO-8859-1?Q?K=F6nig?= , Philipp Stanner , Danilo Krummrich , Maarten Lankhorst , David Airlie , Simona Vetter , Sumit Semwal , Tvrtko Ursulin , Boris Brezillon , "Paul E . McKenney" Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Date: Mon, 15 Jun 2026 12:04:32 +0200 In-Reply-To: <600885fc-7e07-4713-b5c2-a470637040c8@amd.com> References: <20260612104251.2264707-2-phasta@kernel.org> <600885fc-7e07-4713-b5c2-a470637040c8@amd.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-MBO-RS-ID: 5882d3cc14375d1b852 X-MBO-RS-META: 94478hzu1jzm6hdcatri5kbhpubb7dfn On Mon, 2026-06-15 at 11:53 +0200, Christian K=C3=B6nig wrote: > On 6/12/26 12:42, Philipp Stanner wrote: > > dma_fence_is_signaled() returns whether a fence has been signaled > > already. That function contains a fast path opportunistic check which i= s > > not guarded by the lock and, according to Christian, cannot be guarded > > by the lock without causing a massive performance regression. > >=20 > > This now means that dma_fence_is_signaled() can return true WHILE the > > fence callbacks are still being executed. This is razy and has lead to > > at least one bug solved in: > >=20 > > commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race") > >=20 > > Make this race impossible, by simply setting the bit only once the > > callbacks are actually completed. >=20 > Groundhog day, that has been suggested before and it simply doesn't work. >=20 > The flag is intentional set before calling the callbacks because the stat= e needs to be visible. It will be visible. Just later. >=20 > Just see dma_fence_default_wait() for an example why that approach doesn'= t work. What's the issue? It will be set. Just later. Who is ordering with whom? I BTW suggest to write more code comments in the future to document all these supposed pitfalls for those who will hack on that code base once we have left. P. >=20 > Regards, > Christian. >=20 > >=20 > > Signed-off-by: Philipp Stanner > > --- > > =C2=A0drivers/dma-buf/dma-fence.c | 18 ++++++++++++++++-- > > =C2=A01 file changed, 16 insertions(+), 2 deletions(-) > >=20 > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > > index c7ea1e75d38a..2416cc86ce93 100644 > > --- a/drivers/dma-buf/dma-fence.c > > +++ b/drivers/dma-buf/dma-fence.c > > @@ -359,8 +359,19 @@ void dma_fence_signal_timestamp_locked(struct dma_= fence *fence, > > =C2=A0 > > =C2=A0 dma_fence_assert_held(fence); > > =C2=A0 > > - if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, > > - =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 &fence->flags))) > > + /* > > + * First test the bit, so we don't signal an already signaled fence a= gain. > > + * The lock protects against multiple parties setting the bit. The bi= t > > + * is then set at the end of the function. > > + * > > + * The background is that there is a fast path check in > > + * dma_fence_is_signaled() which does not use lock protection and can > > + * return true *while* the fence callbacks are still executing. > > + * > > + * This fast path check supposedly cannot be guarded by the lock beca= use > > + * of significant performance regressions. > > + */ > > + if (unlikely(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))) > > =C2=A0 return; > > =C2=A0 > > =C2=A0 trace_dma_fence_signaled(fence); > > @@ -384,6 +395,9 @@ void dma_fence_signal_timestamp_locked(struct dma_f= ence *fence, > > =C2=A0 INIT_LIST_HEAD(&cur->node); > > =C2=A0 cur->func(fence, cur); > > =C2=A0 } > > + > > + // TODO: we need some barrier here, don't we? > > + set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); > > =C2=A0} > > =C2=A0EXPORT_SYMBOL(dma_fence_signal_timestamp_locked); > > =C2=A0