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.
It sounds like refcount_t should have an refcount_inc_maybe_first()
where 0->1 is ok.
Alice
Alice
