linux-kernel - Re: [PATCH v17 13/18] drm/shmem-helper: Add memory shrinker

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]

Date:   Tue, 3 Oct 2023 11:00:55 +0200
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 v17 13/18] drm/shmem-helper: Add memory shrinker

Hello Dmitry,

On Tue, 3 Oct 2023 03:31:32 +0300
Dmitry Osipenko <dmitry.osipenko@...labora.com> wrote:

> On 9/26/23 10:35, Boris Brezillon wrote:
> >>>> +	__drm_gem_shmem_release_pages(shmem);    
> >>> Make sure you drop the implicit pages_use_count ref the sgt had, this
> >>> way you can still tie the necessity to drop the pages to sgt != NULL in
> >>> drm_gem_shmem_free().    
> >> This will require further refcnt re-initialization when pages are
> >> restored if it's dropped to zero. I don't see how this will improve
> >> anything.  
> > Sorry to disagree, but I do think it matters to have a clear ownership
> > model, and if I look at the code (drm_gem_shmem_get_pages_sgt_locked()),
> > the sgt clearly owns a reference to the pages it points to.  
> 
> It creates too much unnecessary trouble because, again, pages_use_count
> can't drop to zero easily.

Not saying pages_use_count should drop to zero, I'm just saying the
reference that was owned by the sgt should be released when this sgt is
freed, no matter if this sgt destruction is triggered by a GEM eviction,
or because the GEM object is freed entirely.

> Shrinker doesn't own the refcnt and not
> allowed to touch it.

I'm not asking the shrinker to own a reference on the pages either.
It's really the sgt that owns this reference.

> The pages_use_count is then used by things like
> mmap() and etc that use get_pages(), which can be invoked for evicted GEM.

Yes, and I still have a hard time seeing how this interferes with what
I'm suggesting to be honest.

> 
> I'd prefer to keep refcounting as is, don't see how to implement your
> suggestion.

Can you be more specific? I don't really see what the problem is with
decrementing pages_use_count when you free the sgt (eviction), and
re-incrementing it when the sgt is restored (swapin).

Regards,

Boris