linux-kernel - Re: [PATCH v14 02/12] drm/shmem-helper: Add pages_pin

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4c5fa735-9bfd-f92a-8deb-888c7368f89e@collabora.com>
Date:   Mon, 31 Jul 2023 15:27:31 +0300
From:   Dmitry Osipenko <dmitry.osipenko@...labora.com>
To:     Boris Brezillon <boris.brezillon@...labora.com>
Cc:     David Airlie <airlied@...il.com>,
        Gerd Hoffmann <kraxel@...hat.com>,
        Gurchetan Singh <gurchetansingh@...omium.org>,
        Chia-I Wu <olvaffe@...il.com>, Daniel Vetter <daniel@...ll.ch>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Christian König <christian.koenig@....com>,
        Qiang Yu <yuq825@...il.com>,
        Steven Price <steven.price@....com>,
        Emma Anholt <emma@...olt.net>, Melissa Wen <mwen@...lia.com>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        kernel@...labora.com, virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH v14 02/12] drm/shmem-helper: Add pages_pin_count field

On 7/25/23 11:32, Boris Brezillon wrote:
>> Can we make it an atomic_t, so we can avoid taking the lock when the
>> GEM has already been pinned. That's something I need to be able to grab
>> a pin-ref in a path where the GEM resv lock is already held[1]. We could
>> of course expose the locked version,
> My bad, that's actually not true. The problem is not that I call
> drm_gem_shmem_pin() with the resv lock already held, but that I call
> drm_gem_shmem_pin() in a dma-signaling path where I'm not allowed to
> take a resv lock. I know for sure pin_count > 0, because all GEM objects
> mapped to a VM have their memory pinned right now, and this should
> stand until we decide to add support for live-GEM eviction, at which
> point we'll probably have a way to detect when a GEM is evicted, and
> avoid calling drm_gem_shmem_pin() on it.
> 
> TLDR; I can't trade the atomic_t for a drm_gem_shmem_pin_locked(),
> because that wouldn't solve my problem. The other solution would be to
> add an atomic_t at the driver-GEM level, and only call
> drm_gem_shmem_[un]pin() on 0 <-> 1 transitions, but I thought using an
> atomic at the GEM-shmem level, to avoid locking when we can, would be
> beneficial to the rest of the eco-system. Let me know if that's not an
> option, and I'll go back to the driver-specific atomic_t.

Could you please explain why do you need to pin GEM in a signal handler?
This is not something drivers usually do or need to do. You likely also
shouldn't need to detect that GEM is evicted in yours driver. I'd expect
that Panthor shouldn't differ from Panfrost in regards to how GEM memory
management is done and Panfrost doesn't need to do anything special.

Note that patch #14 makes locked pin/unpin functions public and turns
the unlocked variants into helpers, you'll be able to experiment with
these funcs in the Panthor driver.

In general, using atomic_t or kref should be a good thing to do, but
AFAICS it shouldn't bring benefits to the today's drm-shmem users. I'd
want to understand what you're trying to achieve in the Panthor driver.

-- 
Best regards,
Dmitry