linux-kernel - Re: [PATCH v18 15/26] drm/panfrost: Explicitly get and put drm-shmem pages

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: <20231124104053.034496fb@collabora.com>
Date:   Fri, 24 Nov 2023 10:40:53 +0100
From:   Boris Brezillon <boris.brezillon@...labora.com>
To:     Dmitry Osipenko <dmitry.osipenko@...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 v18 15/26] drm/panfrost: Explicitly get and put
 drm-shmem pages

On Thu, 23 Nov 2023 15:48:48 +0100
Boris Brezillon <boris.brezillon@...labora.com> wrote:

> On Mon, 30 Oct 2023 02:01:54 +0300
> Dmitry Osipenko <dmitry.osipenko@...labora.com> wrote:
> 
> > To simplify the drm-shmem refcnt handling, we're moving away from
> > the implicit get_pages() that is used by get_pages_sgt(). From now on
> > drivers will have to pin pages while they use sgt. Panfrost's shrinker
> > doesn't support swapping out BOs, hence pages are pinned and sgt is valid
> > as long as pages' use-count > 0.
> > 
> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@...labora.com>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_gem.c | 17 +++++++++++++++++
> >  drivers/gpu/drm/panfrost/panfrost_mmu.c |  6 ++----
> >  2 files changed, 19 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> > index 6b77d8cebcb2..bb9d43cf7c3c 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> > @@ -47,8 +47,13 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
> >  			}
> >  		}
> >  		kvfree(bo->sgts);
> > +
> > +		drm_gem_shmem_put_pages(&bo->base);
> >  	}
> >  
> > +	if (!bo->is_heap && !obj->import_attach)
> > +		drm_gem_shmem_put_pages(&bo->base);
> > +
> >  	drm_gem_shmem_free(&bo->base);
> >  }
> >  
> > @@ -269,6 +274,7 @@ panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags)
> >  {
> >  	struct drm_gem_shmem_object *shmem;
> >  	struct panfrost_gem_object *bo;
> > +	int err;
> >  
> >  	/* Round up heap allocations to 2MB to keep fault handling simple */
> >  	if (flags & PANFROST_BO_HEAP)
> > @@ -282,7 +288,18 @@ panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags)
> >  	bo->noexec = !!(flags & PANFROST_BO_NOEXEC);
> >  	bo->is_heap = !!(flags & PANFROST_BO_HEAP);
> >  
> > +	if (!bo->is_heap) {
> > +		err = drm_gem_shmem_get_pages(shmem);  
> 
> I really hate the fact we request pages here while we call
> panfrost_mmu_map() in panfrost_gem_open(), because ultimately, pages
> are requested for the MMU mapping. Also hate the quirk we have in shmem
> to call free_pages() instead of put_pages_locked() when the BO refcount
> dropped to zero, and I was hoping we could get rid of it at some point
> by teaching drivers to request pages when they actually need it instead
> of tying pages lifetime to the GEM object lifetime.
> 
> Maybe what we should do instead is move the get/put_pages() in
> panfrost_mmu_map/unmap() (as I suggested), but have a special mapping
> panfrost_mmu_evict/restore() helpers that kill/restore the MMU mappings
> without releasing/acquiring the pages ref.

Okay, so I played with your branch and did what I suggested here ^. The
end result is available here [1]. I also split this patch in two:

- A fix for the error path in panfrost_mmu_map_fault_addr() [2]
- The explicit get/put_pages() stuff with pages ref owned by the
  panfrost_gem_mapping object [3]

[1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commits/virtio-gpu-shrinker-v18
[2]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/9d499e971fdae4d6e52f8871ca27c24b2a2c43d6
[3]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/ba3de65bf1cf0ca95710e743ec85ca67ff1aa223