On Wed, Mar 16, 2022 at 5:13 PM Dmitry Osipenko
<dmitry.osipenko@xxxxxxxxxxxxx> wrote:
>
> On 3/16/22 23:00, Rob Clark wrote:
> > On Mon, Mar 14, 2022 at 3:44 PM Dmitry Osipenko
> > <dmitry.osipenko@xxxxxxxxxxxxx> wrote:
> >>
> >> Introduce a common DRM SHMEM shrinker. It allows to reduce code
> >> duplication among DRM drivers, it also handles complicated lockings
> >> for the drivers. This is initial version of the shrinker that covers
> >> basic needs of GPU drivers.
> >>
> >> This patch is based on a couple ideas borrowed from Rob's Clark MSM
> >> shrinker and Thomas' Zimmermann variant of SHMEM shrinker.
> >>
> >> GPU drivers that want to use generic DRM memory shrinker must support
> >> generic GEM reservations.
> >>
> >> Signed-off-by: Daniel Almeida <daniel.almeida@xxxxxxxxxxxxx>
> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/drm_gem_shmem_helper.c | 194 +++++++++++++++++++++++++
> >> include/drm/drm_device.h | 4 +
> >> include/drm/drm_gem.h | 11 ++
> >> include/drm/drm_gem_shmem_helper.h | 25 ++++
> >> 4 files changed, 234 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> index 37009418cd28..35be2ee98f11 100644
> >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> @@ -139,6 +139,9 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
> >> {
> >> struct drm_gem_object *obj = &shmem->base;
> >>
> >> + /* take out shmem GEM object from the memory shrinker */
> >> + drm_gem_shmem_madvise(shmem, 0);
> >> +
> >> WARN_ON(shmem->vmap_use_count);
> >>
> >> if (obj->import_attach) {
> >> @@ -163,6 +166,42 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
> >> }
> >> EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
> >>
> >> +static void drm_gem_shmem_update_purgeable_status(struct drm_gem_shmem_object *shmem)
> >> +{
> >> + struct drm_gem_object *obj = &shmem->base;
> >> + struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker;
> >> + size_t page_count = obj->size >> PAGE_SHIFT;
> >> +
> >> + if (!gem_shrinker || obj->import_attach || !obj->funcs->purge)
> >> + return;
> >> +
> >> + mutex_lock(&shmem->vmap_lock);
> >> + mutex_lock(&shmem->pages_lock);
> >> + mutex_lock(&gem_shrinker->lock);
> >> +
> >> + if (shmem->madv < 0) {
> >> + list_del_init(&shmem->madv_list);
> >> + goto unlock;
> >> + } else if (shmem->madv > 0) {
> >> + if (!list_empty(&shmem->madv_list))
> >> + goto unlock;
> >> +
> >> + WARN_ON(gem_shrinker->shrinkable_count + page_count < page_count);
> >> + gem_shrinker->shrinkable_count += page_count;
> >> +
> >> + list_add_tail(&shmem->madv_list, &gem_shrinker->lru);
> >> + } else if (!list_empty(&shmem->madv_list)) {
> >> + list_del_init(&shmem->madv_list);
> >> +
> >> + WARN_ON(gem_shrinker->shrinkable_count < page_count);
> >> + gem_shrinker->shrinkable_count -= page_count;
> >> + }
> >> +unlock:
> >> + mutex_unlock(&gem_shrinker->lock);
> >> + mutex_unlock(&shmem->pages_lock);
> >> + mutex_unlock(&shmem->vmap_lock);
> >> +}
> >> +
> >> static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
> >> {
> >> struct drm_gem_object *obj = &shmem->base;
> >> @@ -366,6 +405,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
> >> ret = drm_gem_shmem_vmap_locked(shmem, map);
> >> mutex_unlock(&shmem->vmap_lock);
> >>
> >> + drm_gem_shmem_update_purgeable_status(shmem);
> >> +
> >> return ret;
> >> }
> >> EXPORT_SYMBOL(drm_gem_shmem_vmap);
> >> @@ -409,6 +450,8 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
> >> mutex_lock(&shmem->vmap_lock);
> >> drm_gem_shmem_vunmap_locked(shmem, map);
> >> mutex_unlock(&shmem->vmap_lock);
> >> +
> >> + drm_gem_shmem_update_purgeable_status(shmem);
> >> }
> >> EXPORT_SYMBOL(drm_gem_shmem_vunmap);
> >>
> >> @@ -451,6 +494,8 @@ int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv)
> >>
> >> mutex_unlock(&shmem->pages_lock);
> >>
> >> + drm_gem_shmem_update_purgeable_status(shmem);
> >> +
> >> return (madv >= 0);
> >> }
> >> EXPORT_SYMBOL(drm_gem_shmem_madvise);
> >> @@ -763,6 +808,155 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
> >> }
> >> EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
> >>
> >> +static struct drm_gem_shmem_shrinker *
> >> +to_drm_shrinker(struct shrinker *shrinker)
> >> +{
> >> + return container_of(shrinker, struct drm_gem_shmem_shrinker, base);
> >> +}
> >> +
> >> +static unsigned long
> >> +drm_gem_shmem_shrinker_count_objects(struct shrinker *shrinker,
> >> + struct shrink_control *sc)
> >> +{
> >> + struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker);
> >> + u64 count = gem_shrinker->shrinkable_count;
> >> +
> >> + if (count >= SHRINK_EMPTY)
> >> + return SHRINK_EMPTY - 1;
> >> +
> >> + return count ?: SHRINK_EMPTY;
> >> +}
> >> +
> >> +static unsigned long
> >> +drm_gem_shmem_shrinker_scan_objects(struct shrinker *shrinker,
> >> + struct shrink_control *sc)
> >> +{
> >> + struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker);
> >> + struct drm_gem_shmem_object *shmem;
> >> + struct list_head still_in_list;
> >> + bool lock_contention = true;
> >> + struct drm_gem_object *obj;
> >> + unsigned long freed = 0;
> >> +
> >> + INIT_LIST_HEAD(&still_in_list);
> >> +
> >> + mutex_lock(&gem_shrinker->lock);
> >> +
> >> + while (freed < sc->nr_to_scan) {
> >> + shmem = list_first_entry_or_null(&gem_shrinker->lru,
> >> + typeof(*shmem), madv_list);
> >> + if (!shmem)
> >> + break;
> >> +
> >> + obj = &shmem->base;
> >> + list_move_tail(&shmem->madv_list, &still_in_list);
> >> +
> >> + /*
> >> + * If it's in the process of being freed, gem_object->free()
> >> + * may be blocked on lock waiting to remove it. So just
> >> + * skip it.
> >> + */
> >> + if (!kref_get_unless_zero(&obj->refcount))
> >> + continue;
> >> +
> >> + mutex_unlock(&gem_shrinker->lock);
> >> +
> >> + /* prevent racing with job submission code paths */
> >> + if (!dma_resv_trylock(obj->resv))
> >> + goto shrinker_lock;
> >
> > jfwiw, the trylock here is in the msm code isn't so much for madvise
> > (it is an error to submit jobs that reference DONTNEED objects), but
> > instead for the case of evicting WILLNEED but inactive objects to
> > swap. Ie. in the case that we need to move bo's back in to memory, we
> > don't want to unpin/evict a buffer that is later on the list for the
> > same job.. msm shrinker re-uses the same scan loop for both
> > inactive_dontneed (purge) and inactive_willneed (evict)
>
> I don't see connection between the objects on the shrinker's list and
> the job's BOs. Jobs indeed must not have any objects marked as DONTNEED,
> this case should never happen in practice, but we still need to protect
> from it.
Hmm, let me try to explain with a simple example.. hopefully this makes sense.
Say you have a job with two bo's, A and B.. bo A is not backed with
memory (either hasn't been used before or was evicted. Allocating
pages for A triggers shrinker. But B is still on the
inactive_willneed list, however it is already locked (because we don't
want to evict B to obtain backing pages for A).
>
> > I suppose using trylock is not technically wrong, and it would be a
> > good idea if the shmem helpers supported eviction as well. But I
> > think in the madvise/purge case if you lose the trylock then there is
> > something else bad going on.
>
> This trylock is intended for protecting job's submission path from
> racing with madvise ioctl invocation followed by immediate purging of
> BOs while job is in a process of submission, i.e. it protects from a
> use-after-free.
ahh, ok
> If you'll lose this trylock, then shrinker can't use
> dma_resv_test_signaled() reliably anymore and shrinker may purge BO
> before job had a chance to add fence to the BO's reservation.
>
> > Anyways, from the PoV of minimizing lock contention when under memory
> > pressure, this all looks good to me.
>
> Thank you. I may try to add generic eviction support to the v3.
eviction is a trickier thing to get right, I wouldn't blame you for
splitting that out into it's own patchset ;-)
You probably also would want to make it a thing that is opt-in for
drivers using the shmem helpers
BR,
-R
