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

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 2 Oct 2023 22:28:13 +0300
Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:

> On 9/26/23 10:35, Boris Brezillon wrote:
> >> On 9/15/23 11:46, Boris Brezillon wrote:  
> >>> The naming becomes quite confusing, with drm_gem_shmem_unpin_locked()
> >>> and drm_gem_shmem_unpin_pages_locked(). By the look of it, it seems to
> >>> do exactly the opposite of drm_gem_shmem_swapin_locked(), except for
> >>> the missing ->evicted = true, which we can move here anyway, given
> >>> drm_gem_shmem_purge_locked() explicitly set it to false anyway. The
> >>> other thing that's missing is the
> >>> drm_gem_shmem_update_pages_state_locked(), but it can also be moved
> >>> there I think, if the the ->madv update happens before the
> >>> drm_gem_shmem_unpin_pages_locked() call in
> >>> drm_gem_shmem_purge_locked().
> >>>
> >>> So, how about renaming this function drm_gem_shmem_swapout_locked()?    
> >> The swapout name would be misleading to me because pages aren't moved to
> >> swap, but allowed to be moved. I'll rename it to
> >> drm_gem_shmem_shrinker_unpin_locked().  
> > If you go this way, I would argue that drm_gem_shmem_swapin_locked() is
> > just as incorrect as drm_gem_shmem_swapout_locked(), in that
> > drm_gem_get_pages() might just return pages that were flagged
> > reclaimable but never reclaimed/swapped-out. I do think that having
> > some symmetry in the naming makes more sense than being 100% accurate.  
> 
> That function is internal to drm-shmem and is used for both eviction and
> purging. Having "swap-out" invoked by the purging also doesn't sound good.

The part that discards the GEM in the shmem file is outside this
function (shmem_truncate_range()), so all this function does in practice
is flag the pages as evictable (or rather, clear the unevictable flag),
so they can be reclaimed. The swapout suggesting was mostly based on
the fact it does exactly the opposite of swapin().

> 
> Given that the function in question mainly "unmaps" the pages, what
> about drm_gem_shmem_shkinker_unmap_pages_locked()?

Unmap tends to refer to a VM related operation (removing a mapping in
the CPU or GPU VM), so it's confusing too IMHO. What we do here is
return pages to the shmem file logic, so they can be reclaimed.

Given the drm_gem function doing that is called drm_gem_put_pages(),
maybe rename it drm_gem_shmem_shrinker_put_pages_locked(), and rename
drm_gem_shmem_swapin_locked() into
drm_gem_shmem_shrinker_get_pages_locked(), to be consistent.

> 
> >>>>  {
> >>>>  	struct drm_gem_object *obj = &shmem->base;
> >>>>  	struct drm_device *dev = obj->dev;
> >>>>  
> >>>>  	dma_resv_assert_held(shmem->base.resv);
> >>>>  
> >>>> -	drm_WARN_ON(obj->dev, !drm_gem_shmem_is_purgeable(shmem));
> >>>> +	if (shmem->evicted)
> >>>> +		return;
> >>>>  
> >>>>  	dma_unmap_sgtable(dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0);    
> >>> Are we sure we'll always have sgt != NULL? IIRC, if the GEM is only
> >>> mmap-ed in userspace, get_sgt() is not necessarily called by the driver
> >>> (needed to map in GPU space), and we have a potential NULL deref here.
> >>> Maybe that changed at some point in the series, and sgt is
> >>> unconditionally populated when get_pages() is called now.    
> >> The sgt is always set in this function because it's part of shrinker and
> >> shrinker doesn't touch GEMs without sgt.  
> > Okay, that's questionable. Why would we not want to reclaim BOs that
> > are only mapped in userspace (sgt == NULL && pages_use_count > 0 &&
> > pages_pin_count == 0)? I agree that creating such a BO would be
> > pointless (why create a buffer through DRM if it's not passed to the
> > GPU), but that's still something the API allows...  
> 
> This is a pre-existing behaviour. There is no driver that uses pages
> without sgt, hence there is nobody to test such code paths.
> 
> Maybe will worth to explicitly prohibit usage of get_pages() without
> having sgt for clarity.

Nope, I don't think we should. Panthor is dissociating the BO creation
for the GPU VM map operation, meaning we only get to ask for an sgt when
the BO is first mapped in GPU space. In the meantime, the shrinker logic
might decide to evict an object that has been already CPU-mapped (using
mmap()).

> But this should be separate to this patchset, IMO.

FYI, I'm not being picky just for fun, I do intend to use the
shmem-shrinker in panthor at some point, and I think it's important to
get it right from the beginning, even if all your existing users don't
care. I mean, I would understand if what I was asking was requiring
heavy changes to the existing logic, but, unless I'm wrong, I don't
think it does.