From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 4D2EEC9EC9A for ; Mon, 12 Jan 2026 16:07:10 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id A362410E402; Mon, 12 Jan 2026 16:07:09 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=collabora.com header.i=@collabora.com header.b="oIPgEntU"; dkim-atps=neutral Received: from bali.collaboradmins.com (bali.collaboradmins.com [148.251.105.195]) by gabe.freedesktop.org (Postfix) with ESMTPS id 6FEF310E402 for ; Mon, 12 Jan 2026 16:07:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1768234027; bh=qf2liF3PRGdd5V9MVGiiYjiIaPACQ6TvC6haw+nVM60=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=oIPgEntUkn0gJZSCgScT3rRPQBpcJTQd79KqHzeJTVJChq9gfBr4ZjZBtLTd10yqX oW/KgknSezNyFrvcwmoEoLnryxtpJt2rrXOTC8gViYZrDrMBW1iFiQcsalF+f7U50A sI2PIayLga/8GLw7c7AT2uM+9/JBBFNbZkvOGPQvprjU2ztjcEsE7I2tyZjlSDwWTm 4X4g0DyrezWaGH0o7frB2jJvaqhQt3LDScRTPVLmYvtmUhF8++D6zg8sKAxTEYgMHP tgJrKLKf2yL4W8rldatrO07WthFKk4ucwgejlIbNUIV+6QsSoiqS/8d7tcGNpga1FM 24jqpU4I9kLLA== Received: from fedora (unknown [IPv6:2a01:e0a:2c:6930:d919:a6e:5ea1:8a9f]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (prime256v1) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: bbrezillon) by bali.collaboradmins.com (Postfix) with ESMTPSA id 424A417E0A49; Mon, 12 Jan 2026 17:07:06 +0100 (CET) Date: Mon, 12 Jan 2026 17:06:58 +0100 From: Boris Brezillon To: Alice Ryhl Cc: Steven Price , Liviu Dudau , =?UTF-8?B?QWRyacOhbg==?= Larumbe , dri-devel@lists.freedesktop.org, David Airlie , Simona Vetter , Akash Goel , Rob Clark , Sean Paul , Konrad Dybcio , Akhil P Oommen , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , Dmitry Osipenko , Chris Diamand , Danilo Krummrich , Matthew Brost , Thomas =?UTF-8?B?SGVsbHN0csO2bQ==?= , kernel@collabora.com Subject: Re: [PATCH v1 8/9] drm/panthor: Track the number of mmap on a BO Message-ID: <20260112170658.1bda2573@fedora> In-Reply-To: References: <20260109130801.1239558-1-boris.brezillon@collabora.com> <20260109130801.1239558-9-boris.brezillon@collabora.com> <20260112153953.61eb20dc@fedora> <20260112164908.02d25584@fedora> Organization: Collabora X-Mailer: Claws Mail 4.3.1 (GTK 3.24.51; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Mon, 12 Jan 2026 16:51:26 +0100 Alice Ryhl wrote: > On Mon, Jan 12, 2026 at 4:49=E2=80=AFPM Boris Brezillon > wrote: > > > > On Mon, 12 Jan 2026 16:19:52 +0100 > > Alice Ryhl wrote: > > =20 > > > On Mon, Jan 12, 2026 at 3:40=E2=80=AFPM Boris Brezillon > > > wrote: =20 > > > > > > > > On Mon, 12 Jan 2026 12:33:33 +0000 > > > > Steven Price wrote: > > > > =20 > > > > > On 09/01/2026 13:08, Boris Brezillon wrote: =20 > > > > > > +static void panthor_gem_vm_close(struct vm_area_struct *vma) > > > > > > +{ > > > > > > + struct panthor_gem_object *bo =3D to_panthor_bo(vma->vm_pri= vate_data); > > > > > > + > > > > > > + if (drm_gem_is_imported(&bo->base)) > > > > > > + goto out; > > > > > > + > > > > > > + if (refcount_dec_not_one(&bo->cmap.mmap_count)) > > > > > > + goto out; > > > > > > + > > > > > > + dma_resv_lock(bo->base.resv, NULL); > > > > > > + if (!refcount_dec_not_one(&bo->cmap.mmap_count)) > > > > > > + refcount_set(&bo->cmap.mmap_count, 0); > > > > > > + dma_resv_unlock(bo->base.resv); =20 > > > > > > > > > > I don't think this logic is safe. Holding the resv_lock doesn't p= rotect > > > > > against another thread doing a refcount_inc_not_zero() without ho= lding > > > > > the lock. > > > > > > > > > > I think you can just replace the if() part with a refcount_dec() = call, > > > > > the lock AFAICT is needed because the following patch wants to be= sure > > > > > that !!mmap_count is stable when resv_lock is held. =20 > > > > > > > > I wish I could, but refcount_dec() doesn't let me do the 1 -> 0 wit= hout > > > > complaining :P. =20 > > > > > > I'm pretty sure that refcount_dec() is fine with 1->0. =20 > > > > That's not what [1] says. refcount_dec_and_test() is okay though, but > > it'd force us to do a > > > > (void)refcount_dec_and_test() > > > > and detail why it's okay to ignore the returned value. Not too sure > > which one is better. =20 >=20 > You're right, I mixed it up with refcount_dec_and_test(). >=20 > > > > > I also feel you should invert the conditino for refcount_dec_not_= one, > > > > > leading to the following which I feel is easier to read: > > > > > > > > > > static void panthor_gem_vm_close(struct vm_area_struct *vma) > > > > > { > > > > > [...] > > > > > > > > > > if (!refcount_dec_not_one(&bo->cmap.mmap_count)) { > > > > > dma_resv_lock(bo->base.resv, NULL); > > > > > refcount_dec(&bo->cmap.mmap_count); > > > > > dma_resv_unlock(bo->base.resv); > > > > > } =20 > > > > > > > > The best I can do is: > > > > > > > > if (!refcount_dec_not_one(&bo->cmap.mmap_count)) { > > > > dma_resv_lock(bo->base.resv, NULL); > > > > if (!refcount_dec_not_one(&bo->cmap.mmap_count)) > > > > refcount_set(&bo->cmap.mmap_count, 0); > > > > dma_resv_unlock(bo->base.resv); > > > > } > > > > > > > > so we only take the lock when absolutely needed, but the 1 -> 0 > > > > transition still has to be done with "if (dec_not_one) set(0)". =20 > > > > > > Why not just use atomic_t and use the atomic inc/dec operations? They > > > don't have saturation, but also do not require treating zero > > > specially. =20 > > > > I had suggested using atomics back when I was reviewing the > > shmem-shrinker stuff to avoid this exact same issue. I can't find the > > thread anymore and I can't remember the rationale either (probably that > > saturation detection was useful, still), but the decision was to use a > > refcount_t. I don't mind using atomics here, but I'd rather not be > > blocked on that when/if I try to move that code into a common lib. > > > > [1]https://elixir.bootlin.com/linux/v6.19-rc4/source/include/linux/refc= ount.h#L460 =20 >=20 > It's just a suggestion - no need to block on it. Sure, np. >=20 > It sounds like refcount_t should have an refcount_inc_maybe_first() > where 0->1 is ok. We actually need refcount_dec_maybe_last() in this context, but I get the idea. Don't know if the "ATOMIC INFRASTRUCTURE" maintainers would be okay with this idea though, since it's silencing the "do something special when your counter reaches zero" behavior enforced by the refcount API, and it's not that often that resources are released lazily like that anyway, so maybe the extra line is not that big of a deal.