Re: [PATCH v4 11/15] drm/shmem-helper: Add generic memory shrinker

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, May 12, 2022 at 10:04:53PM +0300, Dmitry Osipenko wrote:
> On 5/12/22 20:04, Daniel Vetter wrote:
> > On Thu, 12 May 2022 at 13:36, Dmitry Osipenko
> > <dmitry.osipenko@collabora.com> wrote:
> >>
> >> On 5/11/22 22:09, Daniel Vetter wrote:
> >>> On Wed, May 11, 2022 at 07:06:18PM +0300, Dmitry Osipenko wrote:
> >>>> On 5/11/22 16:09, Daniel Vetter wrote:
> >>>>>>>>> I'd like to ask you to reduce the scope of the patchset and build the
> >>>>>>>>> shrinker only for virtio-gpu. I know that I first suggested to build
> >>>>>>>>> upon shmem helpers, but it seems that it's easier to do that in a later
> >>>>>>>>> patchset.
> >>>>>>>> The first version of the VirtIO shrinker didn't support memory eviction.
> >>>>>>>> Memory eviction support requires page fault handler to be aware of the
> >>>>>>>> evicted pages, what should we do about it? The page fault handling is a
> >>>>>>>> part of memory management, hence to me drm-shmem is already kinda a MM.
> >>>>>>> Hm I still don't get that part, why does that also not go through the
> >>>>>>> shmem helpers?
> >>>>>> The drm_gem_shmem_vm_ops includes the page faults handling, it's a
> >>>>>> helper by itself that is used by DRM drivers.
> >>>>>>
> >>>>>> I could try to move all the shrinker logic to the VirtIO and re-invent
> >>>>>> virtio_gem_shmem_vm_ops, but what is the point of doing this for each
> >>>>>> driver if we could have it once and for all in the common drm-shmem code?
> >>>>>>
> >>>>>> Maybe I should try to factor out all the shrinker logic from drm-shmem
> >>>>>> into a new drm-shmem-shrinker that could be shared by drivers? Will you
> >>>>>> be okay with this option?
> >>>>> I think we're talking past each another a bit. I'm only bringing up the
> >>>>> purge vs eviction topic we discussed in the other subthread again.
> >>>>
> >>>> Thomas asked to move the whole shrinker code to the VirtIO driver and
> >>>> I's saying that this is not a great idea to me, or am I misunderstanding
> >>>> the Thomas' suggestion? Thomas?
> >>>
> >>> I think it was just me creating a confusion here.
> >>>
> >>> fwiw I do also think that shrinker in shmem helpers makes sense, just in
> >>> case that was also lost in confusion.
> >>
> >> Okay, good that we're on the same page now.
> >>
> >>>>>>> I'm still confused why drivers need to know the difference
> >>>>>>> between evition and purging. Or maybe I'm confused again.
> >>>>>> Example:
> >>>>>>
> >>>>>> If userspace uses IOV addresses, then these addresses must be kept
> >>>>>> reserved while buffer is evicted.
> >>>>>>
> >>>>>> If BO is purged, then we don't need to retain the IOV space allocated
> >>>>>> for the purged BO.
> >>>>> Yeah but is that actually needed by anyone? If userspace fails to allocate
> >>>>> another bo because of lack of gpu address space then it's very easy to
> >>>>> handle that:
> >>>>>
> >>>>> 1. Make a rule that "out of gpu address space" gives you a special errno
> >>>>> code like ENOSPC
> >>>>>
> >>>>> 2. If userspace gets that it walks the list of all buffers it marked as
> >>>>> purgeable and nukes them (whether they have been evicted or not). Then it
> >>>>> retries the bo allocation.
> >>>>>
> >>>>> Alternatively you can do step 2 also directly from the bo alloc ioctl in
> >>>>> step 1. Either way you clean up va space, and actually a lot more (you
> >>>>> potentially nuke all buffers marked as purgeable, not just the ones that
> >>>>> have been purged already) and only when va cleanup is actually needed
> >>>>>
> >>>>> Trying to solve this problem at eviction time otoh means:
> >>>>> - we have this difference between eviction and purging
> >>>>> - it's still not complete, you still need to glue step 2 above into your
> >>>>>   driver somehow, and once step 2 above is glued in doing additional
> >>>>>   cleanup in the purge function is just duplicated logic
> >>>>>
> >>>>> So at least in my opinion this isn't the justification we need. And we
> >>>>> should definitely not just add that complication "in case, for the
> >>>>> future", if we don't have a real need right now. Adding it later on is
> >>>>> easy, removing it later on because it just gets in the way and confuses is
> >>>>> much harder.
> >>>>
> >>>> The IOVA space is only one example.
> >>>>
> >>>> In case of the VirtIO driver, we may have two memory allocation for a
> >>>> BO. One is the shmem allcation in guest and the other is in host's vram.
> >>>> If we will only release the guest's memory on purge, then the vram will
> >>>> remain allocated until BO is destroyed, which unnecessarily sub-optimal.
> >>>
> >>> Hm but why don't you just nuke the memory on the host side too when you
> >>> evict? Allowing the guest memory to be swapped out while keeping the host
> >>> memory allocation alive also doesn't make a lot of sense for me. Both can
> >>> be recreated (I guess at least?) on swap-in.
> >>
> >> Shouldn't be very doable or at least worth the efforts. It's userspace
> >> that manages data uploading, kernel only provides transport for the
> >> virtio-gpu commands.
> >>
> >> Drivers are free to use the same function for both purge() and evict()
> >> callbacks if they want. Getting rid of the purge() callback creates more
> >> problems than solves, IMO.
> > 
> > Hm this still sounds pretty funny and defeats the point of
> > purgeable/evictable buffers a bit I think. But also I guess we'd
> > pushed this bikeshed to the max, so I think if you make ->purge
> > optional and just call ->evict if that's not present, and document it
> > all in the kerneldoc, then I think that's good.
> 
> This is a good enough compromise to me.
> 
> > I just don't think that encouraging drivers to distinguish between
> > evict/purge is a good idea for almost all of them.
> 
> Intel's shrinker checks the "madvise" status of BOs and then decides
> what to do based on it. Perhaps we could move the decision-making about
> purging to drivers and then it will be single evict() callback, but will
> drivers really ever need to be responsible for this decision-making or
> this will be an unnecessary boilerplate code in the drivers? I'll think
> more about this.

tbh I wouldn't worry about details, you've convinced me that some
differentiation between evict and purged makes sense. And yeah maybe
drivers should have a helper to check that instead of explicit argument,
but that's a bikeshed color choice which should be fairly easy to adjust
later on still.

> Thank you all for taking time to look at this patchset. I'm preparing
> the new version.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization