On Mon, 12 Jan 2026 16:51:26 +0100 Alice Ryhl <[email protected]> wrote:
> On Mon, Jan 12, 2026 at 4:49 PM Boris Brezillon > <[email protected]> wrote: > > > > On Mon, 12 Jan 2026 16:19:52 +0100 > > Alice Ryhl <[email protected]> wrote: > > > > > On Mon, Jan 12, 2026 at 3:40 PM Boris Brezillon > > > <[email protected]> wrote: > > > > > > > > On Mon, 12 Jan 2026 12:33:33 +0000 > > > > Steven Price <[email protected]> wrote: > > > > > > > > > On 09/01/2026 13:08, Boris Brezillon wrote: > > > > > > +static void panthor_gem_vm_close(struct vm_area_struct *vma) > > > > > > +{ > > > > > > + struct panthor_gem_object *bo = > > > > > > to_panthor_bo(vma->vm_private_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); > > > > > > > > > > I don't think this logic is safe. Holding the resv_lock doesn't > > > > > protect > > > > > against another thread doing a refcount_inc_not_zero() without holding > > > > > 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. > > > > > > > > I wish I could, but refcount_dec() doesn't let me do the 1 -> 0 without > > > > complaining :P. > > > > > > I'm pretty sure that refcount_dec() is fine with 1->0. > > > > 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. > > You're right, I mixed it up with refcount_dec_and_test(). > > > > > > 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); > > > > > } > > > > > > > > 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)". > > > > > > 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. > > > > 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/refcount.h#L460 > > > > It's just a suggestion - no need to block on it. Sure, np. > > 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.
