| Message ID | 20220417223707.157113-12-dmitry.osipenko@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add generic memory shrinker to VirtIO-GPU and Panfrost DRM drivers | expand |
Hi Am 18.04.22 um 00:37 schrieb Dmitry Osipenko: > Introduce a common DRM SHMEM shrinker. It allows to reduce code > duplication among DRM drivers that implement theirs own shrinkers. > This is initial version of the shrinker that covers basic needs of > GPU drivers, both purging and eviction of shmem objects are supported. > > This patch is based on a couple ideas borrowed from Rob's Clark MSM > shrinker and Thomas' Zimmermann variant of SHMEM shrinker. > > In order to start using DRM SHMEM shrinker drivers should: > > 1. Implement new purge(), evict() + swap_in() GEM callbacks. > 2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device). > 3. Use drm_gem_shmem_set_purgeable_and_evictable(shmem) and alike API > functions to activate shrinking of GEMs. > > Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 765 ++++++++++++++++++++++++- > include/drm/drm_device.h | 4 + > include/drm/drm_gem.h | 35 ++ > include/drm/drm_gem_shmem_helper.h | 105 +++- > 4 files changed, 877 insertions(+), 32 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > index 3ecef571eff3..3838fb8d6f3a 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -88,6 +88,13 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private) > > INIT_LIST_HEAD(&shmem->madv_list); > > + /* > + * Eviction and purging are disabled by default, shmem user must enable > + * them explicitly using drm_gem_shmem_set_evictable/purgeable(). > + */ > + shmem->eviction_disable_count = 1; > + shmem->purging_disable_count = 1; > + > if (!private) { > /* > * Our buffers are kept pinned, so allocating them > @@ -126,6 +133,107 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t > } > EXPORT_SYMBOL_GPL(drm_gem_shmem_create); > > +static void > +drm_gem_shmem_add_pages_to_shrinker(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 (!shmem->pages_shrinkable) { > + WARN_ON(gem_shrinker->shrinkable_count + page_count < page_count); > + gem_shrinker->shrinkable_count += page_count; > + shmem->pages_shrinkable = true; > + } > +} > + > +static void > +drm_gem_shmem_remove_pages_from_shrinker(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 (shmem->pages_shrinkable) { > + WARN_ON(gem_shrinker->shrinkable_count < page_count); > + gem_shrinker->shrinkable_count -= page_count; > + shmem->pages_shrinkable = false; > + } > +} > + > +static void > +drm_gem_shmem_set_pages_state_locked(struct drm_gem_shmem_object *shmem, > + enum drm_gem_shmem_pages_state new_state) > +{ > + struct drm_gem_object *obj = &shmem->base; > + struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker; > + > + lockdep_assert_held(&gem_shrinker->lock); > + lockdep_assert_held(&obj->resv->lock.base); > + > + if (new_state >= DRM_GEM_SHMEM_PAGES_STATE_PINNED) { > + if (drm_gem_shmem_is_evictable(shmem)) > + new_state = DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE; > + > + if (drm_gem_shmem_is_purgeable(shmem)) > + new_state = DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE; > + > + if (!shmem->pages) > + new_state = DRM_GEM_SHMEM_PAGES_STATE_UNPINNED; > + > + if (shmem->evicted) > + new_state = DRM_GEM_SHMEM_PAGES_STATE_EVICTED; > + } > + > + if (shmem->pages_state == new_state) > + return; > + > + switch (new_state) { > + case DRM_GEM_SHMEM_PAGES_STATE_UNPINNED: > + case DRM_GEM_SHMEM_PAGES_STATE_PURGED: > + drm_gem_shmem_remove_pages_from_shrinker(shmem); > + list_del_init(&shmem->madv_list); > + break; > + > + case DRM_GEM_SHMEM_PAGES_STATE_PINNED: > + drm_gem_shmem_remove_pages_from_shrinker(shmem); > + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_active); > + break; > + > + case DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE: > + drm_gem_shmem_add_pages_to_shrinker(shmem); > + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_purgeable); > + break; > + > + case DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE: > + drm_gem_shmem_add_pages_to_shrinker(shmem); > + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_evictable); > + break; > + > + case DRM_GEM_SHMEM_PAGES_STATE_EVICTED: > + drm_gem_shmem_remove_pages_from_shrinker(shmem); > + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_evicted); > + break; > + } > + > + shmem->pages_state = new_state; > +} > + > +static void > +drm_gem_shmem_set_pages_state(struct drm_gem_shmem_object *shmem, > + enum drm_gem_shmem_pages_state new_state) > +{ > + struct drm_gem_object *obj = &shmem->base; > + struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker; > + > + if (!gem_shrinker) > + return; > + > + mutex_lock(&gem_shrinker->lock); > + drm_gem_shmem_set_pages_state_locked(shmem, new_state); > + mutex_unlock(&gem_shrinker->lock); > +} > + > /** > * drm_gem_shmem_free - Free resources associated with a shmem GEM object > * @shmem: shmem GEM object to free > @@ -137,6 +245,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, -1); > + > WARN_ON(shmem->vmap_use_count); > > if (obj->import_attach) { > @@ -148,7 +259,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > sg_free_table(shmem->sgt); > kfree(shmem->sgt); > } > - if (shmem->pages) > + if (shmem->pages_use_count) > drm_gem_shmem_put_pages(shmem); > } > > @@ -159,18 +270,226 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > } > EXPORT_SYMBOL_GPL(drm_gem_shmem_free); > > -static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem) > +static void drm_gem_shmem_update_pages_state_locked(struct drm_gem_shmem_object *shmem) > +{ > + struct drm_gem_object *obj = &shmem->base; > + struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker; > + enum drm_gem_shmem_pages_state new_state; > + > + if (!gem_shrinker || obj->import_attach) > + return; > + > + mutex_lock(&gem_shrinker->lock); > + > + if (!shmem->madv) > + new_state = DRM_GEM_SHMEM_PAGES_STATE_PINNED; > + else if (shmem->madv > 0) > + new_state = DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE; > + else > + new_state = DRM_GEM_SHMEM_PAGES_STATE_PURGED; > + > + drm_gem_shmem_set_pages_state_locked(shmem, new_state); > + > + mutex_unlock(&gem_shrinker->lock); > +} > + > +static void drm_gem_shmem_update_pages_state(struct drm_gem_shmem_object *shmem) > +{ > + dma_resv_lock(shmem->base.resv, NULL); > + drm_gem_shmem_update_pages_state_locked(shmem); > + dma_resv_unlock(shmem->base.resv); > +} > + > +static int > +drm_gem_shmem_set_evictable_locked(struct drm_gem_shmem_object *shmem) > +{ > + int ret = 0; > + > + WARN_ON_ONCE(!shmem->eviction_disable_count--); > + > + if (shmem->madv < 0) > + ret = -ENOMEM; > + > + drm_gem_shmem_update_pages_state_locked(shmem); > + > + return ret; > +} > + > +static int > +drm_gem_shmem_set_unevictable_locked(struct drm_gem_shmem_object *shmem) > +{ > + struct drm_gem_object *obj = &shmem->base; > + int err; > + > + if (shmem->madv < 0) > + return -ENOMEM; > + > + if (shmem->evicted) { > + err = obj->funcs->swap_in(obj); > + if (err) > + return err; > + } > + > + shmem->eviction_disable_count++; > + > + drm_gem_shmem_update_pages_state_locked(shmem); > + > + return 0; > +} > + > +static int > +drm_gem_shmem_set_purgeable_locked(struct drm_gem_shmem_object *shmem) > +{ > + int ret = 0; > + > + WARN_ON_ONCE(!shmem->purging_disable_count--); > + > + if (shmem->madv < 0) > + ret = -ENOMEM; > + > + drm_gem_shmem_update_pages_state_locked(shmem); > + > + return ret; > +} > + > +/** > + * drm_gem_shmem_set_purgeable() - Make GEM purgeable by memory shrinker > + * @shmem: shmem GEM object > + * > + * Tell memory shrinker that this GEM can be purged. Initially purging is > + * disabled for all GEMs. Each set_pureable() call must have corresponding > + * set_unpureable() call. If GEM was purged, then -ENOMEM is returned. > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_gem_shmem_set_purgeable(struct drm_gem_shmem_object *shmem) > +{ > + int ret; > + > + dma_resv_lock(shmem->base.resv, NULL); > + ret = drm_gem_shmem_set_purgeable_locked(shmem); > + dma_resv_unlock(shmem->base.resv); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(drm_gem_shmem_set_purgeable); > + > +static int > +drm_gem_shmem_set_unpurgeable_locked(struct drm_gem_shmem_object *shmem) > +{ > + if (shmem->madv < 0) > + return -ENOMEM; > + > + shmem->purging_disable_count++; > + > + drm_gem_shmem_update_pages_state_locked(shmem); > + > + return 0; > +} > + > +static int > +drm_gem_shmem_set_purgeable_and_evictable_locked(struct drm_gem_shmem_object *shmem) > +{ > + int ret; > + > + ret = drm_gem_shmem_set_evictable_locked(shmem); > + if (!ret) { > + ret = drm_gem_shmem_set_purgeable_locked(shmem); > + if (ret) > + drm_gem_shmem_set_unevictable_locked(shmem); > + } > + > + return ret; > +} > + > +static int > +drm_gem_shmem_set_unpurgeable_and_unevictable_locked(struct drm_gem_shmem_object *shmem) > +{ > + int ret; > + > + ret = drm_gem_shmem_set_unpurgeable_locked(shmem); > + if (!ret) { > + ret = drm_gem_shmem_set_unevictable_locked(shmem); > + if (ret) > + drm_gem_shmem_set_purgeable_locked(shmem); > + } > + > + return ret; > +} > + > +/** > + * drm_gem_shmem_set_purgeable_and_evictable() - Make GEM unpurgeable and > + * unevictable by memory shrinker > + * @shmem: shmem GEM object > + * > + * Tell memory shrinker that this GEM can't be purged and evicted. Each > + * set_purgeable_and_evictable() call must have corresponding > + * unpurgeable_and_unevictable() call. If GEM was purged, then -ENOMEM > + * is returned. > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_gem_shmem_set_purgeable_and_evictable(struct drm_gem_shmem_object *shmem) > +{ > + int ret; > + > + dma_resv_lock(shmem->base.resv, NULL); > + ret = drm_gem_shmem_set_purgeable_and_evictable_locked(shmem); > + dma_resv_unlock(shmem->base.resv); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(drm_gem_shmem_set_purgeable_and_evictable); > + > +/** > + * drm_gem_shmem_set_unpurgeable_and_unevictable() - Make GEM purgeable and > + * evictable by memory shrinker > + * @shmem: shmem GEM object > + * > + * Tell memory shrinker that this GEM can be purged and evicted. Each > + * unpurgeable_and_unevictable() call must have corresponding > + * set_purgeable_and_evictable() call. If GEM was purged, then -ENOMEM > + * is returned. > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_gem_shmem_set_unpurgeable_and_unevictable(struct drm_gem_shmem_object *shmem) > +{ > + int ret; > + > + ret = dma_resv_lock_interruptible(shmem->base.resv, NULL); > + if (ret) > + return ret; > + > + ret = drm_gem_shmem_set_unpurgeable_and_unevictable_locked(shmem); > + dma_resv_unlock(shmem->base.resv); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(drm_gem_shmem_set_unpurgeable_and_unevictable); > + > +static int > +drm_gem_shmem_acquire_pages_locked(struct drm_gem_shmem_object *shmem) > { > struct drm_gem_object *obj = &shmem->base; > struct page **pages; > > - if (shmem->pages_use_count++ > 0) > + if (shmem->madv < 0) { > + WARN_ON(shmem->pages); > + return -ENOMEM; > + } > + > + if (shmem->pages) { > + WARN_ON(!shmem->evicted); > return 0; > + } > > pages = drm_gem_get_pages(obj); > if (IS_ERR(pages)) { > DRM_DEBUG_KMS("Failed to get pages (%ld)\n", PTR_ERR(pages)); > - shmem->pages_use_count = 0; > return PTR_ERR(pages); > } > > @@ -189,6 +508,25 @@ static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem) > return 0; > } > > +static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem) > +{ > + int err; > + > + if (shmem->madv < 0) > + return -ENOMEM; > + > + if (shmem->pages_use_count++ > 0) > + return 0; > + > + err = drm_gem_shmem_acquire_pages_locked(shmem); > + if (err) { > + shmem->pages_use_count = 0; > + return err; > + } > + > + return 0; > +} > + > /* > * drm_gem_shmem_get_pages - Allocate backing pages for a shmem GEM object > * @shmem: shmem GEM object > @@ -209,21 +547,38 @@ int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem) > if (ret) > return ret; > ret = drm_gem_shmem_get_pages_locked(shmem); > + > + drm_gem_shmem_update_pages_state_locked(shmem); > + > dma_resv_unlock(shmem->base.resv); > > return ret; > } > EXPORT_SYMBOL(drm_gem_shmem_get_pages); > > -static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) > +static void drm_gem_shmem_get_pages_no_fail(struct drm_gem_shmem_object *shmem) > { > - struct drm_gem_object *obj = &shmem->base; > + WARN_ON(shmem->base.import_attach); > > - if (WARN_ON_ONCE(!shmem->pages_use_count)) > - return; > + dma_resv_lock(shmem->base.resv, NULL); > > - if (--shmem->pages_use_count > 0) > + if (drm_gem_shmem_get_pages_locked(shmem)) > + shmem->pages_use_count++; > + > + drm_gem_shmem_update_pages_state_locked(shmem); > + > + dma_resv_unlock(shmem->base.resv); > +} > + > +static void > +drm_gem_shmem_release_pages_locked(struct drm_gem_shmem_object *shmem) > +{ > + struct drm_gem_object *obj = &shmem->base; > + > + if (!shmem->pages) { > + WARN_ON(!shmem->evicted && shmem->madv >= 0); > return; > + } > > #ifdef CONFIG_X86 > if (shmem->map_wc) > @@ -236,6 +591,21 @@ static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) > shmem->pages = NULL; > } > > +static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) > +{ > + struct drm_gem_object *obj = &shmem->base; > + > + lockdep_assert_held(&obj->resv->lock.base); > + > + if (WARN_ON(!shmem->pages_use_count)) > + return; > + > + if (--shmem->pages_use_count > 0) > + return; > + > + drm_gem_shmem_release_pages_locked(shmem); > +} > + > /* > * drm_gem_shmem_put_pages - Decrease use count on the backing pages for a shmem GEM object > * @shmem: shmem GEM object > @@ -246,6 +616,7 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem) > { > dma_resv_lock(shmem->base.resv, NULL); > drm_gem_shmem_put_pages_locked(shmem); > + drm_gem_shmem_update_pages_state_locked(shmem); > dma_resv_unlock(shmem->base.resv); > } > EXPORT_SYMBOL(drm_gem_shmem_put_pages); > @@ -262,9 +633,21 @@ EXPORT_SYMBOL(drm_gem_shmem_put_pages); > */ > int drm_gem_shmem_pin(struct drm_gem_shmem_object *shmem) > { > + int err; > + > WARN_ON(shmem->base.import_attach); > > - return drm_gem_shmem_get_pages(shmem); > + err = drm_gem_shmem_set_unpurgeable_and_unevictable(shmem); > + if (err) > + return err; > + > + err = drm_gem_shmem_get_pages(shmem); > + if (err) { > + drm_gem_shmem_set_purgeable_and_evictable(shmem); > + return err; > + } > + > + return 0; > } > EXPORT_SYMBOL(drm_gem_shmem_pin); > > @@ -280,6 +663,7 @@ void drm_gem_shmem_unpin(struct drm_gem_shmem_object *shmem) > WARN_ON(shmem->base.import_attach); > > drm_gem_shmem_put_pages(shmem); > + drm_gem_shmem_set_purgeable_and_evictable(shmem); > } > EXPORT_SYMBOL(drm_gem_shmem_unpin); > > @@ -359,7 +743,18 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem, > ret = dma_resv_lock_interruptible(shmem->base.resv, NULL); > if (ret) > return ret; > + > + ret = drm_gem_shmem_set_unpurgeable_and_unevictable_locked(shmem); > + if (ret) > + goto unlock; > + > ret = drm_gem_shmem_vmap_locked(shmem, map); > + if (ret) > + drm_gem_shmem_set_purgeable_and_evictable_locked(shmem); > + else > + drm_gem_shmem_update_pages_state_locked(shmem); > + > +unlock: > dma_resv_unlock(shmem->base.resv); > > return ret; > @@ -404,9 +799,9 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem, > { > dma_resv_lock(shmem->base.resv, NULL); > drm_gem_shmem_vunmap_locked(shmem, map); > + drm_gem_shmem_update_pages_state_locked(shmem); > + drm_gem_shmem_set_purgeable_and_evictable_locked(shmem); > dma_resv_unlock(shmem->base.resv); > - > - drm_gem_shmem_update_purgeable_status(shmem); > } > EXPORT_SYMBOL(drm_gem_shmem_vunmap); > > @@ -447,29 +842,140 @@ int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv) > > madv = shmem->madv; > > + drm_gem_shmem_update_pages_state_locked(shmem); > + > dma_resv_unlock(shmem->base.resv); > > return (madv >= 0); > } > EXPORT_SYMBOL(drm_gem_shmem_madvise); > > -void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem) > +/** > + * drm_gem_shmem_swap_in_pages_locked() - Moves shmem pages back to memory > + * @shmem: shmem GEM object > + * > + * This function moves pages back to memory if they were previously evicted > + * by the memory shrinker. > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_gem_shmem_swap_in_pages_locked(struct drm_gem_shmem_object *shmem) > +{ > + struct drm_gem_object *obj = &shmem->base; > + struct sg_table *sgt; > + int ret; > + > + lockdep_assert_held(&obj->resv->lock.base); > + > + if (shmem->evicted) { > + ret = drm_gem_shmem_acquire_pages_locked(shmem); > + if (ret) > + return ret; > + > + sgt = drm_gem_shmem_get_sg_table(shmem); > + if (IS_ERR(sgt)) > + return PTR_ERR(sgt); > + > + ret = dma_map_sgtable(obj->dev->dev, sgt, > + DMA_BIDIRECTIONAL, 0); > + if (ret) { > + sg_free_table(sgt); > + kfree(sgt); > + return ret; > + } > + > + shmem->sgt = sgt; > + shmem->evicted = false; > + shmem->pages_state = DRM_GEM_SHMEM_PAGES_STATE_PINNED; > + > + drm_gem_shmem_update_pages_state_locked(shmem); > + } > + > + return shmem->pages ? 0 : -ENOMEM; > +} > +EXPORT_SYMBOL_GPL(drm_gem_shmem_swap_in_pages_locked); > + > +/** > + * drm_gem_shmem_swap_in_locked() - Moves shmem GEM back to memory > + * @shmem: shmem GEM object > + * > + * This function moves shmem GEM back to memory if it was previously evicted > + * by the memory shrinker. The GEM is ready to use on success. > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_gem_shmem_swap_in_locked(struct drm_gem_shmem_object *shmem) > +{ > + struct drm_gem_object *obj = &shmem->base; > + > + lockdep_assert_held(&obj->resv->lock.base); > + > + if (shmem->evicted) > + return obj->funcs->swap_in(obj); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(drm_gem_shmem_swap_in_locked); > + > +static void drm_gem_shmem_unpin_pages_locked(struct drm_gem_shmem_object *shmem) > { > struct drm_gem_object *obj = &shmem->base; > struct drm_device *dev = obj->dev; > > - WARN_ON(!drm_gem_shmem_is_purgeable(shmem)); > + if (shmem->evicted) > + return; > > dma_unmap_sgtable(dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0); > + drm_gem_shmem_release_pages_locked(shmem); > + drm_vma_node_unmap(&obj->vma_node, dev->anon_inode->i_mapping); > + > sg_free_table(shmem->sgt); > kfree(shmem->sgt); > shmem->sgt = NULL; > +} > > - drm_gem_shmem_put_pages_locked(shmem); > +/** > + * drm_gem_shmem_evict_locked - Evict shmem pages > + * @shmem: shmem GEM object > + * > + * This function unpins shmem pages, allowing them to be swapped out from > + * memory. > + */ > +void drm_gem_shmem_evict_locked(struct drm_gem_shmem_object *shmem) > +{ > + struct drm_gem_object *obj = &shmem->base; > > - shmem->madv = -1; > + lockdep_assert_held(&obj->resv->lock.base); > > - drm_vma_node_unmap(&obj->vma_node, dev->anon_inode->i_mapping); > + WARN_ON(!drm_gem_shmem_is_evictable(shmem)); > + WARN_ON(shmem->madv < 0); > + WARN_ON(shmem->evicted); > + > + drm_gem_shmem_unpin_pages_locked(shmem); > + > + shmem->evicted = true; > + drm_gem_shmem_set_pages_state(shmem, DRM_GEM_SHMEM_PAGES_STATE_EVICTED); > +} > +EXPORT_SYMBOL_GPL(drm_gem_shmem_evict_locked); > + > +/** > + * drm_gem_shmem_purge_locked - Purge shmem pages > + * @shmem: shmem GEM object > + * > + * This function permanently releases shmem pages. > + */ > +void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem) > +{ > + struct drm_gem_object *obj = &shmem->base; > + > + lockdep_assert_held(&obj->resv->lock.base); > + > + WARN_ON(!drm_gem_shmem_is_purgeable(shmem)); > + WARN_ON(shmem->madv < 0); > + > + drm_gem_shmem_unpin_pages_locked(shmem); > drm_gem_free_mmap_offset(obj); > > /* Our goal here is to return as much of the memory as > @@ -480,6 +986,9 @@ void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem) > shmem_truncate_range(file_inode(obj->filp), 0, (loff_t)-1); > > invalidate_mapping_pages(file_inode(obj->filp)->i_mapping, 0, (loff_t)-1); > + > + shmem->madv = -1; > + drm_gem_shmem_set_pages_state(shmem, DRM_GEM_SHMEM_PAGES_STATE_PURGED); > } > EXPORT_SYMBOL(drm_gem_shmem_purge_locked); > > @@ -543,22 +1052,31 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf) > vm_fault_t ret; > struct page *page; > pgoff_t page_offset; > + bool pages_inactive; > + int err; > > /* We don't use vmf->pgoff since that has the fake offset */ > page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT; > > dma_resv_lock(shmem->base.resv, NULL); > > - if (page_offset >= num_pages || > - WARN_ON_ONCE(!shmem->pages) || > - shmem->madv < 0) { > + pages_inactive = shmem->pages_state < DRM_GEM_SHMEM_PAGES_STATE_PINNED; > + WARN_ON_ONCE(!shmem->pages ^ pages_inactive); > + > + if (page_offset >= num_pages || (!shmem->pages && !shmem->evicted)) { > ret = VM_FAULT_SIGBUS; > } else { > + err = drm_gem_shmem_swap_in_locked(shmem); > + if (err) { > + ret = VM_FAULT_OOM; > + goto unlock; > + } > + > page = shmem->pages[page_offset]; > > ret = vmf_insert_pfn(vma, vmf->address, page_to_pfn(page)); > } > - > +unlock: > dma_resv_unlock(shmem->base.resv); > > return ret; > @@ -568,13 +1086,8 @@ static void drm_gem_shmem_vm_open(struct vm_area_struct *vma) > { > struct drm_gem_object *obj = vma->vm_private_data; > struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > - int ret; > - > - WARN_ON(shmem->base.import_attach); > - > - ret = drm_gem_shmem_get_pages(shmem); > - WARN_ON_ONCE(ret != 0); > > + drm_gem_shmem_get_pages_no_fail(shmem); > drm_gem_vm_open(vma); > } > > @@ -716,6 +1229,8 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem) > > shmem->sgt = sgt; > > + drm_gem_shmem_update_pages_state(shmem); > + > return sgt; > > err_free_sgt: > @@ -762,6 +1277,202 @@ 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 = READ_ONCE(gem_shrinker->shrinkable_count); > + > + if (count >= SHRINK_EMPTY) > + return SHRINK_EMPTY - 1; > + > + return count ?: SHRINK_EMPTY; > +} > + > +static unsigned long > +drm_gem_shmem_shrinker_run_objects_scan(struct shrinker *shrinker, > + unsigned long nr_to_scan, > + bool *lock_contention, > + bool evict) > +{ > + struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker); > + struct drm_gem_shmem_object *shmem; > + struct list_head still_in_list; > + struct drm_gem_object *obj; > + unsigned long freed = 0; > + struct list_head *lru; > + size_t page_count; > + > + INIT_LIST_HEAD(&still_in_list); > + > + mutex_lock(&gem_shrinker->lock); > + > + if (evict) > + lru = &gem_shrinker->lru_evictable; > + else > + lru = &gem_shrinker->lru_purgeable; > + > + while (freed < nr_to_scan) { > + shmem = list_first_entry_or_null(lru, typeof(*shmem), madv_list); > + if (!shmem) > + break; > + > + obj = &shmem->base; > + page_count = obj->size >> PAGE_SHIFT; > + list_move_tail(&shmem->madv_list, &still_in_list); > + > + if (evict && get_nr_swap_pages() < page_count) > + continue; > + > + /* > + * 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)) { > + *lock_contention |= true; > + goto shrinker_lock; > + } > + > + /* prevent racing with the dma-buf exporting */ > + if (!mutex_trylock(&gem_shrinker->dev->object_name_lock)) { > + *lock_contention |= true; > + goto resv_unlock; > + } > + > + /* check whether h/w uses this object */ > + if (!dma_resv_test_signaled(obj->resv, DMA_RESV_USAGE_WRITE)) > + goto object_name_unlock; > + > + /* GEM may've become unpurgeable while shrinker was unlocked */ > + if (evict) { > + if (!drm_gem_shmem_is_evictable(shmem)) > + goto object_name_unlock; > + } else { > + if (!drm_gem_shmem_is_purgeable(shmem)) > + goto object_name_unlock; > + } > + > + if (evict) > + freed += obj->funcs->evict(obj); > + else > + freed += obj->funcs->purge(obj); > +object_name_unlock: > + mutex_unlock(&gem_shrinker->dev->object_name_lock); > +resv_unlock: > + dma_resv_unlock(obj->resv); > +shrinker_lock: > + drm_gem_object_put(&shmem->base); > + mutex_lock(&gem_shrinker->lock); > + } > + > + list_splice_tail(&still_in_list, lru); > + > + mutex_unlock(&gem_shrinker->lock); > + > + return freed; > +} > + > +static unsigned long > +drm_gem_shmem_shrinker_scan_objects(struct shrinker *shrinker, > + struct shrink_control *sc) > +{ > + unsigned long nr_to_scan = sc->nr_to_scan; > + bool lock_contention = false; > + unsigned long freed; > + > + /* purge as many objects as we can */ > + freed = drm_gem_shmem_shrinker_run_objects_scan(shrinker, nr_to_scan, > + &lock_contention, false); > + nr_to_scan -= freed; > + > + /* evict as many objects as we can */ > + if (freed < nr_to_scan) > + freed += drm_gem_shmem_shrinker_run_objects_scan(shrinker, > + nr_to_scan, > + &lock_contention, > + true); > + > + return (!freed && !lock_contention) ? SHRINK_STOP : freed; > +} > + > +/** > + * drm_gem_shmem_shrinker_register() - Register shmem shrinker > + * @dev: DRM device > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_gem_shmem_shrinker_register(struct drm_device *dev) > +{ > + struct drm_gem_shmem_shrinker *gem_shrinker; > + int err; > + > + if (WARN_ON(dev->shmem_shrinker)) > + return -EBUSY; > + > + gem_shrinker = kzalloc(sizeof(*gem_shrinker), GFP_KERNEL); > + if (!gem_shrinker) > + return -ENOMEM; > + > + gem_shrinker->base.count_objects = drm_gem_shmem_shrinker_count_objects; > + gem_shrinker->base.scan_objects = drm_gem_shmem_shrinker_scan_objects; > + gem_shrinker->base.seeks = DEFAULT_SEEKS; > + gem_shrinker->dev = dev; > + > + INIT_LIST_HEAD(&gem_shrinker->lru_purgeable); > + INIT_LIST_HEAD(&gem_shrinker->lru_evictable); > + INIT_LIST_HEAD(&gem_shrinker->lru_evicted); > + INIT_LIST_HEAD(&gem_shrinker->lru_active); > + mutex_init(&gem_shrinker->lock); > + > + dev->shmem_shrinker = gem_shrinker; > + > + err = register_shrinker(&gem_shrinker->base); > + if (err) { > + dev->shmem_shrinker = NULL; > + kfree(gem_shrinker); > + return err; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(drm_gem_shmem_shrinker_register); > + > +/** > + * drm_gem_shmem_shrinker_unregister() - Unregister shmem shrinker > + * @dev: DRM device > + */ > +void drm_gem_shmem_shrinker_unregister(struct drm_device *dev) > +{ > + struct drm_gem_shmem_shrinker *gem_shrinker = dev->shmem_shrinker; > + > + if (gem_shrinker) { > + unregister_shrinker(&gem_shrinker->base); > + WARN_ON(!list_empty(&gem_shrinker->lru_purgeable)); > + WARN_ON(!list_empty(&gem_shrinker->lru_evictable)); > + WARN_ON(!list_empty(&gem_shrinker->lru_evicted)); > + WARN_ON(!list_empty(&gem_shrinker->lru_active)); > + mutex_destroy(&gem_shrinker->lock); > + dev->shmem_shrinker = NULL; > + kfree(gem_shrinker); > + } > +} > +EXPORT_SYMBOL_GPL(drm_gem_shmem_shrinker_unregister); > + > MODULE_DESCRIPTION("DRM SHMEM memory-management helpers"); > MODULE_IMPORT_NS(DMA_BUF); > MODULE_LICENSE("GPL v2"); > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h > index 9923c7a6885e..929546cad894 100644 > --- a/include/drm/drm_device.h > +++ b/include/drm/drm_device.h > @@ -16,6 +16,7 @@ struct drm_vblank_crtc; > struct drm_vma_offset_manager; > struct drm_vram_mm; > struct drm_fb_helper; > +struct drm_gem_shmem_shrinker; > > struct inode; > > @@ -277,6 +278,9 @@ struct drm_device { > /** @vram_mm: VRAM MM memory manager */ > struct drm_vram_mm *vram_mm; > > + /** @shmem_shrinker: SHMEM GEM memory shrinker */ > + struct drm_gem_shmem_shrinker *shmem_shrinker; > + > /** > * @switch_power_state: > * > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > index 9d7c61a122dc..390d1ce08ed3 100644 > --- a/include/drm/drm_gem.h > +++ b/include/drm/drm_gem.h > @@ -172,6 +172,41 @@ struct drm_gem_object_funcs { > * This is optional but necessary for mmap support. > */ > const struct vm_operations_struct *vm_ops; > + > + /** > + * @purge: > + * > + * Releases the GEM object's allocated backing storage to the system. > + * > + * Returns the number of pages that have been freed by purging the GEM object. > + * > + * This callback is used by the GEM shrinker. > + */ > + unsigned long (*purge)(struct drm_gem_object *obj); > + > + /** > + * @evict: > + * > + * Unpins the GEM object's allocated backing storage, allowing shmem pages > + * to be swapped out. What's the difference to the existing unpin() callback? > + * > + * Returns the number of pages that have been unpinned. > + * > + * This callback is used by the GEM shrinker. > + */ > + unsigned long (*evict)(struct drm_gem_object *obj); > + > + /** > + * @swap_in: > + * > + * Pins GEM object's allocated backing storage if it was previously evicted, > + * moving swapped out pages back to memory. > + * > + * Returns 0 on success, or -errno on error. > + * > + * This callback is used by the GEM shrinker. > + */ > + int (*swap_in)(struct drm_gem_object *obj); Why do you need swap_in()? This can be done on-demand as part of a pin or vmap operation. > }; > > /** > diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h > index 70889533962a..a65557b446e6 100644 > --- a/include/drm/drm_gem_shmem_helper.h > +++ b/include/drm/drm_gem_shmem_helper.h > @@ -6,6 +6,7 @@ > #include <linux/fs.h> > #include <linux/mm.h> > #include <linux/mutex.h> > +#include <linux/shrinker.h> > > #include <drm/drm_file.h> > #include <drm/drm_gem.h> > @@ -15,8 +16,18 @@ > struct dma_buf_attachment; > struct drm_mode_create_dumb; > struct drm_printer; > +struct drm_device; > struct sg_table; > > +enum drm_gem_shmem_pages_state { > + DRM_GEM_SHMEM_PAGES_STATE_PURGED = -2, > + DRM_GEM_SHMEM_PAGES_STATE_EVICTED = -1, > + DRM_GEM_SHMEM_PAGES_STATE_UNPINNED = 0, > + DRM_GEM_SHMEM_PAGES_STATE_PINNED = 1, > + DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE = 2, > + DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE = 3, > +}; These states can be detected by looking at the vmap and pin refcounts. No need to store them explicitly. In your patch, they also come with a big zoo of trivial helpers. None of that seems necessary AFAICT. What's the difference between purge and evict BTW? > + > /** > * struct drm_gem_shmem_object - GEM object backed by shmem > */ > @@ -43,8 +54,8 @@ struct drm_gem_shmem_object { > * @madv: State for madvise > * > * 0 is active/inuse. > + * 1 is not-needed/can-be-purged > * A negative value is the object is purged. > - * Positive values are driver specific and not used by the helpers. > */ > int madv; > > @@ -91,6 +102,40 @@ struct drm_gem_shmem_object { > * @map_wc: map object write-combined (instead of using shmem defaults). > */ > bool map_wc; > + > + /** > + * @eviction_disable_count: > + * > + * The shmem pages are disallowed to be evicted by the memory shrinker > + * while count is non-zero. Used internally by memory shrinker. > + */ > + unsigned int eviction_disable_count; > + > + /** > + * @purging_disable_count: > + * > + * The shmem pages are disallowed to be purged by the memory shrinker > + * while count is non-zero. Used internally by memory shrinker. > + */ > + unsigned int purging_disable_count; > + > + /** > + * @pages_state: Current state of shmem pages. Used internally by > + * memory shrinker. > + */ > + enum drm_gem_shmem_pages_state pages_state; > + > + /** > + * @evicted: True if shmem pages were evicted by the memory shrinker. > + * Used internally by memory shrinker. > + */ > + bool evicted; > + > + /** > + * @pages_shrinkable: True if shmem pages can be evicted or purged > + * by the memory shrinker. Used internally by memory shrinker. > + */ > + bool pages_shrinkable; As commented before, this state can be foundby looking at existing fields. No need to store it separately. Best regards Thomas > }; > > #define to_drm_gem_shmem_obj(obj) \ > @@ -111,15 +156,33 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct > > int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv); > > +int drm_gem_shmem_set_purgeable(struct drm_gem_shmem_object *shmem); > +int drm_gem_shmem_set_purgeable_and_evictable(struct drm_gem_shmem_object *shmem); > +int drm_gem_shmem_set_unpurgeable_and_unevictable(struct drm_gem_shmem_object *shmem); > + > +static inline bool drm_gem_shmem_is_evictable(struct drm_gem_shmem_object *shmem) > +{ > + return (shmem->madv >= 0) && !shmem->eviction_disable_count && > + shmem->base.funcs->evict && shmem->base.funcs->swap_in && > + !shmem->vmap_use_count && !shmem->base.dma_buf && > + !shmem->base.import_attach && shmem->sgt; > +} > + > static inline bool drm_gem_shmem_is_purgeable(struct drm_gem_shmem_object *shmem) > { > - return (shmem->madv > 0) && > - !shmem->vmap_use_count && shmem->sgt && > - !shmem->base.dma_buf && !shmem->base.import_attach; > + return (shmem->madv > 0) && !shmem->purging_disable_count && > + !shmem->vmap_use_count && shmem->base.funcs->purge && > + !shmem->base.dma_buf && !shmem->base.import_attach && > + shmem->sgt; > } > > -void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem); > +int drm_gem_shmem_swap_in_pages_locked(struct drm_gem_shmem_object *shmem); > +int drm_gem_shmem_swap_in_locked(struct drm_gem_shmem_object *shmem); > + > +void drm_gem_shmem_evict_locked(struct drm_gem_shmem_object *shmem); > + > bool drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem); > +void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem); > > struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_shmem_object *shmem); > struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem); > @@ -262,6 +325,38 @@ static inline int drm_gem_shmem_object_mmap(struct drm_gem_object *obj, struct v > return drm_gem_shmem_mmap(shmem, vma); > } > > +/** > + * struct drm_gem_shmem_shrinker - Generic memory shrinker for shmem GEMs > + */ > +struct drm_gem_shmem_shrinker { > + /** @base: Shrinker for purging shmem GEM objects */ > + struct shrinker base; > + > + /** @lock: Protects @lru_* */ > + struct mutex lock; > + > + /** @lru_purgeable: List of shmem GEM objects available for purging */ > + struct list_head lru_purgeable; > + > + /** @lru_active: List of active shmem GEM objects */ > + struct list_head lru_active; > + > + /** @lru_evictable: List of shmem GEM objects that can be evicted */ > + struct list_head lru_evictable; > + > + /** @lru_evicted: List of evicted shmem GEM objects */ > + struct list_head lru_evicted; > + > + /** @dev: DRM device that uses this shrinker */ > + struct drm_device *dev; > + > + /** @shrinkable_count: Count of shmem GEM pages to be purged and evicted */ > + u64 shrinkable_count; > +}; > + > +int drm_gem_shmem_shrinker_register(struct drm_device *dev); > +void drm_gem_shmem_shrinker_unregister(struct drm_device *dev); > + > /* > * Driver ops > */
On 4/19/22 10:22, Thomas Zimmermann wrote: > Hi > > Am 18.04.22 um 00:37 schrieb Dmitry Osipenko: >> Introduce a common DRM SHMEM shrinker. It allows to reduce code >> duplication among DRM drivers that implement theirs own shrinkers. >> This is initial version of the shrinker that covers basic needs of >> GPU drivers, both purging and eviction of shmem objects are supported. >> >> This patch is based on a couple ideas borrowed from Rob's Clark MSM >> shrinker and Thomas' Zimmermann variant of SHMEM shrinker. >> >> In order to start using DRM SHMEM shrinker drivers should: >> >> 1. Implement new purge(), evict() + swap_in() GEM callbacks. >> 2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device). >> 3. Use drm_gem_shmem_set_purgeable_and_evictable(shmem) and alike API >>    functions to activate shrinking of GEMs. >> >> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >> --- >>  drivers/gpu/drm/drm_gem_shmem_helper.c | 765 ++++++++++++++++++++++++- >>  include/drm/drm_device.h              |  4 + >>  include/drm/drm_gem.h                 | 35 ++ >>  include/drm/drm_gem_shmem_helper.h    | 105 +++- >>  4 files changed, 877 insertions(+), 32 deletions(-) ... >> @@ -172,6 +172,41 @@ struct drm_gem_object_funcs { >>       * This is optional but necessary for mmap support. >>       */ >>      const struct vm_operations_struct *vm_ops; >> + >> +   /** >> +    * @purge: >> +    * >> +    * Releases the GEM object's allocated backing storage to the >> system. >> +    * >> +    * Returns the number of pages that have been freed by purging >> the GEM object. >> +    * >> +    * This callback is used by the GEM shrinker. >> +    */ >> +   unsigned long (*purge)(struct drm_gem_object *obj); >> + >> +   /** >> +    * @evict: >> +    * >> +    * Unpins the GEM object's allocated backing storage, allowing >> shmem pages >> +    * to be swapped out. > > What's the difference to the existing unpin() callback? Drivers need to do more than just unpinning pages when GEMs are evicted. Unpinning is only a part of the eviction process. I'll improve the doc-comment in v5. For example, for VirtIO-GPU driver we need to to detach host from the guest's memory before pages are evicted [1]. [1] https://gitlab.collabora.com/dmitry.osipenko/linux-kernel-rd/-/blob/932eb03198bce3a21353b09ab71e95f1c19b84c2/drivers/gpu/drm/virtio/virtgpu_object.c#L145 In case of Panfrost driver, we will need to remove mappings before pages are evicted. >> +    * >> +    * Returns the number of pages that have been unpinned. >> +    * >> +    * This callback is used by the GEM shrinker. >> +    */ >> +   unsigned long (*evict)(struct drm_gem_object *obj); >> + >> +   /** >> +    * @swap_in: >> +    * >> +    * Pins GEM object's allocated backing storage if it was >> previously evicted, >> +    * moving swapped out pages back to memory. >> +    * >> +    * Returns 0 on success, or -errno on error. >> +    * >> +    * This callback is used by the GEM shrinker. >> +    */ >> +   int (*swap_in)(struct drm_gem_object *obj); > > Why do you need swap_in()? This can be done on-demand as part of a pin > or vmap operation. Similarly to the unpinning, the pining of pages is only a part of what needs to be done for GPU drivers. Besides of returning pages back to memory, we also need to make them accessible to GPU and this is a driver-specific process. This why we need the additional callbacks. >>  }; >>   /** >> diff --git a/include/drm/drm_gem_shmem_helper.h >> b/include/drm/drm_gem_shmem_helper.h >> index 70889533962a..a65557b446e6 100644 >> --- a/include/drm/drm_gem_shmem_helper.h >> +++ b/include/drm/drm_gem_shmem_helper.h >> @@ -6,6 +6,7 @@ >>  #include <linux/fs.h> >>  #include <linux/mm.h> >>  #include <linux/mutex.h> >> +#include <linux/shrinker.h> >>   #include <drm/drm_file.h> >>  #include <drm/drm_gem.h> >> @@ -15,8 +16,18 @@ >>  struct dma_buf_attachment; >>  struct drm_mode_create_dumb; >>  struct drm_printer; >> +struct drm_device; >>  struct sg_table; >>  +enum drm_gem_shmem_pages_state { >> +   DRM_GEM_SHMEM_PAGES_STATE_PURGED = -2, >> +   DRM_GEM_SHMEM_PAGES_STATE_EVICTED = -1, >> +   DRM_GEM_SHMEM_PAGES_STATE_UNPINNED = 0, >> +   DRM_GEM_SHMEM_PAGES_STATE_PINNED = 1, >> +   DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE = 2, >> +   DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE = 3, >> +}; > > These states can be detected by looking at the vmap and pin refcounts. > No need to store them explicitly. I'll try to revisit this, but I was finding that it's much more difficult to follow and debug code without the explicit states. > In your patch, they also come with a > big zoo of trivial helpers. None of that seems necessary AFAICT. There are couple functions which could be squashed, although this may hurt readability of the code a tad. I'll try to take another look at this for v5. > What's the difference between purge and evict BTW? The evicted pages are moved out from memory to a SWAP partition or file. The purged pages are destroyed permanently. >> + >>  /** >>   * struct drm_gem_shmem_object - GEM object backed by shmem >>   */ >> @@ -43,8 +54,8 @@ struct drm_gem_shmem_object { >>       * @madv: State for madvise >>       * >>       * 0 is active/inuse. >> +    * 1 is not-needed/can-be-purged >>       * A negative value is the object is purged. >> -    * Positive values are driver specific and not used by the helpers. >>       */ >>      int madv; >>  @@ -91,6 +102,40 @@ struct drm_gem_shmem_object { >>       * @map_wc: map object write-combined (instead of using shmem >> defaults). >>       */ >>      bool map_wc; >> + >> +   /** >> +    * @eviction_disable_count: >> +    * >> +    * The shmem pages are disallowed to be evicted by the memory >> shrinker >> +    * while count is non-zero. Used internally by memory shrinker. >> +    */ >> +   unsigned int eviction_disable_count; >> + >> +   /** >> +    * @purging_disable_count: >> +    * >> +    * The shmem pages are disallowed to be purged by the memory >> shrinker >> +    * while count is non-zero. Used internally by memory shrinker. >> +    */ >> +   unsigned int purging_disable_count; >> + >> +   /** >> +    * @pages_state: Current state of shmem pages. Used internally by >> +    * memory shrinker. >> +    */ >> +   enum drm_gem_shmem_pages_state pages_state; >> + >> +   /** >> +    * @evicted: True if shmem pages were evicted by the memory >> shrinker. >> +    * Used internally by memory shrinker. >> +    */ >> +   bool evicted; >> + >> +   /** >> +    * @pages_shrinkable: True if shmem pages can be evicted or purged >> +    * by the memory shrinker. Used internally by memory shrinker. >> +    */ >> +   bool pages_shrinkable; > > As commented before, this state can be foundby looking at existing > fields. No need to store it separately. When we're transitioning from "evictable" to a "purgeable" state, we must not add pages twice to the "shrinkable_count" variable. Hence this is not a state, but a variable which prevents the double accounting of the pages. Please see drm_gem_shmem_add_pages_to_shrinker() in this patch. Perhaps something like "pages_accounted_by_shrinker" could be a better name for the variable. I'll revisit this for v5.
On Tue, Apr 19, 2022 at 11:40:41PM +0300, Dmitry Osipenko wrote: > On 4/19/22 10:22, Thomas Zimmermann wrote: > > Hi > > > > Am 18.04.22 um 00:37 schrieb Dmitry Osipenko: > >> Introduce a common DRM SHMEM shrinker. It allows to reduce code > >> duplication among DRM drivers that implement theirs own shrinkers. > >> This is initial version of the shrinker that covers basic needs of > >> GPU drivers, both purging and eviction of shmem objects are supported. > >> > >> This patch is based on a couple ideas borrowed from Rob's Clark MSM > >> shrinker and Thomas' Zimmermann variant of SHMEM shrinker. > >> > >> In order to start using DRM SHMEM shrinker drivers should: > >> > >> 1. Implement new purge(), evict() + swap_in() GEM callbacks. > >> 2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device). > >> 3. Use drm_gem_shmem_set_purgeable_and_evictable(shmem) and alike API > >>    functions to activate shrinking of GEMs. > >> > >> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com> > >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >> --- > >>  drivers/gpu/drm/drm_gem_shmem_helper.c | 765 ++++++++++++++++++++++++- > >>  include/drm/drm_device.h              |  4 + > >>  include/drm/drm_gem.h                 | 35 ++ > >>  include/drm/drm_gem_shmem_helper.h    | 105 +++- > >>  4 files changed, 877 insertions(+), 32 deletions(-) > ... > >> @@ -172,6 +172,41 @@ struct drm_gem_object_funcs { > >>       * This is optional but necessary for mmap support. > >>       */ > >>      const struct vm_operations_struct *vm_ops; > >> + > >> +   /** > >> +    * @purge: > >> +    * > >> +    * Releases the GEM object's allocated backing storage to the > >> system. > >> +    * > >> +    * Returns the number of pages that have been freed by purging > >> the GEM object. > >> +    * > >> +    * This callback is used by the GEM shrinker. > >> +    */ > >> +   unsigned long (*purge)(struct drm_gem_object *obj); Hm I feel like drivers shouldn't need to know the difference here? Like shmem helpers can track what's purgeable, and for eviction/purging the driver callback should do the same? The only difference is when we try to re-reserve the backing storage. When the object has been evicted that should suceed, but when the object is purged that will fail. That's the difference between evict and purge for drivers? > >> + > >> +   /** > >> +    * @evict: > >> +    * > >> +    * Unpins the GEM object's allocated backing storage, allowing > >> shmem pages > >> +    * to be swapped out. > > > > What's the difference to the existing unpin() callback? > > Drivers need to do more than just unpinning pages when GEMs are evicted. > Unpinning is only a part of the eviction process. I'll improve the > doc-comment in v5. > > For example, for VirtIO-GPU driver we need to to detach host from the > guest's memory before pages are evicted [1]. > > [1] > https://gitlab.collabora.com/dmitry.osipenko/linux-kernel-rd/-/blob/932eb03198bce3a21353b09ab71e95f1c19b84c2/drivers/gpu/drm/virtio/virtgpu_object.c#L145 > > In case of Panfrost driver, we will need to remove mappings before pages > are evicted. It might be good to align this with ttm, otoh that all works quite a bit differently for ttm since ttm supports buffer moves and a lot more fancy stuff. I'm bringing this up since I have this fancy idea that eventually we could glue shmem helpers into ttm in some cases for managing buffers when they sit in system memory (as opposed to vram). > >> +    * > >> +    * Returns the number of pages that have been unpinned. > >> +    * > >> +    * This callback is used by the GEM shrinker. > >> +    */ > >> +   unsigned long (*evict)(struct drm_gem_object *obj); > >> + > >> +   /** > >> +    * @swap_in: > >> +    * > >> +    * Pins GEM object's allocated backing storage if it was > >> previously evicted, > >> +    * moving swapped out pages back to memory. > >> +    * > >> +    * Returns 0 on success, or -errno on error. > >> +    * > >> +    * This callback is used by the GEM shrinker. > >> +    */ > >> +   int (*swap_in)(struct drm_gem_object *obj); > > > > Why do you need swap_in()? This can be done on-demand as part of a pin > > or vmap operation. > > Similarly to the unpinning, the pining of pages is only a part of what > needs to be done for GPU drivers. Besides of returning pages back to > memory, we also need to make them accessible to GPU and this is a > driver-specific process. This why we need the additional callbacks. This is a bit much midlayer. The way this works in ttm is you reserve all the objects you need (which makes sure they're physically available again), and then the driver goes through and makes sure the page tables are all set up again. Once you get towards gpu vm that's really the only approach, since your swap_in has no idea for which vm it needs to restore pagetables (and restoring it for all is a bit meh). If drivers want to optimize this they can adjust/set any tracking information from their evict callback as needed. > > >>  }; > >>   /** > >> diff --git a/include/drm/drm_gem_shmem_helper.h > >> b/include/drm/drm_gem_shmem_helper.h > >> index 70889533962a..a65557b446e6 100644 > >> --- a/include/drm/drm_gem_shmem_helper.h > >> +++ b/include/drm/drm_gem_shmem_helper.h > >> @@ -6,6 +6,7 @@ > >>  #include <linux/fs.h> > >>  #include <linux/mm.h> > >>  #include <linux/mutex.h> > >> +#include <linux/shrinker.h> > >>   #include <drm/drm_file.h> > >>  #include <drm/drm_gem.h> > >> @@ -15,8 +16,18 @@ > >>  struct dma_buf_attachment; > >>  struct drm_mode_create_dumb; > >>  struct drm_printer; > >> +struct drm_device; > >>  struct sg_table; > >>  +enum drm_gem_shmem_pages_state { > >> +   DRM_GEM_SHMEM_PAGES_STATE_PURGED = -2, > >> +   DRM_GEM_SHMEM_PAGES_STATE_EVICTED = -1, > >> +   DRM_GEM_SHMEM_PAGES_STATE_UNPINNED = 0, > >> +   DRM_GEM_SHMEM_PAGES_STATE_PINNED = 1, > >> +   DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE = 2, > >> +   DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE = 3, > >> +}; > > > > These states can be detected by looking at the vmap and pin refcounts. > > No need to store them explicitly. > > I'll try to revisit this, but I was finding that it's much more > difficult to follow and debug code without the explicit states. purgeable/purged needs some state, but pinned shouldn't be duplicated, so I concur here a bit. > > In your patch, they also come with a > > big zoo of trivial helpers. None of that seems necessary AFAICT. > > There are couple functions which could be squashed, although this may > hurt readability of the code a tad. I'll try to take another look at > this for v5. > > > What's the difference between purge and evict BTW? > > The evicted pages are moved out from memory to a SWAP partition or file. > > The purged pages are destroyed permanently. > > >> + > >>  /** > >>   * struct drm_gem_shmem_object - GEM object backed by shmem > >>   */ > >> @@ -43,8 +54,8 @@ struct drm_gem_shmem_object { > >>       * @madv: State for madvise > >>       * > >>       * 0 is active/inuse. > >> +    * 1 is not-needed/can-be-purged > >>       * A negative value is the object is purged. > >> -    * Positive values are driver specific and not used by the helpers. > >>       */ > >>      int madv; > >>  @@ -91,6 +102,40 @@ struct drm_gem_shmem_object { > >>       * @map_wc: map object write-combined (instead of using shmem > >> defaults). > >>       */ > >>      bool map_wc; > >> + > >> +   /** > >> +    * @eviction_disable_count: > >> +    * > >> +    * The shmem pages are disallowed to be evicted by the memory > >> shrinker > >> +    * while count is non-zero. Used internally by memory shrinker. > >> +    */ > >> +   unsigned int eviction_disable_count; > >> + > >> +   /** > >> +    * @purging_disable_count: > >> +    * > >> +    * The shmem pages are disallowed to be purged by the memory > >> shrinker > >> +    * while count is non-zero. Used internally by memory shrinker. > >> +    */ > >> +   unsigned int purging_disable_count; What are these disable counts for? The way purgeable works in other drivers is that userspace sets purgeable or not, and it's up to userspace to not make a mess of this. There's also some interactions, and I guess a bunch of drivers get this wrong in funny ways. Not sure how to best clean this up. - Once you have a shrinker/dynamic memory management you should _not_ pin pages, except when it's truly permanent like for scanout. Instead drivers should attach dma_fence to the dma_resv to denote in-flight access. - A pinned buffer object is not allowed to be put into purgeable state, and a bo in purgeable state should not be allowed to be pinned. - Drivers need to hold dma_resv_lock for long enough in their command submission, i.e. from the point where the reserve the buffers and make sure that mappings exists, to the point where the request is submitted to hw or drm/sched and fences are installed. But I think a lot of current shmem users just pin as part of execbuf, so this won't work quite so well right out of the box. Anyway with that design I don't think there should ever be a need to disable shrinking. > >> + > >> +   /** > >> +    * @pages_state: Current state of shmem pages. Used internally by > >> +    * memory shrinker. > >> +    */ > >> +   enum drm_gem_shmem_pages_state pages_state; > >> + > >> +   /** > >> +    * @evicted: True if shmem pages were evicted by the memory > >> shrinker. > >> +    * Used internally by memory shrinker. > >> +    */ > >> +   bool evicted; > >> + > >> +   /** > >> +    * @pages_shrinkable: True if shmem pages can be evicted or purged > >> +    * by the memory shrinker. Used internally by memory shrinker. > >> +    */ > >> +   bool pages_shrinkable; > > > > As commented before, this state can be foundby looking at existing > > fields. No need to store it separately. > > When we're transitioning from "evictable" to a "purgeable" state, we > must not add pages twice to the "shrinkable_count" variable. Hence this > is not a state, but a variable which prevents the double accounting of > the pages. Please see drm_gem_shmem_add_pages_to_shrinker() in this patch. > > Perhaps something like "pages_accounted_by_shrinker" could be a better > name for the variable. I'll revisit this for v5. Hm not sure we need to account this? Usually the shrinker just counts when it's asked to do so, not practively maintain that count. Once you start shrinking burning cpu time is generally not too terrible. -Daniel
27.04.2022 18:03, Daniel Vetter wrote: >> ... >>>> @@ -172,6 +172,41 @@ struct drm_gem_object_funcs { >>>> Â Â Â Â Â Â * This is optional but necessary for mmap support. >>>> Â Â Â Â Â Â */ >>>> Â Â Â Â Â const struct vm_operations_struct *vm_ops; >>>> + >>>> +Â Â Â /** >>>> +Â Â Â Â * @purge: >>>> +Â Â Â Â * >>>> +Â Â Â Â * Releases the GEM object's allocated backing storage to the >>>> system. >>>> +Â Â Â Â * >>>> +Â Â Â Â * Returns the number of pages that have been freed by purging >>>> the GEM object. >>>> +Â Â Â Â * >>>> +Â Â Â Â * This callback is used by the GEM shrinker. >>>> +Â Â Â Â */ >>>> +Â Â Â unsigned long (*purge)(struct drm_gem_object *obj); > > Hm I feel like drivers shouldn't need to know the difference here? > > Like shmem helpers can track what's purgeable, and for eviction/purging > the driver callback should do the same? > > The only difference is when we try to re-reserve the backing storage. When > the object has been evicted that should suceed, but when the object is > purged that will fail. > > That's the difference between evict and purge for drivers? When buffer is purged, we can permanently release the backing storage and the reserved IOV space, re-using the freed space by new BOs. When buffer is evicted, the BO's IOV should be kept reserved and the re-reservation of the backing storage should succeed. >>>> + >>>> +Â Â Â /** >>>> +Â Â Â Â * @evict: >>>> +Â Â Â Â * >>>> +Â Â Â Â * Unpins the GEM object's allocated backing storage, allowing >>>> shmem pages >>>> +Â Â Â Â * to be swapped out. >>> >>> What's the difference to the existing unpin() callback? >> >> Drivers need to do more than just unpinning pages when GEMs are evicted. >> Unpinning is only a part of the eviction process. I'll improve the >> doc-comment in v5. >> >> For example, for VirtIO-GPU driver we need to to detach host from the >> guest's memory before pages are evicted [1]. >> >> [1] >> https://gitlab.collabora.com/dmitry.osipenko/linux-kernel-rd/-/blob/932eb03198bce3a21353b09ab71e95f1c19b84c2/drivers/gpu/drm/virtio/virtgpu_object.c#L145 >> >> In case of Panfrost driver, we will need to remove mappings before pages >> are evicted. > > It might be good to align this with ttm, otoh that all works quite a bit > differently for ttm since ttm supports buffer moves and a lot more fancy > stuff. > > I'm bringing this up since I have this fancy idea that eventually we could > glue shmem helpers into ttm in some cases for managing buffers when they > sit in system memory (as opposed to vram). I'll take a look at ttm for v6. >>>> +Â Â Â Â * >>>> +Â Â Â Â * Returns the number of pages that have been unpinned. >>>> +Â Â Â Â * >>>> +Â Â Â Â * This callback is used by the GEM shrinker. >>>> +Â Â Â Â */ >>>> +Â Â Â unsigned long (*evict)(struct drm_gem_object *obj); >>>> + >>>> +Â Â Â /** >>>> +Â Â Â Â * @swap_in: >>>> +Â Â Â Â * >>>> +Â Â Â Â * Pins GEM object's allocated backing storage if it was >>>> previously evicted, >>>> +Â Â Â Â * moving swapped out pages back to memory. >>>> +Â Â Â Â * >>>> +Â Â Â Â * Returns 0 on success, or -errno on error. >>>> +Â Â Â Â * >>>> +Â Â Â Â * This callback is used by the GEM shrinker. >>>> +Â Â Â Â */ >>>> +Â Â Â int (*swap_in)(struct drm_gem_object *obj); >>> >>> Why do you need swap_in()? This can be done on-demand as part of a pin >>> or vmap operation. >> >> Similarly to the unpinning, the pining of pages is only a part of what >> needs to be done for GPU drivers. Besides of returning pages back to >> memory, we also need to make them accessible to GPU and this is a >> driver-specific process. This why we need the additional callbacks. > > This is a bit much midlayer. The way this works in ttm is you reserve all > the objects you need (which makes sure they're physically available > again), and then the driver goes through and makes sure the page tables > are all set up again. > > Once you get towards gpu vm that's really the only approach, since your > swap_in has no idea for which vm it needs to restore pagetables (and > restoring it for all is a bit meh). > > If drivers want to optimize this they can adjust/set any tracking > information from their evict callback as needed. In practice, majority of BOs have only one mapping. Only shared BOs usually have extra mappings and shared BOs aren't evictable. When memory pages are gone, then all the GPU mappings also should be gone. Perhaps it's indeed won't be a bad idea to move out the restoring of h/w VMs from the swap_in() and make drivers to handle the restoring by themselves, so swap_in() will be only about restoring the pages. I'll try to improve it in v6. >>>> Â }; >>>> Â Â /** >>>> diff --git a/include/drm/drm_gem_shmem_helper.h >>>> b/include/drm/drm_gem_shmem_helper.h >>>> index 70889533962a..a65557b446e6 100644 >>>> --- a/include/drm/drm_gem_shmem_helper.h >>>> +++ b/include/drm/drm_gem_shmem_helper.h >>>> @@ -6,6 +6,7 @@ >>>> Â #include <linux/fs.h> >>>> Â #include <linux/mm.h> >>>> Â #include <linux/mutex.h> >>>> +#include <linux/shrinker.h> >>>> Â Â #include <drm/drm_file.h> >>>> Â #include <drm/drm_gem.h> >>>> @@ -15,8 +16,18 @@ >>>> Â struct dma_buf_attachment; >>>> Â struct drm_mode_create_dumb; >>>> Â struct drm_printer; >>>> +struct drm_device; >>>> Â struct sg_table; >>>> Â +enum drm_gem_shmem_pages_state { >>>> +Â Â Â DRM_GEM_SHMEM_PAGES_STATE_PURGED = -2, >>>> +Â Â Â DRM_GEM_SHMEM_PAGES_STATE_EVICTED = -1, >>>> +Â Â Â DRM_GEM_SHMEM_PAGES_STATE_UNPINNED = 0, >>>> +Â Â Â DRM_GEM_SHMEM_PAGES_STATE_PINNED = 1, >>>> +Â Â Â DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE = 2, >>>> +Â Â Â DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE = 3, >>>> +}; >>> >>> These states can be detected by looking at the vmap and pin refcounts. >>> No need to store them explicitly. >> >> I'll try to revisit this, but I was finding that it's much more >> difficult to follow and debug code without the explicit states. > > purgeable/purged needs some state, but pinned shouldn't be duplicated, so > I concur here a bit. > >>> In your patch, they also come with a >>> big zoo of trivial helpers. None of that seems necessary AFAICT. >> >> There are couple functions which could be squashed, although this may >> hurt readability of the code a tad. I'll try to take another look at >> this for v5. >> >>> What's the difference between purge and evict BTW? >> >> The evicted pages are moved out from memory to a SWAP partition or file. >> >> The purged pages are destroyed permanently. >> >>>> + >>>> Â /** >>>> Â Â * struct drm_gem_shmem_object - GEM object backed by shmem >>>> Â Â */ >>>> @@ -43,8 +54,8 @@ struct drm_gem_shmem_object { >>>> Â Â Â Â Â Â * @madv: State for madvise >>>> Â Â Â Â Â Â * >>>> Â Â Â Â Â Â * 0 is active/inuse. >>>> +Â Â Â Â * 1 is not-needed/can-be-purged >>>> Â Â Â Â Â Â * A negative value is the object is purged. >>>> -Â Â Â Â * Positive values are driver specific and not used by the helpers. >>>> Â Â Â Â Â Â */ >>>> Â Â Â Â Â int madv; >>>> Â @@ -91,6 +102,40 @@ struct drm_gem_shmem_object { >>>> Â Â Â Â Â Â * @map_wc: map object write-combined (instead of using shmem >>>> defaults). >>>> Â Â Â Â Â Â */ >>>> Â Â Â Â Â bool map_wc; >>>> + >>>> +Â Â Â /** >>>> +Â Â Â Â * @eviction_disable_count: >>>> +Â Â Â Â * >>>> +Â Â Â Â * The shmem pages are disallowed to be evicted by the memory >>>> shrinker >>>> +Â Â Â Â * while count is non-zero. Used internally by memory shrinker. >>>> +Â Â Â Â */ >>>> +Â Â Â unsigned int eviction_disable_count; >>>> + >>>> +Â Â Â /** >>>> +Â Â Â Â * @purging_disable_count: >>>> +Â Â Â Â * >>>> +Â Â Â Â * The shmem pages are disallowed to be purged by the memory >>>> shrinker >>>> +Â Â Â Â * while count is non-zero. Used internally by memory shrinker. >>>> +Â Â Â Â */ >>>> +Â Â Â unsigned int purging_disable_count; > > What are these disable counts for? Some of BO types should stay pinned permanently, this applies to both VirtIO and Panfrost drivers that make use of the generic shrinker in this patchset. Hence I made objects unpurgeable and unevictable by default. Initially the idea of these counts was to allow drivers to explicitly disable purging and eviction, and do it multiple times. If driver disables eviction in two different places in the code, then we need to track the eviction-disable count. In the v5 of this patchset drivers don't need to explicitly disable shrinking anymore, they only need to enable it. The counts are also used internally by DRM SHMEM core to track the vmappings and pinnings, but perhaps pages_use_count could be used for that instead. I'll revisit it for v6. > The way purgeable works in other drivers is that userspace sets purgeable > or not, and it's up to userspace to not make a mess of this. > > There's also some interactions, and I guess a bunch of drivers get this > wrong in funny ways. Not sure how to best clean this up. > > - Once you have a shrinker/dynamic memory management you should _not_ pin > pages, except when it's truly permanent like for scanout. Instead > drivers should attach dma_fence to the dma_resv to denote in-flight > access. By default pages are pinned when drm_gem_shmem_get_pages_sgt() is invoked by drivers during of BO creation time. We could declare that pages_use_count=1 means the pages are allowed to be evicted and purged if shrinker is enabled. Then the further drm_gem_shmem_pin/vmap() calls will bump the pages_use_count, disallowing the eviction and purging, like you're suggesting, and we won't need the explicit counts. > - A pinned buffer object is not allowed to be put into purgeable state, > and a bo in purgeable state should not be allowed to be pinned. > > - Drivers need to hold dma_resv_lock for long enough in their command > submission, i.e. from the point where the reserve the buffers and make > sure that mappings exists, to the point where the request is submitted > to hw or drm/sched and fences are installed. > > But I think a lot of current shmem users just pin as part of execbuf, so > this won't work quite so well right out of the box. The current shmem users assume that BO is pinned permanently once it has been created. > Anyway with that design I don't think there should ever be a need to > disable shrinking. To me what you described mostly matches to what I did in the v5. >>>> + >>>> +Â Â Â /** >>>> +Â Â Â Â * @pages_state: Current state of shmem pages. Used internally by >>>> +Â Â Â Â * memory shrinker. >>>> +Â Â Â Â */ >>>> +Â Â Â enum drm_gem_shmem_pages_state pages_state; >>>> + >>>> +Â Â Â /** >>>> +Â Â Â Â * @evicted: True if shmem pages were evicted by the memory >>>> shrinker. >>>> +Â Â Â Â * Used internally by memory shrinker. >>>> +Â Â Â Â */ >>>> +Â Â Â bool evicted; >>>> + >>>> +Â Â Â /** >>>> +Â Â Â Â * @pages_shrinkable: True if shmem pages can be evicted or purged >>>> +Â Â Â Â * by the memory shrinker. Used internally by memory shrinker. >>>> +Â Â Â Â */ >>>> +Â Â Â bool pages_shrinkable; >>> >>> As commented before, this state can be foundby looking at existing >>> fields. No need to store it separately. >> >> When we're transitioning from "evictable" to a "purgeable" state, we >> must not add pages twice to the "shrinkable_count" variable. Hence this >> is not a state, but a variable which prevents the double accounting of >> the pages. Please see drm_gem_shmem_add_pages_to_shrinker() in this patch. >> >> Perhaps something like "pages_accounted_by_shrinker" could be a better >> name for the variable. I'll revisit this for v5. > > Hm not sure we need to account this? Usually the shrinker just counts when > it's asked to do so, not practively maintain that count. Once you start > shrinking burning cpu time is generally not too terrible. We could count pages on demand by walking up the "evictable" list, but then the shrinker's lock needs to be taken by the drm_gem_shmem_shrinker_count_objects() to protect the list. Previously Rob Clark said that the profiling of freedreno's shrinker showed that it's worthwhile to reduce the locks as much as possible, including the case of counting shrinkable objects.
On Thu, Apr 28, 2022 at 09:20:15PM +0300, Dmitry Osipenko wrote: > 27.04.2022 18:03, Daniel Vetter wrote: > >> ... > >>>> @@ -172,6 +172,41 @@ struct drm_gem_object_funcs { > >>>> Â Â Â Â Â Â * This is optional but necessary for mmap support. > >>>> Â Â Â Â Â Â */ > >>>> Â Â Â Â Â const struct vm_operations_struct *vm_ops; > >>>> + > >>>> +Â Â Â /** > >>>> +Â Â Â Â * @purge: > >>>> +Â Â Â Â * > >>>> +Â Â Â Â * Releases the GEM object's allocated backing storage to the > >>>> system. > >>>> +Â Â Â Â * > >>>> +Â Â Â Â * Returns the number of pages that have been freed by purging > >>>> the GEM object. > >>>> +Â Â Â Â * > >>>> +Â Â Â Â * This callback is used by the GEM shrinker. > >>>> +Â Â Â Â */ > >>>> +Â Â Â unsigned long (*purge)(struct drm_gem_object *obj); > > > > Hm I feel like drivers shouldn't need to know the difference here? > > > > Like shmem helpers can track what's purgeable, and for eviction/purging > > the driver callback should do the same? > > > > The only difference is when we try to re-reserve the backing storage. When > > the object has been evicted that should suceed, but when the object is > > purged that will fail. > > > > That's the difference between evict and purge for drivers? > > When buffer is purged, we can permanently release the backing storage > and the reserved IOV space, re-using the freed space by new BOs. > > When buffer is evicted, the BO's IOV should be kept reserved and the > re-reservation of the backing storage should succeed. Yeah but what's the difference for driver callbacks? In both cases the driver callback needs to tear down gpu mappings and pagetables. The only difference happens after that in the shmem helper: For purge we ditch the shmem object, for evict we keep it. Drivers shouldn't need to care about that difference, hence why the two callbacks? > > >>>> + > >>>> +Â Â Â /** > >>>> +Â Â Â Â * @evict: > >>>> +Â Â Â Â * > >>>> +Â Â Â Â * Unpins the GEM object's allocated backing storage, allowing > >>>> shmem pages > >>>> +Â Â Â Â * to be swapped out. > >>> > >>> What's the difference to the existing unpin() callback? > >> > >> Drivers need to do more than just unpinning pages when GEMs are evicted. > >> Unpinning is only a part of the eviction process. I'll improve the > >> doc-comment in v5. > >> > >> For example, for VirtIO-GPU driver we need to to detach host from the > >> guest's memory before pages are evicted [1]. > >> > >> [1] > >> https://gitlab.collabora.com/dmitry.osipenko/linux-kernel-rd/-/blob/932eb03198bce3a21353b09ab71e95f1c19b84c2/drivers/gpu/drm/virtio/virtgpu_object.c#L145 > >> > >> In case of Panfrost driver, we will need to remove mappings before pages > >> are evicted. > > > > It might be good to align this with ttm, otoh that all works quite a bit > > differently for ttm since ttm supports buffer moves and a lot more fancy > > stuff. > > > > I'm bringing this up since I have this fancy idea that eventually we could > > glue shmem helpers into ttm in some cases for managing buffers when they > > sit in system memory (as opposed to vram). > > I'll take a look at ttm for v6. > > >>>> +Â Â Â Â * > >>>> +Â Â Â Â * Returns the number of pages that have been unpinned. > >>>> +Â Â Â Â * > >>>> +Â Â Â Â * This callback is used by the GEM shrinker. > >>>> +Â Â Â Â */ > >>>> +Â Â Â unsigned long (*evict)(struct drm_gem_object *obj); > >>>> + > >>>> +Â Â Â /** > >>>> +Â Â Â Â * @swap_in: > >>>> +Â Â Â Â * > >>>> +Â Â Â Â * Pins GEM object's allocated backing storage if it was > >>>> previously evicted, > >>>> +Â Â Â Â * moving swapped out pages back to memory. > >>>> +Â Â Â Â * > >>>> +Â Â Â Â * Returns 0 on success, or -errno on error. > >>>> +Â Â Â Â * > >>>> +Â Â Â Â * This callback is used by the GEM shrinker. > >>>> +Â Â Â Â */ > >>>> +Â Â Â int (*swap_in)(struct drm_gem_object *obj); > >>> > >>> Why do you need swap_in()? This can be done on-demand as part of a pin > >>> or vmap operation. > >> > >> Similarly to the unpinning, the pining of pages is only a part of what > >> needs to be done for GPU drivers. Besides of returning pages back to > >> memory, we also need to make them accessible to GPU and this is a > >> driver-specific process. This why we need the additional callbacks. > > > > This is a bit much midlayer. The way this works in ttm is you reserve all > > the objects you need (which makes sure they're physically available > > again), and then the driver goes through and makes sure the page tables > > are all set up again. > > > > Once you get towards gpu vm that's really the only approach, since your > > swap_in has no idea for which vm it needs to restore pagetables (and > > restoring it for all is a bit meh). > > > > If drivers want to optimize this they can adjust/set any tracking > > information from their evict callback as needed. > > In practice, majority of BOs have only one mapping. Only shared BOs > usually have extra mappings and shared BOs aren't evictable. That seems like a fairly arbitrary limitations, and e.g. i915 doesn't have this limitation, and also has a shrinker. I don't think it should be built into the design. > When memory pages are gone, then all the GPU mappings also should be > gone. Perhaps it's indeed won't be a bad idea to move out the restoring > of h/w VMs from the swap_in() and make drivers to handle the restoring > by themselves, so swap_in() will be only about restoring the pages. I'll > try to improve it in v6. Sounds good. -Daniel > > >>>> Â }; > >>>> Â Â /** > >>>> diff --git a/include/drm/drm_gem_shmem_helper.h > >>>> b/include/drm/drm_gem_shmem_helper.h > >>>> index 70889533962a..a65557b446e6 100644 > >>>> --- a/include/drm/drm_gem_shmem_helper.h > >>>> +++ b/include/drm/drm_gem_shmem_helper.h > >>>> @@ -6,6 +6,7 @@ > >>>> Â #include <linux/fs.h> > >>>> Â #include <linux/mm.h> > >>>> Â #include <linux/mutex.h> > >>>> +#include <linux/shrinker.h> > >>>> Â Â #include <drm/drm_file.h> > >>>> Â #include <drm/drm_gem.h> > >>>> @@ -15,8 +16,18 @@ > >>>> Â struct dma_buf_attachment; > >>>> Â struct drm_mode_create_dumb; > >>>> Â struct drm_printer; > >>>> +struct drm_device; > >>>> Â struct sg_table; > >>>> Â +enum drm_gem_shmem_pages_state { > >>>> +Â Â Â DRM_GEM_SHMEM_PAGES_STATE_PURGED = -2, > >>>> +Â Â Â DRM_GEM_SHMEM_PAGES_STATE_EVICTED = -1, > >>>> +Â Â Â DRM_GEM_SHMEM_PAGES_STATE_UNPINNED = 0, > >>>> +Â Â Â DRM_GEM_SHMEM_PAGES_STATE_PINNED = 1, > >>>> +Â Â Â DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE = 2, > >>>> +Â Â Â DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE = 3, > >>>> +}; > >>> > >>> These states can be detected by looking at the vmap and pin refcounts. > >>> No need to store them explicitly. > >> > >> I'll try to revisit this, but I was finding that it's much more > >> difficult to follow and debug code without the explicit states. > > > > purgeable/purged needs some state, but pinned shouldn't be duplicated, so > > I concur here a bit. > > > >>> In your patch, they also come with a > >>> big zoo of trivial helpers. None of that seems necessary AFAICT. > >> > >> There are couple functions which could be squashed, although this may > >> hurt readability of the code a tad. I'll try to take another look at > >> this for v5. > >> > >>> What's the difference between purge and evict BTW? > >> > >> The evicted pages are moved out from memory to a SWAP partition or file. > >> > >> The purged pages are destroyed permanently. > >> > >>>> + > >>>> Â /** > >>>> Â Â * struct drm_gem_shmem_object - GEM object backed by shmem > >>>> Â Â */ > >>>> @@ -43,8 +54,8 @@ struct drm_gem_shmem_object { > >>>> Â Â Â Â Â Â * @madv: State for madvise > >>>> Â Â Â Â Â Â * > >>>> Â Â Â Â Â Â * 0 is active/inuse. > >>>> +Â Â Â Â * 1 is not-needed/can-be-purged > >>>> Â Â Â Â Â Â * A negative value is the object is purged. > >>>> -Â Â Â Â * Positive values are driver specific and not used by the helpers. > >>>> Â Â Â Â Â Â */ > >>>> Â Â Â Â Â int madv; > >>>> Â @@ -91,6 +102,40 @@ struct drm_gem_shmem_object { > >>>> Â Â Â Â Â Â * @map_wc: map object write-combined (instead of using shmem > >>>> defaults). > >>>> Â Â Â Â Â Â */ > >>>> Â Â Â Â Â bool map_wc; > >>>> + > >>>> +Â Â Â /** > >>>> +Â Â Â Â * @eviction_disable_count: > >>>> +Â Â Â Â * > >>>> +Â Â Â Â * The shmem pages are disallowed to be evicted by the memory > >>>> shrinker > >>>> +Â Â Â Â * while count is non-zero. Used internally by memory shrinker. > >>>> +Â Â Â Â */ > >>>> +Â Â Â unsigned int eviction_disable_count; > >>>> + > >>>> +Â Â Â /** > >>>> +Â Â Â Â * @purging_disable_count: > >>>> +Â Â Â Â * > >>>> +Â Â Â Â * The shmem pages are disallowed to be purged by the memory > >>>> shrinker > >>>> +Â Â Â Â * while count is non-zero. Used internally by memory shrinker. > >>>> +Â Â Â Â */ > >>>> +Â Â Â unsigned int purging_disable_count; > > > > What are these disable counts for? > > Some of BO types should stay pinned permanently, this applies to both > VirtIO and Panfrost drivers that make use of the generic shrinker in > this patchset. Hence I made objects unpurgeable and unevictable by default. > > Initially the idea of these counts was to allow drivers to explicitly > disable purging and eviction, and do it multiple times. If driver > disables eviction in two different places in the code, then we need to > track the eviction-disable count. > > In the v5 of this patchset drivers don't need to explicitly disable > shrinking anymore, they only need to enable it. The counts are also used > internally by DRM SHMEM core to track the vmappings and pinnings, but > perhaps pages_use_count could be used for that instead. I'll revisit it > for v6. > > > The way purgeable works in other drivers is that userspace sets purgeable > > or not, and it's up to userspace to not make a mess of this. > > > > There's also some interactions, and I guess a bunch of drivers get this > > wrong in funny ways. Not sure how to best clean this up. > > > > - Once you have a shrinker/dynamic memory management you should _not_ pin > > pages, except when it's truly permanent like for scanout. Instead > > drivers should attach dma_fence to the dma_resv to denote in-flight > > access. > > By default pages are pinned when drm_gem_shmem_get_pages_sgt() is > invoked by drivers during of BO creation time. > > We could declare that pages_use_count=1 means the pages are allowed to > be evicted and purged if shrinker is enabled. Then the further > drm_gem_shmem_pin/vmap() calls will bump the pages_use_count, > disallowing the eviction and purging, like you're suggesting, and we > won't need the explicit counts. > > > - A pinned buffer object is not allowed to be put into purgeable state, > > and a bo in purgeable state should not be allowed to be pinned. > > > > - Drivers need to hold dma_resv_lock for long enough in their command > > submission, i.e. from the point where the reserve the buffers and make > > sure that mappings exists, to the point where the request is submitted > > to hw or drm/sched and fences are installed. > > > > But I think a lot of current shmem users just pin as part of execbuf, so > > this won't work quite so well right out of the box. > > The current shmem users assume that BO is pinned permanently once it has > been created. > > > Anyway with that design I don't think there should ever be a need to > > disable shrinking. > > To me what you described mostly matches to what I did in the v5. > > >>>> + > >>>> +Â Â Â /** > >>>> +Â Â Â Â * @pages_state: Current state of shmem pages. Used internally by > >>>> +Â Â Â Â * memory shrinker. > >>>> +Â Â Â Â */ > >>>> +Â Â Â enum drm_gem_shmem_pages_state pages_state; > >>>> + > >>>> +Â Â Â /** > >>>> +Â Â Â Â * @evicted: True if shmem pages were evicted by the memory > >>>> shrinker. > >>>> +Â Â Â Â * Used internally by memory shrinker. > >>>> +Â Â Â Â */ > >>>> +Â Â Â bool evicted; > >>>> + > >>>> +Â Â Â /** > >>>> +Â Â Â Â * @pages_shrinkable: True if shmem pages can be evicted or purged > >>>> +Â Â Â Â * by the memory shrinker. Used internally by memory shrinker. > >>>> +Â Â Â Â */ > >>>> +Â Â Â bool pages_shrinkable; > >>> > >>> As commented before, this state can be foundby looking at existing > >>> fields. No need to store it separately. > >> > >> When we're transitioning from "evictable" to a "purgeable" state, we > >> must not add pages twice to the "shrinkable_count" variable. Hence this > >> is not a state, but a variable which prevents the double accounting of > >> the pages. Please see drm_gem_shmem_add_pages_to_shrinker() in this patch. > >> > >> Perhaps something like "pages_accounted_by_shrinker" could be a better > >> name for the variable. I'll revisit this for v5. > > > > Hm not sure we need to account this? Usually the shrinker just counts when > > it's asked to do so, not practively maintain that count. Once you start > > shrinking burning cpu time is generally not too terrible. > > We could count pages on demand by walking up the "evictable" list, but > then the shrinker's lock needs to be taken by the > drm_gem_shmem_shrinker_count_objects() to protect the list. > > Previously Rob Clark said that the profiling of freedreno's shrinker > showed that it's worthwhile to reduce the locks as much as possible, > including the case of counting shrinkable objects.
Hi Am 18.04.22 um 00:37 schrieb Dmitry Osipenko: > Introduce a common DRM SHMEM shrinker. It allows to reduce code > duplication among DRM drivers that implement theirs own shrinkers. > This is initial version of the shrinker that covers basic needs of > GPU drivers, both purging and eviction of shmem objects are supported. > > This patch is based on a couple ideas borrowed from Rob's Clark MSM > shrinker and Thomas' Zimmermann variant of SHMEM shrinker. > > In order to start using DRM SHMEM shrinker drivers should: > > 1. Implement new purge(), evict() + swap_in() GEM callbacks. > 2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device). > 3. Use drm_gem_shmem_set_purgeable_and_evictable(shmem) and alike API > functions to activate shrinking of GEMs. Honestly speaking, after reading the patch and the discussion here I really don't like where all tis is going. The interfaces and implementation are overengineered. Descisions about evicting and purging should be done by the memory manager. For the most part, it's none of the driver's business. 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. Best regards Thomas > > Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 765 ++++++++++++++++++++++++- > include/drm/drm_device.h | 4 + > include/drm/drm_gem.h | 35 ++ > include/drm/drm_gem_shmem_helper.h | 105 +++- > 4 files changed, 877 insertions(+), 32 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > index 3ecef571eff3..3838fb8d6f3a 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -88,6 +88,13 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private) > > INIT_LIST_HEAD(&shmem->madv_list); > > + /* > + * Eviction and purging are disabled by default, shmem user must enable > + * them explicitly using drm_gem_shmem_set_evictable/purgeable(). > + */ > + shmem->eviction_disable_count = 1; > + shmem->purging_disable_count = 1; > + > if (!private) { > /* > * Our buffers are kept pinned, so allocating them > @@ -126,6 +133,107 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t > } > EXPORT_SYMBOL_GPL(drm_gem_shmem_create); > > +static void > +drm_gem_shmem_add_pages_to_shrinker(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 (!shmem->pages_shrinkable) { > + WARN_ON(gem_shrinker->shrinkable_count + page_count < page_count); > + gem_shrinker->shrinkable_count += page_count; > + shmem->pages_shrinkable = true; > + } > +} > + > +static void > +drm_gem_shmem_remove_pages_from_shrinker(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 (shmem->pages_shrinkable) { > + WARN_ON(gem_shrinker->shrinkable_count < page_count); > + gem_shrinker->shrinkable_count -= page_count; > + shmem->pages_shrinkable = false; > + } > +} > + > +static void > +drm_gem_shmem_set_pages_state_locked(struct drm_gem_shmem_object *shmem, > + enum drm_gem_shmem_pages_state new_state) > +{ > + struct drm_gem_object *obj = &shmem->base; > + struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker; > + > + lockdep_assert_held(&gem_shrinker->lock); > + lockdep_assert_held(&obj->resv->lock.base); > + > + if (new_state >= DRM_GEM_SHMEM_PAGES_STATE_PINNED) { > + if (drm_gem_shmem_is_evictable(shmem)) > + new_state = DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE; > + > + if (drm_gem_shmem_is_purgeable(shmem)) > + new_state = DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE; > + > + if (!shmem->pages) > + new_state = DRM_GEM_SHMEM_PAGES_STATE_UNPINNED; > + > + if (shmem->evicted) > + new_state = DRM_GEM_SHMEM_PAGES_STATE_EVICTED; > + } > + > + if (shmem->pages_state == new_state) > + return; > + > + switch (new_state) { > + case DRM_GEM_SHMEM_PAGES_STATE_UNPINNED: > + case DRM_GEM_SHMEM_PAGES_STATE_PURGED: > + drm_gem_shmem_remove_pages_from_shrinker(shmem); > + list_del_init(&shmem->madv_list); > + break; > + > + case DRM_GEM_SHMEM_PAGES_STATE_PINNED: > + drm_gem_shmem_remove_pages_from_shrinker(shmem); > + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_active); > + break; > + > + case DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE: > + drm_gem_shmem_add_pages_to_shrinker(shmem); > + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_purgeable); > + break; > + > + case DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE: > + drm_gem_shmem_add_pages_to_shrinker(shmem); > + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_evictable); > + break; > + > + case DRM_GEM_SHMEM_PAGES_STATE_EVICTED: > + drm_gem_shmem_remove_pages_from_shrinker(shmem); > + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_evicted); > + break; > + } > + > + shmem->pages_state = new_state; > +} > + > +static void > +drm_gem_shmem_set_pages_state(struct drm_gem_shmem_object *shmem, > + enum drm_gem_shmem_pages_state new_state) > +{ > + struct drm_gem_object *obj = &shmem->base; > + struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker; > + > + if (!gem_shrinker) > + return; > + > + mutex_lock(&gem_shrinker->lock); > + drm_gem_shmem_set_pages_state_locked(shmem, new_state); > + mutex_unlock(&gem_shrinker->lock); > +} > + > /** > * drm_gem_shmem_free - Free resources associated with a shmem GEM object > * @shmem: shmem GEM object to free > @@ -137,6 +245,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, -1); > + > WARN_ON(shmem->vmap_use_count); > > if (obj->import_attach) { > @@ -148,7 +259,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > sg_free_table(shmem->sgt); > kfree(shmem->sgt); > } > - if (shmem->pages) > + if (shmem->pages_use_count) > drm_gem_shmem_put_pages(shmem); > } > > @@ -159,18 +270,226 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > } > EXPORT_SYMBOL_GPL(drm_gem_shmem_free); > > -static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem) > +static void drm_gem_shmem_update_pages_state_locked(struct drm_gem_shmem_object *shmem) > +{ > + struct drm_gem_object *obj = &shmem->base; > + struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker; > + enum drm_gem_shmem_pages_state new_state; > + > + if (!gem_shrinker || obj->import_attach) > + return; > + > + mutex_lock(&gem_shrinker->lock); > + > + if (!shmem->madv) > + new_state = DRM_GEM_SHMEM_PAGES_STATE_PINNED; > + else if (shmem->madv > 0) > + new_state = DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE; > + else > + new_state = DRM_GEM_SHMEM_PAGES_STATE_PURGED; > + > + drm_gem_shmem_set_pages_state_locked(shmem, new_state); > + > + mutex_unlock(&gem_shrinker->lock); > +} > + > +static void drm_gem_shmem_update_pages_state(struct drm_gem_shmem_object *shmem) > +{ > + dma_resv_lock(shmem->base.resv, NULL); > + drm_gem_shmem_update_pages_state_locked(shmem); > + dma_resv_unlock(shmem->base.resv); > +} > + > +static int > +drm_gem_shmem_set_evictable_locked(struct drm_gem_shmem_object *shmem) > +{ > + int ret = 0; > + > + WARN_ON_ONCE(!shmem->eviction_disable_count--); > + > + if (shmem->madv < 0) > + ret = -ENOMEM; > + > + drm_gem_shmem_update_pages_state_locked(shmem); > + > + return ret; > +} > + > +static int > +drm_gem_shmem_set_unevictable_locked(struct drm_gem_shmem_object *shmem) > +{ > + struct drm_gem_object *obj = &shmem->base; > + int err; > + > + if (shmem->madv < 0) > + return -ENOMEM; > + > + if (shmem->evicted) { > + err = obj->funcs->swap_in(obj); > + if (err) > + return err; > + } > + > + shmem->eviction_disable_count++; > + > + drm_gem_shmem_update_pages_state_locked(shmem); > + > + return 0; > +} > + > +static int > +drm_gem_shmem_set_purgeable_locked(struct drm_gem_shmem_object *shmem) > +{ > + int ret = 0; > + > + WARN_ON_ONCE(!shmem->purging_disable_count--); > + > + if (shmem->madv < 0) > + ret = -ENOMEM; > + > + drm_gem_shmem_update_pages_state_locked(shmem); > + > + return ret; > +} > + > +/** > + * drm_gem_shmem_set_purgeable() - Make GEM purgeable by memory shrinker > + * @shmem: shmem GEM object > + * > + * Tell memory shrinker that this GEM can be purged. Initially purging is > + * disabled for all GEMs. Each set_pureable() call must have corresponding > + * set_unpureable() call. If GEM was purged, then -ENOMEM is returned. > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_gem_shmem_set_purgeable(struct drm_gem_shmem_object *shmem) > +{ > + int ret; > + > + dma_resv_lock(shmem->base.resv, NULL); > + ret = drm_gem_shmem_set_purgeable_locked(shmem); > + dma_resv_unlock(shmem->base.resv); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(drm_gem_shmem_set_purgeable); > + > +static int > +drm_gem_shmem_set_unpurgeable_locked(struct drm_gem_shmem_object *shmem) > +{ > + if (shmem->madv < 0) > + return -ENOMEM; > + > + shmem->purging_disable_count++; > + > + drm_gem_shmem_update_pages_state_locked(shmem); > + > + return 0; > +} > + > +static int > +drm_gem_shmem_set_purgeable_and_evictable_locked(struct drm_gem_shmem_object *shmem) > +{ > + int ret; > + > + ret = drm_gem_shmem_set_evictable_locked(shmem); > + if (!ret) { > + ret = drm_gem_shmem_set_purgeable_locked(shmem); > + if (ret) > + drm_gem_shmem_set_unevictable_locked(shmem); > + } > + > + return ret; > +} > + > +static int > +drm_gem_shmem_set_unpurgeable_and_unevictable_locked(struct drm_gem_shmem_object *shmem) > +{ > + int ret; > + > + ret = drm_gem_shmem_set_unpurgeable_locked(shmem); > + if (!ret) { > + ret = drm_gem_shmem_set_unevictable_locked(shmem); > + if (ret) > + drm_gem_shmem_set_purgeable_locked(shmem); > + } > + > + return ret; > +} > + > +/** > + * drm_gem_shmem_set_purgeable_and_evictable() - Make GEM unpurgeable and > + * unevictable by memory shrinker > + * @shmem: shmem GEM object > + * > + * Tell memory shrinker that this GEM can't be purged and evicted. Each > + * set_purgeable_and_evictable() call must have corresponding > + * unpurgeable_and_unevictable() call. If GEM was purged, then -ENOMEM > + * is returned. > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_gem_shmem_set_purgeable_and_evictable(struct drm_gem_shmem_object *shmem) > +{ > + int ret; > + > + dma_resv_lock(shmem->base.resv, NULL); > + ret = drm_gem_shmem_set_purgeable_and_evictable_locked(shmem); > + dma_resv_unlock(shmem->base.resv); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(drm_gem_shmem_set_purgeable_and_evictable); > + > +/** > + * drm_gem_shmem_set_unpurgeable_and_unevictable() - Make GEM purgeable and > + * evictable by memory shrinker > + * @shmem: shmem GEM object > + * > + * Tell memory shrinker that this GEM can be purged and evicted. Each > + * unpurgeable_and_unevictable() call must have corresponding > + * set_purgeable_and_evictable() call. If GEM was purged, then -ENOMEM > + * is returned. > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_gem_shmem_set_unpurgeable_and_unevictable(struct drm_gem_shmem_object *shmem) > +{ > + int ret; > + > + ret = dma_resv_lock_interruptible(shmem->base.resv, NULL); > + if (ret) > + return ret; > + > + ret = drm_gem_shmem_set_unpurgeable_and_unevictable_locked(shmem); > + dma_resv_unlock(shmem->base.resv); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(drm_gem_shmem_set_unpurgeable_and_unevictable); > + > +static int > +drm_gem_shmem_acquire_pages_locked(struct drm_gem_shmem_object *shmem) > { > struct drm_gem_object *obj = &shmem->base; > struct page **pages; > > - if (shmem->pages_use_count++ > 0) > + if (shmem->madv < 0) { > + WARN_ON(shmem->pages); > + return -ENOMEM; > + } > + > + if (shmem->pages) { > + WARN_ON(!shmem->evicted); > return 0; > + } > > pages = drm_gem_get_pages(obj); > if (IS_ERR(pages)) { > DRM_DEBUG_KMS("Failed to get pages (%ld)\n", PTR_ERR(pages)); > - shmem->pages_use_count = 0; > return PTR_ERR(pages); > } > > @@ -189,6 +508,25 @@ static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem) > return 0; > } > > +static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem) > +{ > + int err; > + > + if (shmem->madv < 0) > + return -ENOMEM; > + > + if (shmem->pages_use_count++ > 0) > + return 0; > + > + err = drm_gem_shmem_acquire_pages_locked(shmem); > + if (err) { > + shmem->pages_use_count = 0; > + return err; > + } > + > + return 0; > +} > + > /* > * drm_gem_shmem_get_pages - Allocate backing pages for a shmem GEM object > * @shmem: shmem GEM object > @@ -209,21 +547,38 @@ int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem) > if (ret) > return ret; > ret = drm_gem_shmem_get_pages_locked(shmem); > + > + drm_gem_shmem_update_pages_state_locked(shmem); > + > dma_resv_unlock(shmem->base.resv); > > return ret; > } > EXPORT_SYMBOL(drm_gem_shmem_get_pages); > > -static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) > +static void drm_gem_shmem_get_pages_no_fail(struct drm_gem_shmem_object *shmem) > { > - struct drm_gem_object *obj = &shmem->base; > + WARN_ON(shmem->base.import_attach); > > - if (WARN_ON_ONCE(!shmem->pages_use_count)) > - return; > + dma_resv_lock(shmem->base.resv, NULL); > > - if (--shmem->pages_use_count > 0) > + if (drm_gem_shmem_get_pages_locked(shmem)) > + shmem->pages_use_count++; > + > + drm_gem_shmem_update_pages_state_locked(shmem); > + > + dma_resv_unlock(shmem->base.resv); > +} > + > +static void > +drm_gem_shmem_release_pages_locked(struct drm_gem_shmem_object *shmem) > +{ > + struct drm_gem_object *obj = &shmem->base; > + > + if (!shmem->pages) { > + WARN_ON(!shmem->evicted && shmem->madv >= 0); > return; > + } > > #ifdef CONFIG_X86 > if (shmem->map_wc) > @@ -236,6 +591,21 @@ static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) > shmem->pages = NULL; > } > > +static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) > +{ > + struct drm_gem_object *obj = &shmem->base; > + > + lockdep_assert_held(&obj->resv->lock.base); > + > + if (WARN_ON(!shmem->pages_use_count)) > + return; > + > + if (--shmem->pages_use_count > 0) > + return; > + > + drm_gem_shmem_release_pages_locked(shmem); > +} > + > /* > * drm_gem_shmem_put_pages - Decrease use count on the backing pages for a shmem GEM object > * @shmem: shmem GEM object > @@ -246,6 +616,7 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem) > { > dma_resv_lock(shmem->base.resv, NULL); > drm_gem_shmem_put_pages_locked(shmem); > + drm_gem_shmem_update_pages_state_locked(shmem); > dma_resv_unlock(shmem->base.resv); > } > EXPORT_SYMBOL(drm_gem_shmem_put_pages); > @@ -262,9 +633,21 @@ EXPORT_SYMBOL(drm_gem_shmem_put_pages); > */ > int drm_gem_shmem_pin(struct drm_gem_shmem_object *shmem) > { > + int err; > + > WARN_ON(shmem->base.import_attach); > > - return drm_gem_shmem_get_pages(shmem); > + err = drm_gem_shmem_set_unpurgeable_and_unevictable(shmem); > + if (err) > + return err; > + > + err = drm_gem_shmem_get_pages(shmem); > + if (err) { > + drm_gem_shmem_set_purgeable_and_evictable(shmem); > + return err; > + } > + > + return 0; > } > EXPORT_SYMBOL(drm_gem_shmem_pin); > > @@ -280,6 +663,7 @@ void drm_gem_shmem_unpin(struct drm_gem_shmem_object *shmem) > WARN_ON(shmem->base.import_attach); > > drm_gem_shmem_put_pages(shmem); > + drm_gem_shmem_set_purgeable_and_evictable(shmem); > } > EXPORT_SYMBOL(drm_gem_shmem_unpin); > > @@ -359,7 +743,18 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem, > ret = dma_resv_lock_interruptible(shmem->base.resv, NULL); > if (ret) > return ret; > + > + ret = drm_gem_shmem_set_unpurgeable_and_unevictable_locked(shmem); > + if (ret) > + goto unlock; > + > ret = drm_gem_shmem_vmap_locked(shmem, map); > + if (ret) > + drm_gem_shmem_set_purgeable_and_evictable_locked(shmem); > + else > + drm_gem_shmem_update_pages_state_locked(shmem); > + > +unlock: > dma_resv_unlock(shmem->base.resv); > > return ret; > @@ -404,9 +799,9 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem, > { > dma_resv_lock(shmem->base.resv, NULL); > drm_gem_shmem_vunmap_locked(shmem, map); > + drm_gem_shmem_update_pages_state_locked(shmem); > + drm_gem_shmem_set_purgeable_and_evictable_locked(shmem); > dma_resv_unlock(shmem->base.resv); > - > - drm_gem_shmem_update_purgeable_status(shmem); > } > EXPORT_SYMBOL(drm_gem_shmem_vunmap); > > @@ -447,29 +842,140 @@ int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv) > > madv = shmem->madv; > > + drm_gem_shmem_update_pages_state_locked(shmem); > + > dma_resv_unlock(shmem->base.resv); > > return (madv >= 0); > } > EXPORT_SYMBOL(drm_gem_shmem_madvise); > > -void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem) > +/** > + * drm_gem_shmem_swap_in_pages_locked() - Moves shmem pages back to memory > + * @shmem: shmem GEM object > + * > + * This function moves pages back to memory if they were previously evicted > + * by the memory shrinker. > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_gem_shmem_swap_in_pages_locked(struct drm_gem_shmem_object *shmem) > +{ > + struct drm_gem_object *obj = &shmem->base; > + struct sg_table *sgt; > + int ret; > + > + lockdep_assert_held(&obj->resv->lock.base); > + > + if (shmem->evicted) { > + ret = drm_gem_shmem_acquire_pages_locked(shmem); > + if (ret) > + return ret; > + > + sgt = drm_gem_shmem_get_sg_table(shmem); > + if (IS_ERR(sgt)) > + return PTR_ERR(sgt); > + > + ret = dma_map_sgtable(obj->dev->dev, sgt, > + DMA_BIDIRECTIONAL, 0); > + if (ret) { > + sg_free_table(sgt); > + kfree(sgt); > + return ret; > + } > + > + shmem->sgt = sgt; > + shmem->evicted = false; > + shmem->pages_state = DRM_GEM_SHMEM_PAGES_STATE_PINNED; > + > + drm_gem_shmem_update_pages_state_locked(shmem); > + } > + > + return shmem->pages ? 0 : -ENOMEM; > +} > +EXPORT_SYMBOL_GPL(drm_gem_shmem_swap_in_pages_locked); > + > +/** > + * drm_gem_shmem_swap_in_locked() - Moves shmem GEM back to memory > + * @shmem: shmem GEM object > + * > + * This function moves shmem GEM back to memory if it was previously evicted > + * by the memory shrinker. The GEM is ready to use on success. > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_gem_shmem_swap_in_locked(struct drm_gem_shmem_object *shmem) > +{ > + struct drm_gem_object *obj = &shmem->base; > + > + lockdep_assert_held(&obj->resv->lock.base); > + > + if (shmem->evicted) > + return obj->funcs->swap_in(obj); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(drm_gem_shmem_swap_in_locked); > + > +static void drm_gem_shmem_unpin_pages_locked(struct drm_gem_shmem_object *shmem) > { > struct drm_gem_object *obj = &shmem->base; > struct drm_device *dev = obj->dev; > > - WARN_ON(!drm_gem_shmem_is_purgeable(shmem)); > + if (shmem->evicted) > + return; > > dma_unmap_sgtable(dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0); > + drm_gem_shmem_release_pages_locked(shmem); > + drm_vma_node_unmap(&obj->vma_node, dev->anon_inode->i_mapping); > + > sg_free_table(shmem->sgt); > kfree(shmem->sgt); > shmem->sgt = NULL; > +} > > - drm_gem_shmem_put_pages_locked(shmem); > +/** > + * drm_gem_shmem_evict_locked - Evict shmem pages > + * @shmem: shmem GEM object > + * > + * This function unpins shmem pages, allowing them to be swapped out from > + * memory. > + */ > +void drm_gem_shmem_evict_locked(struct drm_gem_shmem_object *shmem) > +{ > + struct drm_gem_object *obj = &shmem->base; > > - shmem->madv = -1; > + lockdep_assert_held(&obj->resv->lock.base); > > - drm_vma_node_unmap(&obj->vma_node, dev->anon_inode->i_mapping); > + WARN_ON(!drm_gem_shmem_is_evictable(shmem)); > + WARN_ON(shmem->madv < 0); > + WARN_ON(shmem->evicted); > + > + drm_gem_shmem_unpin_pages_locked(shmem); > + > + shmem->evicted = true; > + drm_gem_shmem_set_pages_state(shmem, DRM_GEM_SHMEM_PAGES_STATE_EVICTED); > +} > +EXPORT_SYMBOL_GPL(drm_gem_shmem_evict_locked); > + > +/** > + * drm_gem_shmem_purge_locked - Purge shmem pages > + * @shmem: shmem GEM object > + * > + * This function permanently releases shmem pages. > + */ > +void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem) > +{ > + struct drm_gem_object *obj = &shmem->base; > + > + lockdep_assert_held(&obj->resv->lock.base); > + > + WARN_ON(!drm_gem_shmem_is_purgeable(shmem)); > + WARN_ON(shmem->madv < 0); > + > + drm_gem_shmem_unpin_pages_locked(shmem); > drm_gem_free_mmap_offset(obj); > > /* Our goal here is to return as much of the memory as > @@ -480,6 +986,9 @@ void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem) > shmem_truncate_range(file_inode(obj->filp), 0, (loff_t)-1); > > invalidate_mapping_pages(file_inode(obj->filp)->i_mapping, 0, (loff_t)-1); > + > + shmem->madv = -1; > + drm_gem_shmem_set_pages_state(shmem, DRM_GEM_SHMEM_PAGES_STATE_PURGED); > } > EXPORT_SYMBOL(drm_gem_shmem_purge_locked); > > @@ -543,22 +1052,31 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf) > vm_fault_t ret; > struct page *page; > pgoff_t page_offset; > + bool pages_inactive; > + int err; > > /* We don't use vmf->pgoff since that has the fake offset */ > page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT; > > dma_resv_lock(shmem->base.resv, NULL); > > - if (page_offset >= num_pages || > - WARN_ON_ONCE(!shmem->pages) || > - shmem->madv < 0) { > + pages_inactive = shmem->pages_state < DRM_GEM_SHMEM_PAGES_STATE_PINNED; > + WARN_ON_ONCE(!shmem->pages ^ pages_inactive); > + > + if (page_offset >= num_pages || (!shmem->pages && !shmem->evicted)) { > ret = VM_FAULT_SIGBUS; > } else { > + err = drm_gem_shmem_swap_in_locked(shmem); > + if (err) { > + ret = VM_FAULT_OOM; > + goto unlock; > + } > + > page = shmem->pages[page_offset]; > > ret = vmf_insert_pfn(vma, vmf->address, page_to_pfn(page)); > } > - > +unlock: > dma_resv_unlock(shmem->base.resv); > > return ret; > @@ -568,13 +1086,8 @@ static void drm_gem_shmem_vm_open(struct vm_area_struct *vma) > { > struct drm_gem_object *obj = vma->vm_private_data; > struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > - int ret; > - > - WARN_ON(shmem->base.import_attach); > - > - ret = drm_gem_shmem_get_pages(shmem); > - WARN_ON_ONCE(ret != 0); > > + drm_gem_shmem_get_pages_no_fail(shmem); > drm_gem_vm_open(vma); > } > > @@ -716,6 +1229,8 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem) > > shmem->sgt = sgt; > > + drm_gem_shmem_update_pages_state(shmem); > + > return sgt; > > err_free_sgt: > @@ -762,6 +1277,202 @@ 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 = READ_ONCE(gem_shrinker->shrinkable_count); > + > + if (count >= SHRINK_EMPTY) > + return SHRINK_EMPTY - 1; > + > + return count ?: SHRINK_EMPTY; > +} > + > +static unsigned long > +drm_gem_shmem_shrinker_run_objects_scan(struct shrinker *shrinker, > + unsigned long nr_to_scan, > + bool *lock_contention, > + bool evict) > +{ > + struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker); > + struct drm_gem_shmem_object *shmem; > + struct list_head still_in_list; > + struct drm_gem_object *obj; > + unsigned long freed = 0; > + struct list_head *lru; > + size_t page_count; > + > + INIT_LIST_HEAD(&still_in_list); > + > + mutex_lock(&gem_shrinker->lock); > + > + if (evict) > + lru = &gem_shrinker->lru_evictable; > + else > + lru = &gem_shrinker->lru_purgeable; > + > + while (freed < nr_to_scan) { > + shmem = list_first_entry_or_null(lru, typeof(*shmem), madv_list); > + if (!shmem) > + break; > + > + obj = &shmem->base; > + page_count = obj->size >> PAGE_SHIFT; > + list_move_tail(&shmem->madv_list, &still_in_list); > + > + if (evict && get_nr_swap_pages() < page_count) > + continue; > + > + /* > + * 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)) { > + *lock_contention |= true; > + goto shrinker_lock; > + } > + > + /* prevent racing with the dma-buf exporting */ > + if (!mutex_trylock(&gem_shrinker->dev->object_name_lock)) { > + *lock_contention |= true; > + goto resv_unlock; > + } > + > + /* check whether h/w uses this object */ > + if (!dma_resv_test_signaled(obj->resv, DMA_RESV_USAGE_WRITE)) > + goto object_name_unlock; > + > + /* GEM may've become unpurgeable while shrinker was unlocked */ > + if (evict) { > + if (!drm_gem_shmem_is_evictable(shmem)) > + goto object_name_unlock; > + } else { > + if (!drm_gem_shmem_is_purgeable(shmem)) > + goto object_name_unlock; > + } > + > + if (evict) > + freed += obj->funcs->evict(obj); > + else > + freed += obj->funcs->purge(obj); > +object_name_unlock: > + mutex_unlock(&gem_shrinker->dev->object_name_lock); > +resv_unlock: > + dma_resv_unlock(obj->resv); > +shrinker_lock: > + drm_gem_object_put(&shmem->base); > + mutex_lock(&gem_shrinker->lock); > + } > + > + list_splice_tail(&still_in_list, lru); > + > + mutex_unlock(&gem_shrinker->lock); > + > + return freed; > +} > + > +static unsigned long > +drm_gem_shmem_shrinker_scan_objects(struct shrinker *shrinker, > + struct shrink_control *sc) > +{ > + unsigned long nr_to_scan = sc->nr_to_scan; > + bool lock_contention = false; > + unsigned long freed; > + > + /* purge as many objects as we can */ > + freed = drm_gem_shmem_shrinker_run_objects_scan(shrinker, nr_to_scan, > + &lock_contention, false); > + nr_to_scan -= freed; > + > + /* evict as many objects as we can */ > + if (freed < nr_to_scan) > + freed += drm_gem_shmem_shrinker_run_objects_scan(shrinker, > + nr_to_scan, > + &lock_contention, > + true); > + > + return (!freed && !lock_contention) ? SHRINK_STOP : freed; > +} > + > +/** > + * drm_gem_shmem_shrinker_register() - Register shmem shrinker > + * @dev: DRM device > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_gem_shmem_shrinker_register(struct drm_device *dev) > +{ > + struct drm_gem_shmem_shrinker *gem_shrinker; > + int err; > + > + if (WARN_ON(dev->shmem_shrinker)) > + return -EBUSY; > + > + gem_shrinker = kzalloc(sizeof(*gem_shrinker), GFP_KERNEL); > + if (!gem_shrinker) > + return -ENOMEM; > + > + gem_shrinker->base.count_objects = drm_gem_shmem_shrinker_count_objects; > + gem_shrinker->base.scan_objects = drm_gem_shmem_shrinker_scan_objects; > + gem_shrinker->base.seeks = DEFAULT_SEEKS; > + gem_shrinker->dev = dev; > + > + INIT_LIST_HEAD(&gem_shrinker->lru_purgeable); > + INIT_LIST_HEAD(&gem_shrinker->lru_evictable); > + INIT_LIST_HEAD(&gem_shrinker->lru_evicted); > + INIT_LIST_HEAD(&gem_shrinker->lru_active); > + mutex_init(&gem_shrinker->lock); > + > + dev->shmem_shrinker = gem_shrinker; > + > + err = register_shrinker(&gem_shrinker->base); > + if (err) { > + dev->shmem_shrinker = NULL; > + kfree(gem_shrinker); > + return err; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(drm_gem_shmem_shrinker_register); > + > +/** > + * drm_gem_shmem_shrinker_unregister() - Unregister shmem shrinker > + * @dev: DRM device > + */ > +void drm_gem_shmem_shrinker_unregister(struct drm_device *dev) > +{ > + struct drm_gem_shmem_shrinker *gem_shrinker = dev->shmem_shrinker; > + > + if (gem_shrinker) { > + unregister_shrinker(&gem_shrinker->base); > + WARN_ON(!list_empty(&gem_shrinker->lru_purgeable)); > + WARN_ON(!list_empty(&gem_shrinker->lru_evictable)); > + WARN_ON(!list_empty(&gem_shrinker->lru_evicted)); > + WARN_ON(!list_empty(&gem_shrinker->lru_active)); > + mutex_destroy(&gem_shrinker->lock); > + dev->shmem_shrinker = NULL; > + kfree(gem_shrinker); > + } > +} > +EXPORT_SYMBOL_GPL(drm_gem_shmem_shrinker_unregister); > + > MODULE_DESCRIPTION("DRM SHMEM memory-management helpers"); > MODULE_IMPORT_NS(DMA_BUF); > MODULE_LICENSE("GPL v2"); > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h > index 9923c7a6885e..929546cad894 100644 > --- a/include/drm/drm_device.h > +++ b/include/drm/drm_device.h > @@ -16,6 +16,7 @@ struct drm_vblank_crtc; > struct drm_vma_offset_manager; > struct drm_vram_mm; > struct drm_fb_helper; > +struct drm_gem_shmem_shrinker; > > struct inode; > > @@ -277,6 +278,9 @@ struct drm_device { > /** @vram_mm: VRAM MM memory manager */ > struct drm_vram_mm *vram_mm; > > + /** @shmem_shrinker: SHMEM GEM memory shrinker */ > + struct drm_gem_shmem_shrinker *shmem_shrinker; > + > /** > * @switch_power_state: > * > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > index 9d7c61a122dc..390d1ce08ed3 100644 > --- a/include/drm/drm_gem.h > +++ b/include/drm/drm_gem.h > @@ -172,6 +172,41 @@ struct drm_gem_object_funcs { > * This is optional but necessary for mmap support. > */ > const struct vm_operations_struct *vm_ops; > + > + /** > + * @purge: > + * > + * Releases the GEM object's allocated backing storage to the system. > + * > + * Returns the number of pages that have been freed by purging the GEM object. > + * > + * This callback is used by the GEM shrinker. > + */ > + unsigned long (*purge)(struct drm_gem_object *obj); > + > + /** > + * @evict: > + * > + * Unpins the GEM object's allocated backing storage, allowing shmem pages > + * to be swapped out. > + * > + * Returns the number of pages that have been unpinned. > + * > + * This callback is used by the GEM shrinker. > + */ > + unsigned long (*evict)(struct drm_gem_object *obj); > + > + /** > + * @swap_in: > + * > + * Pins GEM object's allocated backing storage if it was previously evicted, > + * moving swapped out pages back to memory. > + * > + * Returns 0 on success, or -errno on error. > + * > + * This callback is used by the GEM shrinker. > + */ > + int (*swap_in)(struct drm_gem_object *obj); > }; > > /** > diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h > index 70889533962a..a65557b446e6 100644 > --- a/include/drm/drm_gem_shmem_helper.h > +++ b/include/drm/drm_gem_shmem_helper.h > @@ -6,6 +6,7 @@ > #include <linux/fs.h> > #include <linux/mm.h> > #include <linux/mutex.h> > +#include <linux/shrinker.h> > > #include <drm/drm_file.h> > #include <drm/drm_gem.h> > @@ -15,8 +16,18 @@ > struct dma_buf_attachment; > struct drm_mode_create_dumb; > struct drm_printer; > +struct drm_device; > struct sg_table; > > +enum drm_gem_shmem_pages_state { > + DRM_GEM_SHMEM_PAGES_STATE_PURGED = -2, > + DRM_GEM_SHMEM_PAGES_STATE_EVICTED = -1, > + DRM_GEM_SHMEM_PAGES_STATE_UNPINNED = 0, > + DRM_GEM_SHMEM_PAGES_STATE_PINNED = 1, > + DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE = 2, > + DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE = 3, > +}; > + > /** > * struct drm_gem_shmem_object - GEM object backed by shmem > */ > @@ -43,8 +54,8 @@ struct drm_gem_shmem_object { > * @madv: State for madvise > * > * 0 is active/inuse. > + * 1 is not-needed/can-be-purged > * A negative value is the object is purged. > - * Positive values are driver specific and not used by the helpers. > */ > int madv; > > @@ -91,6 +102,40 @@ struct drm_gem_shmem_object { > * @map_wc: map object write-combined (instead of using shmem defaults). > */ > bool map_wc; > + > + /** > + * @eviction_disable_count: > + * > + * The shmem pages are disallowed to be evicted by the memory shrinker > + * while count is non-zero. Used internally by memory shrinker. > + */ > + unsigned int eviction_disable_count; > + > + /** > + * @purging_disable_count: > + * > + * The shmem pages are disallowed to be purged by the memory shrinker > + * while count is non-zero. Used internally by memory shrinker. > + */ > + unsigned int purging_disable_count; > + > + /** > + * @pages_state: Current state of shmem pages. Used internally by > + * memory shrinker. > + */ > + enum drm_gem_shmem_pages_state pages_state; > + > + /** > + * @evicted: True if shmem pages were evicted by the memory shrinker. > + * Used internally by memory shrinker. > + */ > + bool evicted; > + > + /** > + * @pages_shrinkable: True if shmem pages can be evicted or purged > + * by the memory shrinker. Used internally by memory shrinker. > + */ > + bool pages_shrinkable; > }; > > #define to_drm_gem_shmem_obj(obj) \ > @@ -111,15 +156,33 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct > > int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv); > > +int drm_gem_shmem_set_purgeable(struct drm_gem_shmem_object *shmem); > +int drm_gem_shmem_set_purgeable_and_evictable(struct drm_gem_shmem_object *shmem); > +int drm_gem_shmem_set_unpurgeable_and_unevictable(struct drm_gem_shmem_object *shmem); > + > +static inline bool drm_gem_shmem_is_evictable(struct drm_gem_shmem_object *shmem) > +{ > + return (shmem->madv >= 0) && !shmem->eviction_disable_count && > + shmem->base.funcs->evict && shmem->base.funcs->swap_in && > + !shmem->vmap_use_count && !shmem->base.dma_buf && > + !shmem->base.import_attach && shmem->sgt; > +} > + > static inline bool drm_gem_shmem_is_purgeable(struct drm_gem_shmem_object *shmem) > { > - return (shmem->madv > 0) && > - !shmem->vmap_use_count && shmem->sgt && > - !shmem->base.dma_buf && !shmem->base.import_attach; > + return (shmem->madv > 0) && !shmem->purging_disable_count && > + !shmem->vmap_use_count && shmem->base.funcs->purge && > + !shmem->base.dma_buf && !shmem->base.import_attach && > + shmem->sgt; > } > > -void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem); > +int drm_gem_shmem_swap_in_pages_locked(struct drm_gem_shmem_object *shmem); > +int drm_gem_shmem_swap_in_locked(struct drm_gem_shmem_object *shmem); > + > +void drm_gem_shmem_evict_locked(struct drm_gem_shmem_object *shmem); > + > bool drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem); > +void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem); > > struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_shmem_object *shmem); > struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem); > @@ -262,6 +325,38 @@ static inline int drm_gem_shmem_object_mmap(struct drm_gem_object *obj, struct v > return drm_gem_shmem_mmap(shmem, vma); > } > > +/** > + * struct drm_gem_shmem_shrinker - Generic memory shrinker for shmem GEMs > + */ > +struct drm_gem_shmem_shrinker { > + /** @base: Shrinker for purging shmem GEM objects */ > + struct shrinker base; > + > + /** @lock: Protects @lru_* */ > + struct mutex lock; > + > + /** @lru_purgeable: List of shmem GEM objects available for purging */ > + struct list_head lru_purgeable; > + > + /** @lru_active: List of active shmem GEM objects */ > + struct list_head lru_active; > + > + /** @lru_evictable: List of shmem GEM objects that can be evicted */ > + struct list_head lru_evictable; > + > + /** @lru_evicted: List of evicted shmem GEM objects */ > + struct list_head lru_evicted; > + > + /** @dev: DRM device that uses this shrinker */ > + struct drm_device *dev; > + > + /** @shrinkable_count: Count of shmem GEM pages to be purged and evicted */ > + u64 shrinkable_count; > +}; > + > +int drm_gem_shmem_shrinker_register(struct drm_device *dev); > +void drm_gem_shmem_shrinker_unregister(struct drm_device *dev); > + > /* > * Driver ops > */
On Thu, May 05, 2022 at 10:34:02AM +0200, Thomas Zimmermann wrote: > Hi > > Am 18.04.22 um 00:37 schrieb Dmitry Osipenko: > > Introduce a common DRM SHMEM shrinker. It allows to reduce code > > duplication among DRM drivers that implement theirs own shrinkers. > > This is initial version of the shrinker that covers basic needs of > > GPU drivers, both purging and eviction of shmem objects are supported. > > > > This patch is based on a couple ideas borrowed from Rob's Clark MSM > > shrinker and Thomas' Zimmermann variant of SHMEM shrinker. > > > > In order to start using DRM SHMEM shrinker drivers should: > > > > 1. Implement new purge(), evict() + swap_in() GEM callbacks. > > 2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device). > > 3. Use drm_gem_shmem_set_purgeable_and_evictable(shmem) and alike API > > functions to activate shrinking of GEMs. > > Honestly speaking, after reading the patch and the discussion here I really > don't like where all tis is going. The interfaces and implementation are > overengineered. Descisions about evicting and purging should be done by the > memory manager. For the most part, it's none of the driver's business. > > 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. We have a few shrinkers already all over, so extracting that does make sense I think. I do agree that there's probably a few more steps than necessary involved right now in all this for the helper<->driver interface. -Daniel > > Best regards > Thomas > > > > > Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com> > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > --- > > drivers/gpu/drm/drm_gem_shmem_helper.c | 765 ++++++++++++++++++++++++- > > include/drm/drm_device.h | 4 + > > include/drm/drm_gem.h | 35 ++ > > include/drm/drm_gem_shmem_helper.h | 105 +++- > > 4 files changed, 877 insertions(+), 32 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > > index 3ecef571eff3..3838fb8d6f3a 100644 > > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > > @@ -88,6 +88,13 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private) > > INIT_LIST_HEAD(&shmem->madv_list); > > + /* > > + * Eviction and purging are disabled by default, shmem user must enable > > + * them explicitly using drm_gem_shmem_set_evictable/purgeable(). > > + */ > > + shmem->eviction_disable_count = 1; > > + shmem->purging_disable_count = 1; > > + > > if (!private) { > > /* > > * Our buffers are kept pinned, so allocating them > > @@ -126,6 +133,107 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t > > } > > EXPORT_SYMBOL_GPL(drm_gem_shmem_create); > > +static void > > +drm_gem_shmem_add_pages_to_shrinker(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 (!shmem->pages_shrinkable) { > > + WARN_ON(gem_shrinker->shrinkable_count + page_count < page_count); > > + gem_shrinker->shrinkable_count += page_count; > > + shmem->pages_shrinkable = true; > > + } > > +} > > + > > +static void > > +drm_gem_shmem_remove_pages_from_shrinker(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 (shmem->pages_shrinkable) { > > + WARN_ON(gem_shrinker->shrinkable_count < page_count); > > + gem_shrinker->shrinkable_count -= page_count; > > + shmem->pages_shrinkable = false; > > + } > > +} > > + > > +static void > > +drm_gem_shmem_set_pages_state_locked(struct drm_gem_shmem_object *shmem, > > + enum drm_gem_shmem_pages_state new_state) > > +{ > > + struct drm_gem_object *obj = &shmem->base; > > + struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker; > > + > > + lockdep_assert_held(&gem_shrinker->lock); > > + lockdep_assert_held(&obj->resv->lock.base); > > + > > + if (new_state >= DRM_GEM_SHMEM_PAGES_STATE_PINNED) { > > + if (drm_gem_shmem_is_evictable(shmem)) > > + new_state = DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE; > > + > > + if (drm_gem_shmem_is_purgeable(shmem)) > > + new_state = DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE; > > + > > + if (!shmem->pages) > > + new_state = DRM_GEM_SHMEM_PAGES_STATE_UNPINNED; > > + > > + if (shmem->evicted) > > + new_state = DRM_GEM_SHMEM_PAGES_STATE_EVICTED; > > + } > > + > > + if (shmem->pages_state == new_state) > > + return; > > + > > + switch (new_state) { > > + case DRM_GEM_SHMEM_PAGES_STATE_UNPINNED: > > + case DRM_GEM_SHMEM_PAGES_STATE_PURGED: > > + drm_gem_shmem_remove_pages_from_shrinker(shmem); > > + list_del_init(&shmem->madv_list); > > + break; > > + > > + case DRM_GEM_SHMEM_PAGES_STATE_PINNED: > > + drm_gem_shmem_remove_pages_from_shrinker(shmem); > > + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_active); > > + break; > > + > > + case DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE: > > + drm_gem_shmem_add_pages_to_shrinker(shmem); > > + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_purgeable); > > + break; > > + > > + case DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE: > > + drm_gem_shmem_add_pages_to_shrinker(shmem); > > + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_evictable); > > + break; > > + > > + case DRM_GEM_SHMEM_PAGES_STATE_EVICTED: > > + drm_gem_shmem_remove_pages_from_shrinker(shmem); > > + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_evicted); > > + break; > > + } > > + > > + shmem->pages_state = new_state; > > +} > > + > > +static void > > +drm_gem_shmem_set_pages_state(struct drm_gem_shmem_object *shmem, > > + enum drm_gem_shmem_pages_state new_state) > > +{ > > + struct drm_gem_object *obj = &shmem->base; > > + struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker; > > + > > + if (!gem_shrinker) > > + return; > > + > > + mutex_lock(&gem_shrinker->lock); > > + drm_gem_shmem_set_pages_state_locked(shmem, new_state); > > + mutex_unlock(&gem_shrinker->lock); > > +} > > + > > /** > > * drm_gem_shmem_free - Free resources associated with a shmem GEM object > > * @shmem: shmem GEM object to free > > @@ -137,6 +245,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, -1); > > + > > WARN_ON(shmem->vmap_use_count); > > if (obj->import_attach) { > > @@ -148,7 +259,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > > sg_free_table(shmem->sgt); > > kfree(shmem->sgt); > > } > > - if (shmem->pages) > > + if (shmem->pages_use_count) > > drm_gem_shmem_put_pages(shmem); > > } > > @@ -159,18 +270,226 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > > } > > EXPORT_SYMBOL_GPL(drm_gem_shmem_free); > > -static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem) > > +static void drm_gem_shmem_update_pages_state_locked(struct drm_gem_shmem_object *shmem) > > +{ > > + struct drm_gem_object *obj = &shmem->base; > > + struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker; > > + enum drm_gem_shmem_pages_state new_state; > > + > > + if (!gem_shrinker || obj->import_attach) > > + return; > > + > > + mutex_lock(&gem_shrinker->lock); > > + > > + if (!shmem->madv) > > + new_state = DRM_GEM_SHMEM_PAGES_STATE_PINNED; > > + else if (shmem->madv > 0) > > + new_state = DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE; > > + else > > + new_state = DRM_GEM_SHMEM_PAGES_STATE_PURGED; > > + > > + drm_gem_shmem_set_pages_state_locked(shmem, new_state); > > + > > + mutex_unlock(&gem_shrinker->lock); > > +} > > + > > +static void drm_gem_shmem_update_pages_state(struct drm_gem_shmem_object *shmem) > > +{ > > + dma_resv_lock(shmem->base.resv, NULL); > > + drm_gem_shmem_update_pages_state_locked(shmem); > > + dma_resv_unlock(shmem->base.resv); > > +} > > + > > +static int > > +drm_gem_shmem_set_evictable_locked(struct drm_gem_shmem_object *shmem) > > +{ > > + int ret = 0; > > + > > + WARN_ON_ONCE(!shmem->eviction_disable_count--); > > + > > + if (shmem->madv < 0) > > + ret = -ENOMEM; > > + > > + drm_gem_shmem_update_pages_state_locked(shmem); > > + > > + return ret; > > +} > > + > > +static int > > +drm_gem_shmem_set_unevictable_locked(struct drm_gem_shmem_object *shmem) > > +{ > > + struct drm_gem_object *obj = &shmem->base; > > + int err; > > + > > + if (shmem->madv < 0) > > + return -ENOMEM; > > + > > + if (shmem->evicted) { > > + err = obj->funcs->swap_in(obj); > > + if (err) > > + return err; > > + } > > + > > + shmem->eviction_disable_count++; > > + > > + drm_gem_shmem_update_pages_state_locked(shmem); > > + > > + return 0; > > +} > > + > > +static int > > +drm_gem_shmem_set_purgeable_locked(struct drm_gem_shmem_object *shmem) > > +{ > > + int ret = 0; > > + > > + WARN_ON_ONCE(!shmem->purging_disable_count--); > > + > > + if (shmem->madv < 0) > > + ret = -ENOMEM; > > + > > + drm_gem_shmem_update_pages_state_locked(shmem); > > + > > + return ret; > > +} > > + > > +/** > > + * drm_gem_shmem_set_purgeable() - Make GEM purgeable by memory shrinker > > + * @shmem: shmem GEM object > > + * > > + * Tell memory shrinker that this GEM can be purged. Initially purging is > > + * disabled for all GEMs. Each set_pureable() call must have corresponding > > + * set_unpureable() call. If GEM was purged, then -ENOMEM is returned. > > + * > > + * Returns: > > + * 0 on success or a negative error code on failure. > > + */ > > +int drm_gem_shmem_set_purgeable(struct drm_gem_shmem_object *shmem) > > +{ > > + int ret; > > + > > + dma_resv_lock(shmem->base.resv, NULL); > > + ret = drm_gem_shmem_set_purgeable_locked(shmem); > > + dma_resv_unlock(shmem->base.resv); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(drm_gem_shmem_set_purgeable); > > + > > +static int > > +drm_gem_shmem_set_unpurgeable_locked(struct drm_gem_shmem_object *shmem) > > +{ > > + if (shmem->madv < 0) > > + return -ENOMEM; > > + > > + shmem->purging_disable_count++; > > + > > + drm_gem_shmem_update_pages_state_locked(shmem); > > + > > + return 0; > > +} > > + > > +static int > > +drm_gem_shmem_set_purgeable_and_evictable_locked(struct drm_gem_shmem_object *shmem) > > +{ > > + int ret; > > + > > + ret = drm_gem_shmem_set_evictable_locked(shmem); > > + if (!ret) { > > + ret = drm_gem_shmem_set_purgeable_locked(shmem); > > + if (ret) > > + drm_gem_shmem_set_unevictable_locked(shmem); > > + } > > + > > + return ret; > > +} > > + > > +static int > > +drm_gem_shmem_set_unpurgeable_and_unevictable_locked(struct drm_gem_shmem_object *shmem) > > +{ > > + int ret; > > + > > + ret = drm_gem_shmem_set_unpurgeable_locked(shmem); > > + if (!ret) { > > + ret = drm_gem_shmem_set_unevictable_locked(shmem); > > + if (ret) > > + drm_gem_shmem_set_purgeable_locked(shmem); > > + } > > + > > + return ret; > > +} > > + > > +/** > > + * drm_gem_shmem_set_purgeable_and_evictable() - Make GEM unpurgeable and > > + * unevictable by memory shrinker > > + * @shmem: shmem GEM object > > + * > > + * Tell memory shrinker that this GEM can't be purged and evicted. Each > > + * set_purgeable_and_evictable() call must have corresponding > > + * unpurgeable_and_unevictable() call. If GEM was purged, then -ENOMEM > > + * is returned. > > + * > > + * Returns: > > + * 0 on success or a negative error code on failure. > > + */ > > +int drm_gem_shmem_set_purgeable_and_evictable(struct drm_gem_shmem_object *shmem) > > +{ > > + int ret; > > + > > + dma_resv_lock(shmem->base.resv, NULL); > > + ret = drm_gem_shmem_set_purgeable_and_evictable_locked(shmem); > > + dma_resv_unlock(shmem->base.resv); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(drm_gem_shmem_set_purgeable_and_evictable); > > + > > +/** > > + * drm_gem_shmem_set_unpurgeable_and_unevictable() - Make GEM purgeable and > > + * evictable by memory shrinker > > + * @shmem: shmem GEM object > > + * > > + * Tell memory shrinker that this GEM can be purged and evicted. Each > > + * unpurgeable_and_unevictable() call must have corresponding > > + * set_purgeable_and_evictable() call. If GEM was purged, then -ENOMEM > > + * is returned. > > + * > > + * Returns: > > + * 0 on success or a negative error code on failure. > > + */ > > +int drm_gem_shmem_set_unpurgeable_and_unevictable(struct drm_gem_shmem_object *shmem) > > +{ > > + int ret; > > + > > + ret = dma_resv_lock_interruptible(shmem->base.resv, NULL); > > + if (ret) > > + return ret; > > + > > + ret = drm_gem_shmem_set_unpurgeable_and_unevictable_locked(shmem); > > + dma_resv_unlock(shmem->base.resv); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(drm_gem_shmem_set_unpurgeable_and_unevictable); > > + > > +static int > > +drm_gem_shmem_acquire_pages_locked(struct drm_gem_shmem_object *shmem) > > { > > struct drm_gem_object *obj = &shmem->base; > > struct page **pages; > > - if (shmem->pages_use_count++ > 0) > > + if (shmem->madv < 0) { > > + WARN_ON(shmem->pages); > > + return -ENOMEM; > > + } > > + > > + if (shmem->pages) { > > + WARN_ON(!shmem->evicted); > > return 0; > > + } > > pages = drm_gem_get_pages(obj); > > if (IS_ERR(pages)) { > > DRM_DEBUG_KMS("Failed to get pages (%ld)\n", PTR_ERR(pages)); > > - shmem->pages_use_count = 0; > > return PTR_ERR(pages); > > } > > @@ -189,6 +508,25 @@ static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem) > > return 0; > > } > > +static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem) > > +{ > > + int err; > > + > > + if (shmem->madv < 0) > > + return -ENOMEM; > > + > > + if (shmem->pages_use_count++ > 0) > > + return 0; > > + > > + err = drm_gem_shmem_acquire_pages_locked(shmem); > > + if (err) { > > + shmem->pages_use_count = 0; > > + return err; > > + } > > + > > + return 0; > > +} > > + > > /* > > * drm_gem_shmem_get_pages - Allocate backing pages for a shmem GEM object > > * @shmem: shmem GEM object > > @@ -209,21 +547,38 @@ int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem) > > if (ret) > > return ret; > > ret = drm_gem_shmem_get_pages_locked(shmem); > > + > > + drm_gem_shmem_update_pages_state_locked(shmem); > > + > > dma_resv_unlock(shmem->base.resv); > > return ret; > > } > > EXPORT_SYMBOL(drm_gem_shmem_get_pages); > > -static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) > > +static void drm_gem_shmem_get_pages_no_fail(struct drm_gem_shmem_object *shmem) > > { > > - struct drm_gem_object *obj = &shmem->base; > > + WARN_ON(shmem->base.import_attach); > > - if (WARN_ON_ONCE(!shmem->pages_use_count)) > > - return; > > + dma_resv_lock(shmem->base.resv, NULL); > > - if (--shmem->pages_use_count > 0) > > + if (drm_gem_shmem_get_pages_locked(shmem)) > > + shmem->pages_use_count++; > > + > > + drm_gem_shmem_update_pages_state_locked(shmem); > > + > > + dma_resv_unlock(shmem->base.resv); > > +} > > + > > +static void > > +drm_gem_shmem_release_pages_locked(struct drm_gem_shmem_object *shmem) > > +{ > > + struct drm_gem_object *obj = &shmem->base; > > + > > + if (!shmem->pages) { > > + WARN_ON(!shmem->evicted && shmem->madv >= 0); > > return; > > + } > > #ifdef CONFIG_X86 > > if (shmem->map_wc) > > @@ -236,6 +591,21 @@ static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) > > shmem->pages = NULL; > > } > > +static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) > > +{ > > + struct drm_gem_object *obj = &shmem->base; > > + > > + lockdep_assert_held(&obj->resv->lock.base); > > + > > + if (WARN_ON(!shmem->pages_use_count)) > > + return; > > + > > + if (--shmem->pages_use_count > 0) > > + return; > > + > > + drm_gem_shmem_release_pages_locked(shmem); > > +} > > + > > /* > > * drm_gem_shmem_put_pages - Decrease use count on the backing pages for a shmem GEM object > > * @shmem: shmem GEM object > > @@ -246,6 +616,7 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem) > > { > > dma_resv_lock(shmem->base.resv, NULL); > > drm_gem_shmem_put_pages_locked(shmem); > > + drm_gem_shmem_update_pages_state_locked(shmem); > > dma_resv_unlock(shmem->base.resv); > > } > > EXPORT_SYMBOL(drm_gem_shmem_put_pages); > > @@ -262,9 +633,21 @@ EXPORT_SYMBOL(drm_gem_shmem_put_pages); > > */ > > int drm_gem_shmem_pin(struct drm_gem_shmem_object *shmem) > > { > > + int err; > > + > > WARN_ON(shmem->base.import_attach); > > - return drm_gem_shmem_get_pages(shmem); > > + err = drm_gem_shmem_set_unpurgeable_and_unevictable(shmem); > > + if (err) > > + return err; > > + > > + err = drm_gem_shmem_get_pages(shmem); > > + if (err) { > > + drm_gem_shmem_set_purgeable_and_evictable(shmem); > > + return err; > > + } > > + > > + return 0; > > } > > EXPORT_SYMBOL(drm_gem_shmem_pin); > > @@ -280,6 +663,7 @@ void drm_gem_shmem_unpin(struct drm_gem_shmem_object *shmem) > > WARN_ON(shmem->base.import_attach); > > drm_gem_shmem_put_pages(shmem); > > + drm_gem_shmem_set_purgeable_and_evictable(shmem); > > } > > EXPORT_SYMBOL(drm_gem_shmem_unpin); > > @@ -359,7 +743,18 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem, > > ret = dma_resv_lock_interruptible(shmem->base.resv, NULL); > > if (ret) > > return ret; > > + > > + ret = drm_gem_shmem_set_unpurgeable_and_unevictable_locked(shmem); > > + if (ret) > > + goto unlock; > > + > > ret = drm_gem_shmem_vmap_locked(shmem, map); > > + if (ret) > > + drm_gem_shmem_set_purgeable_and_evictable_locked(shmem); > > + else > > + drm_gem_shmem_update_pages_state_locked(shmem); > > + > > +unlock: > > dma_resv_unlock(shmem->base.resv); > > return ret; > > @@ -404,9 +799,9 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem, > > { > > dma_resv_lock(shmem->base.resv, NULL); > > drm_gem_shmem_vunmap_locked(shmem, map); > > + drm_gem_shmem_update_pages_state_locked(shmem); > > + drm_gem_shmem_set_purgeable_and_evictable_locked(shmem); > > dma_resv_unlock(shmem->base.resv); > > - > > - drm_gem_shmem_update_purgeable_status(shmem); > > } > > EXPORT_SYMBOL(drm_gem_shmem_vunmap); > > @@ -447,29 +842,140 @@ int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv) > > madv = shmem->madv; > > + drm_gem_shmem_update_pages_state_locked(shmem); > > + > > dma_resv_unlock(shmem->base.resv); > > return (madv >= 0); > > } > > EXPORT_SYMBOL(drm_gem_shmem_madvise); > > -void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem) > > +/** > > + * drm_gem_shmem_swap_in_pages_locked() - Moves shmem pages back to memory > > + * @shmem: shmem GEM object > > + * > > + * This function moves pages back to memory if they were previously evicted > > + * by the memory shrinker. > > + * > > + * Returns: > > + * 0 on success or a negative error code on failure. > > + */ > > +int drm_gem_shmem_swap_in_pages_locked(struct drm_gem_shmem_object *shmem) > > +{ > > + struct drm_gem_object *obj = &shmem->base; > > + struct sg_table *sgt; > > + int ret; > > + > > + lockdep_assert_held(&obj->resv->lock.base); > > + > > + if (shmem->evicted) { > > + ret = drm_gem_shmem_acquire_pages_locked(shmem); > > + if (ret) > > + return ret; > > + > > + sgt = drm_gem_shmem_get_sg_table(shmem); > > + if (IS_ERR(sgt)) > > + return PTR_ERR(sgt); > > + > > + ret = dma_map_sgtable(obj->dev->dev, sgt, > > + DMA_BIDIRECTIONAL, 0); > > + if (ret) { > > + sg_free_table(sgt); > > + kfree(sgt); > > + return ret; > > + } > > + > > + shmem->sgt = sgt; > > + shmem->evicted = false; > > + shmem->pages_state = DRM_GEM_SHMEM_PAGES_STATE_PINNED; > > + > > + drm_gem_shmem_update_pages_state_locked(shmem); > > + } > > + > > + return shmem->pages ? 0 : -ENOMEM; > > +} > > +EXPORT_SYMBOL_GPL(drm_gem_shmem_swap_in_pages_locked); > > + > > +/** > > + * drm_gem_shmem_swap_in_locked() - Moves shmem GEM back to memory > > + * @shmem: shmem GEM object > > + * > > + * This function moves shmem GEM back to memory if it was previously evicted > > + * by the memory shrinker. The GEM is ready to use on success. > > + * > > + * Returns: > > + * 0 on success or a negative error code on failure. > > + */ > > +int drm_gem_shmem_swap_in_locked(struct drm_gem_shmem_object *shmem) > > +{ > > + struct drm_gem_object *obj = &shmem->base; > > + > > + lockdep_assert_held(&obj->resv->lock.base); > > + > > + if (shmem->evicted) > > + return obj->funcs->swap_in(obj); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(drm_gem_shmem_swap_in_locked); > > + > > +static void drm_gem_shmem_unpin_pages_locked(struct drm_gem_shmem_object *shmem) > > { > > struct drm_gem_object *obj = &shmem->base; > > struct drm_device *dev = obj->dev; > > - WARN_ON(!drm_gem_shmem_is_purgeable(shmem)); > > + if (shmem->evicted) > > + return; > > dma_unmap_sgtable(dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0); > > + drm_gem_shmem_release_pages_locked(shmem); > > + drm_vma_node_unmap(&obj->vma_node, dev->anon_inode->i_mapping); > > + > > sg_free_table(shmem->sgt); > > kfree(shmem->sgt); > > shmem->sgt = NULL; > > +} > > - drm_gem_shmem_put_pages_locked(shmem); > > +/** > > + * drm_gem_shmem_evict_locked - Evict shmem pages > > + * @shmem: shmem GEM object > > + * > > + * This function unpins shmem pages, allowing them to be swapped out from > > + * memory. > > + */ > > +void drm_gem_shmem_evict_locked(struct drm_gem_shmem_object *shmem) > > +{ > > + struct drm_gem_object *obj = &shmem->base; > > - shmem->madv = -1; > > + lockdep_assert_held(&obj->resv->lock.base); > > - drm_vma_node_unmap(&obj->vma_node, dev->anon_inode->i_mapping); > > + WARN_ON(!drm_gem_shmem_is_evictable(shmem)); > > + WARN_ON(shmem->madv < 0); > > + WARN_ON(shmem->evicted); > > + > > + drm_gem_shmem_unpin_pages_locked(shmem); > > + > > + shmem->evicted = true; > > + drm_gem_shmem_set_pages_state(shmem, DRM_GEM_SHMEM_PAGES_STATE_EVICTED); > > +} > > +EXPORT_SYMBOL_GPL(drm_gem_shmem_evict_locked); > > + > > +/** > > + * drm_gem_shmem_purge_locked - Purge shmem pages > > + * @shmem: shmem GEM object > > + * > > + * This function permanently releases shmem pages. > > + */ > > +void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem) > > +{ > > + struct drm_gem_object *obj = &shmem->base; > > + > > + lockdep_assert_held(&obj->resv->lock.base); > > + > > + WARN_ON(!drm_gem_shmem_is_purgeable(shmem)); > > + WARN_ON(shmem->madv < 0); > > + > > + drm_gem_shmem_unpin_pages_locked(shmem); > > drm_gem_free_mmap_offset(obj); > > /* Our goal here is to return as much of the memory as > > @@ -480,6 +986,9 @@ void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem) > > shmem_truncate_range(file_inode(obj->filp), 0, (loff_t)-1); > > invalidate_mapping_pages(file_inode(obj->filp)->i_mapping, 0, (loff_t)-1); > > + > > + shmem->madv = -1; > > + drm_gem_shmem_set_pages_state(shmem, DRM_GEM_SHMEM_PAGES_STATE_PURGED); > > } > > EXPORT_SYMBOL(drm_gem_shmem_purge_locked); > > @@ -543,22 +1052,31 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf) > > vm_fault_t ret; > > struct page *page; > > pgoff_t page_offset; > > + bool pages_inactive; > > + int err; > > /* We don't use vmf->pgoff since that has the fake offset */ > > page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT; > > dma_resv_lock(shmem->base.resv, NULL); > > - if (page_offset >= num_pages || > > - WARN_ON_ONCE(!shmem->pages) || > > - shmem->madv < 0) { > > + pages_inactive = shmem->pages_state < DRM_GEM_SHMEM_PAGES_STATE_PINNED; > > + WARN_ON_ONCE(!shmem->pages ^ pages_inactive); > > + > > + if (page_offset >= num_pages || (!shmem->pages && !shmem->evicted)) { > > ret = VM_FAULT_SIGBUS; > > } else { > > + err = drm_gem_shmem_swap_in_locked(shmem); > > + if (err) { > > + ret = VM_FAULT_OOM; > > + goto unlock; > > + } > > + > > page = shmem->pages[page_offset]; > > ret = vmf_insert_pfn(vma, vmf->address, page_to_pfn(page)); > > } > > - > > +unlock: > > dma_resv_unlock(shmem->base.resv); > > return ret; > > @@ -568,13 +1086,8 @@ static void drm_gem_shmem_vm_open(struct vm_area_struct *vma) > > { > > struct drm_gem_object *obj = vma->vm_private_data; > > struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > > - int ret; > > - > > - WARN_ON(shmem->base.import_attach); > > - > > - ret = drm_gem_shmem_get_pages(shmem); > > - WARN_ON_ONCE(ret != 0); > > + drm_gem_shmem_get_pages_no_fail(shmem); > > drm_gem_vm_open(vma); > > } > > @@ -716,6 +1229,8 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem) > > shmem->sgt = sgt; > > + drm_gem_shmem_update_pages_state(shmem); > > + > > return sgt; > > err_free_sgt: > > @@ -762,6 +1277,202 @@ 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 = READ_ONCE(gem_shrinker->shrinkable_count); > > + > > + if (count >= SHRINK_EMPTY) > > + return SHRINK_EMPTY - 1; > > + > > + return count ?: SHRINK_EMPTY; > > +} > > + > > +static unsigned long > > +drm_gem_shmem_shrinker_run_objects_scan(struct shrinker *shrinker, > > + unsigned long nr_to_scan, > > + bool *lock_contention, > > + bool evict) > > +{ > > + struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker); > > + struct drm_gem_shmem_object *shmem; > > + struct list_head still_in_list; > > + struct drm_gem_object *obj; > > + unsigned long freed = 0; > > + struct list_head *lru; > > + size_t page_count; > > + > > + INIT_LIST_HEAD(&still_in_list); > > + > > + mutex_lock(&gem_shrinker->lock); > > + > > + if (evict) > > + lru = &gem_shrinker->lru_evictable; > > + else > > + lru = &gem_shrinker->lru_purgeable; > > + > > + while (freed < nr_to_scan) { > > + shmem = list_first_entry_or_null(lru, typeof(*shmem), madv_list); > > + if (!shmem) > > + break; > > + > > + obj = &shmem->base; > > + page_count = obj->size >> PAGE_SHIFT; > > + list_move_tail(&shmem->madv_list, &still_in_list); > > + > > + if (evict && get_nr_swap_pages() < page_count) > > + continue; > > + > > + /* > > + * 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)) { > > + *lock_contention |= true; > > + goto shrinker_lock; > > + } > > + > > + /* prevent racing with the dma-buf exporting */ > > + if (!mutex_trylock(&gem_shrinker->dev->object_name_lock)) { > > + *lock_contention |= true; > > + goto resv_unlock; > > + } > > + > > + /* check whether h/w uses this object */ > > + if (!dma_resv_test_signaled(obj->resv, DMA_RESV_USAGE_WRITE)) > > + goto object_name_unlock; > > + > > + /* GEM may've become unpurgeable while shrinker was unlocked */ > > + if (evict) { > > + if (!drm_gem_shmem_is_evictable(shmem)) > > + goto object_name_unlock; > > + } else { > > + if (!drm_gem_shmem_is_purgeable(shmem)) > > + goto object_name_unlock; > > + } > > + > > + if (evict) > > + freed += obj->funcs->evict(obj); > > + else > > + freed += obj->funcs->purge(obj); > > +object_name_unlock: > > + mutex_unlock(&gem_shrinker->dev->object_name_lock); > > +resv_unlock: > > + dma_resv_unlock(obj->resv); > > +shrinker_lock: > > + drm_gem_object_put(&shmem->base); > > + mutex_lock(&gem_shrinker->lock); > > + } > > + > > + list_splice_tail(&still_in_list, lru); > > + > > + mutex_unlock(&gem_shrinker->lock); > > + > > + return freed; > > +} > > + > > +static unsigned long > > +drm_gem_shmem_shrinker_scan_objects(struct shrinker *shrinker, > > + struct shrink_control *sc) > > +{ > > + unsigned long nr_to_scan = sc->nr_to_scan; > > + bool lock_contention = false; > > + unsigned long freed; > > + > > + /* purge as many objects as we can */ > > + freed = drm_gem_shmem_shrinker_run_objects_scan(shrinker, nr_to_scan, > > + &lock_contention, false); > > + nr_to_scan -= freed; > > + > > + /* evict as many objects as we can */ > > + if (freed < nr_to_scan) > > + freed += drm_gem_shmem_shrinker_run_objects_scan(shrinker, > > + nr_to_scan, > > + &lock_contention, > > + true); > > + > > + return (!freed && !lock_contention) ? SHRINK_STOP : freed; > > +} > > + > > +/** > > + * drm_gem_shmem_shrinker_register() - Register shmem shrinker > > + * @dev: DRM device > > + * > > + * Returns: > > + * 0 on success or a negative error code on failure. > > + */ > > +int drm_gem_shmem_shrinker_register(struct drm_device *dev) > > +{ > > + struct drm_gem_shmem_shrinker *gem_shrinker; > > + int err; > > + > > + if (WARN_ON(dev->shmem_shrinker)) > > + return -EBUSY; > > + > > + gem_shrinker = kzalloc(sizeof(*gem_shrinker), GFP_KERNEL); > > + if (!gem_shrinker) > > + return -ENOMEM; > > + > > + gem_shrinker->base.count_objects = drm_gem_shmem_shrinker_count_objects; > > + gem_shrinker->base.scan_objects = drm_gem_shmem_shrinker_scan_objects; > > + gem_shrinker->base.seeks = DEFAULT_SEEKS; > > + gem_shrinker->dev = dev; > > + > > + INIT_LIST_HEAD(&gem_shrinker->lru_purgeable); > > + INIT_LIST_HEAD(&gem_shrinker->lru_evictable); > > + INIT_LIST_HEAD(&gem_shrinker->lru_evicted); > > + INIT_LIST_HEAD(&gem_shrinker->lru_active); > > + mutex_init(&gem_shrinker->lock); > > + > > + dev->shmem_shrinker = gem_shrinker; > > + > > + err = register_shrinker(&gem_shrinker->base); > > + if (err) { > > + dev->shmem_shrinker = NULL; > > + kfree(gem_shrinker); > > + return err; > > + } > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(drm_gem_shmem_shrinker_register); > > + > > +/** > > + * drm_gem_shmem_shrinker_unregister() - Unregister shmem shrinker > > + * @dev: DRM device > > + */ > > +void drm_gem_shmem_shrinker_unregister(struct drm_device *dev) > > +{ > > + struct drm_gem_shmem_shrinker *gem_shrinker = dev->shmem_shrinker; > > + > > + if (gem_shrinker) { > > + unregister_shrinker(&gem_shrinker->base); > > + WARN_ON(!list_empty(&gem_shrinker->lru_purgeable)); > > + WARN_ON(!list_empty(&gem_shrinker->lru_evictable)); > > + WARN_ON(!list_empty(&gem_shrinker->lru_evicted)); > > + WARN_ON(!list_empty(&gem_shrinker->lru_active)); > > + mutex_destroy(&gem_shrinker->lock); > > + dev->shmem_shrinker = NULL; > > + kfree(gem_shrinker); > > + } > > +} > > +EXPORT_SYMBOL_GPL(drm_gem_shmem_shrinker_unregister); > > + > > MODULE_DESCRIPTION("DRM SHMEM memory-management helpers"); > > MODULE_IMPORT_NS(DMA_BUF); > > MODULE_LICENSE("GPL v2"); > > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h > > index 9923c7a6885e..929546cad894 100644 > > --- a/include/drm/drm_device.h > > +++ b/include/drm/drm_device.h > > @@ -16,6 +16,7 @@ struct drm_vblank_crtc; > > struct drm_vma_offset_manager; > > struct drm_vram_mm; > > struct drm_fb_helper; > > +struct drm_gem_shmem_shrinker; > > struct inode; > > @@ -277,6 +278,9 @@ struct drm_device { > > /** @vram_mm: VRAM MM memory manager */ > > struct drm_vram_mm *vram_mm; > > + /** @shmem_shrinker: SHMEM GEM memory shrinker */ > > + struct drm_gem_shmem_shrinker *shmem_shrinker; > > + > > /** > > * @switch_power_state: > > * > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > > index 9d7c61a122dc..390d1ce08ed3 100644 > > --- a/include/drm/drm_gem.h > > +++ b/include/drm/drm_gem.h > > @@ -172,6 +172,41 @@ struct drm_gem_object_funcs { > > * This is optional but necessary for mmap support. > > */ > > const struct vm_operations_struct *vm_ops; > > + > > + /** > > + * @purge: > > + * > > + * Releases the GEM object's allocated backing storage to the system. > > + * > > + * Returns the number of pages that have been freed by purging the GEM object. > > + * > > + * This callback is used by the GEM shrinker. > > + */ > > + unsigned long (*purge)(struct drm_gem_object *obj); > > + > > + /** > > + * @evict: > > + * > > + * Unpins the GEM object's allocated backing storage, allowing shmem pages > > + * to be swapped out. > > + * > > + * Returns the number of pages that have been unpinned. > > + * > > + * This callback is used by the GEM shrinker. > > + */ > > + unsigned long (*evict)(struct drm_gem_object *obj); > > + > > + /** > > + * @swap_in: > > + * > > + * Pins GEM object's allocated backing storage if it was previously evicted, > > + * moving swapped out pages back to memory. > > + * > > + * Returns 0 on success, or -errno on error. > > + * > > + * This callback is used by the GEM shrinker. > > + */ > > + int (*swap_in)(struct drm_gem_object *obj); > > }; > > /** > > diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h > > index 70889533962a..a65557b446e6 100644 > > --- a/include/drm/drm_gem_shmem_helper.h > > +++ b/include/drm/drm_gem_shmem_helper.h > > @@ -6,6 +6,7 @@ > > #include <linux/fs.h> > > #include <linux/mm.h> > > #include <linux/mutex.h> > > +#include <linux/shrinker.h> > > #include <drm/drm_file.h> > > #include <drm/drm_gem.h> > > @@ -15,8 +16,18 @@ > > struct dma_buf_attachment; > > struct drm_mode_create_dumb; > > struct drm_printer; > > +struct drm_device; > > struct sg_table; > > +enum drm_gem_shmem_pages_state { > > + DRM_GEM_SHMEM_PAGES_STATE_PURGED = -2, > > + DRM_GEM_SHMEM_PAGES_STATE_EVICTED = -1, > > + DRM_GEM_SHMEM_PAGES_STATE_UNPINNED = 0, > > + DRM_GEM_SHMEM_PAGES_STATE_PINNED = 1, > > + DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE = 2, > > + DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE = 3, > > +}; > > + > > /** > > * struct drm_gem_shmem_object - GEM object backed by shmem > > */ > > @@ -43,8 +54,8 @@ struct drm_gem_shmem_object { > > * @madv: State for madvise > > * > > * 0 is active/inuse. > > + * 1 is not-needed/can-be-purged > > * A negative value is the object is purged. > > - * Positive values are driver specific and not used by the helpers. > > */ > > int madv; > > @@ -91,6 +102,40 @@ struct drm_gem_shmem_object { > > * @map_wc: map object write-combined (instead of using shmem defaults). > > */ > > bool map_wc; > > + > > + /** > > + * @eviction_disable_count: > > + * > > + * The shmem pages are disallowed to be evicted by the memory shrinker > > + * while count is non-zero. Used internally by memory shrinker. > > + */ > > + unsigned int eviction_disable_count; > > + > > + /** > > + * @purging_disable_count: > > + * > > + * The shmem pages are disallowed to be purged by the memory shrinker > > + * while count is non-zero. Used internally by memory shrinker. > > + */ > > + unsigned int purging_disable_count; > > + > > + /** > > + * @pages_state: Current state of shmem pages. Used internally by > > + * memory shrinker. > > + */ > > + enum drm_gem_shmem_pages_state pages_state; > > + > > + /** > > + * @evicted: True if shmem pages were evicted by the memory shrinker. > > + * Used internally by memory shrinker. > > + */ > > + bool evicted; > > + > > + /** > > + * @pages_shrinkable: True if shmem pages can be evicted or purged > > + * by the memory shrinker. Used internally by memory shrinker. > > + */ > > + bool pages_shrinkable; > > }; > > #define to_drm_gem_shmem_obj(obj) \ > > @@ -111,15 +156,33 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct > > int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv); > > +int drm_gem_shmem_set_purgeable(struct drm_gem_shmem_object *shmem); > > +int drm_gem_shmem_set_purgeable_and_evictable(struct drm_gem_shmem_object *shmem); > > +int drm_gem_shmem_set_unpurgeable_and_unevictable(struct drm_gem_shmem_object *shmem); > > + > > +static inline bool drm_gem_shmem_is_evictable(struct drm_gem_shmem_object *shmem) > > +{ > > + return (shmem->madv >= 0) && !shmem->eviction_disable_count && > > + shmem->base.funcs->evict && shmem->base.funcs->swap_in && > > + !shmem->vmap_use_count && !shmem->base.dma_buf && > > + !shmem->base.import_attach && shmem->sgt; > > +} > > + > > static inline bool drm_gem_shmem_is_purgeable(struct drm_gem_shmem_object *shmem) > > { > > - return (shmem->madv > 0) && > > - !shmem->vmap_use_count && shmem->sgt && > > - !shmem->base.dma_buf && !shmem->base.import_attach; > > + return (shmem->madv > 0) && !shmem->purging_disable_count && > > + !shmem->vmap_use_count && shmem->base.funcs->purge && > > + !shmem->base.dma_buf && !shmem->base.import_attach && > > + shmem->sgt; > > } > > -void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem); > > +int drm_gem_shmem_swap_in_pages_locked(struct drm_gem_shmem_object *shmem); > > +int drm_gem_shmem_swap_in_locked(struct drm_gem_shmem_object *shmem); > > + > > +void drm_gem_shmem_evict_locked(struct drm_gem_shmem_object *shmem); > > + > > bool drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem); > > +void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem); > > struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_shmem_object *shmem); > > struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem); > > @@ -262,6 +325,38 @@ static inline int drm_gem_shmem_object_mmap(struct drm_gem_object *obj, struct v > > return drm_gem_shmem_mmap(shmem, vma); > > } > > +/** > > + * struct drm_gem_shmem_shrinker - Generic memory shrinker for shmem GEMs > > + */ > > +struct drm_gem_shmem_shrinker { > > + /** @base: Shrinker for purging shmem GEM objects */ > > + struct shrinker base; > > + > > + /** @lock: Protects @lru_* */ > > + struct mutex lock; > > + > > + /** @lru_purgeable: List of shmem GEM objects available for purging */ > > + struct list_head lru_purgeable; > > + > > + /** @lru_active: List of active shmem GEM objects */ > > + struct list_head lru_active; > > + > > + /** @lru_evictable: List of shmem GEM objects that can be evicted */ > > + struct list_head lru_evictable; > > + > > + /** @lru_evicted: List of evicted shmem GEM objects */ > > + struct list_head lru_evicted; > > + > > + /** @dev: DRM device that uses this shrinker */ > > + struct drm_device *dev; > > + > > + /** @shrinkable_count: Count of shmem GEM pages to be purged and evicted */ > > + u64 shrinkable_count; > > +}; > > + > > +int drm_gem_shmem_shrinker_register(struct drm_device *dev); > > +void drm_gem_shmem_shrinker_unregister(struct drm_device *dev); > > + > > /* > > * Driver ops > > */ > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev
On 5/5/22 11:34, Thomas Zimmermann wrote: > Hi > > Am 18.04.22 um 00:37 schrieb Dmitry Osipenko: >> Introduce a common DRM SHMEM shrinker. It allows to reduce code >> duplication among DRM drivers that implement theirs own shrinkers. >> This is initial version of the shrinker that covers basic needs of >> GPU drivers, both purging and eviction of shmem objects are supported. >> >> This patch is based on a couple ideas borrowed from Rob's Clark MSM >> shrinker and Thomas' Zimmermann variant of SHMEM shrinker. >> >> In order to start using DRM SHMEM shrinker drivers should: >> >> 1. Implement new purge(), evict() + swap_in() GEM callbacks. >> 2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device). >> 3. Use drm_gem_shmem_set_purgeable_and_evictable(shmem) and alike API >>    functions to activate shrinking of GEMs. > > Honestly speaking, after reading the patch and the discussion here I > really don't like where all tis is going. The interfaces and > implementation are overengineered. Descisions about evicting and > purging should be done by the memory manager. For the most part, it's > none of the driver's business. Daniel mostly suggesting to make interface more flexible for future drivers, so we won't need to re-do it later on. My version of the interface is based on what drivers need today. Why do you think it's a problem to turn shmem helper into the simple generic memory manager? I don't see how it's better to have drivers duplicating the exactly same efforts and making different mistakes. The shmem shrinker implementation is mostly based on the freedreno's shrinker and it's very easy to enable generic shrinker for VirtIO and Panfrost drivers. I think in the future freedreno and other drivers could switch to use drm shmem instead of open coding the memory management. > 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.
On Fri, May 06, 2022 at 03:10:43AM +0300, Dmitry Osipenko wrote: > On 5/5/22 11:34, Thomas Zimmermann wrote: > > Hi > > > > Am 18.04.22 um 00:37 schrieb Dmitry Osipenko: > >> Introduce a common DRM SHMEM shrinker. It allows to reduce code > >> duplication among DRM drivers that implement theirs own shrinkers. > >> This is initial version of the shrinker that covers basic needs of > >> GPU drivers, both purging and eviction of shmem objects are supported. > >> > >> This patch is based on a couple ideas borrowed from Rob's Clark MSM > >> shrinker and Thomas' Zimmermann variant of SHMEM shrinker. > >> > >> In order to start using DRM SHMEM shrinker drivers should: > >> > >> 1. Implement new purge(), evict() + swap_in() GEM callbacks. > >> 2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device). > >> 3. Use drm_gem_shmem_set_purgeable_and_evictable(shmem) and alike API > >>    functions to activate shrinking of GEMs. > > > > Honestly speaking, after reading the patch and the discussion here I > > really don't like where all tis is going. The interfaces and > > implementation are overengineered. Descisions about evicting and > > purging should be done by the memory manager. For the most part, it's > > none of the driver's business. > > Daniel mostly suggesting to make interface more flexible for future > drivers, so we won't need to re-do it later on. My version of the > interface is based on what drivers need today. > > Why do you think it's a problem to turn shmem helper into the simple > generic memory manager? I don't see how it's better to have drivers > duplicating the exactly same efforts and making different mistakes. > > The shmem shrinker implementation is mostly based on the freedreno's > shrinker and it's very easy to enable generic shrinker for VirtIO and > Panfrost drivers. I think in the future freedreno and other drivers > could switch to use drm shmem instead of open coding the memory management. Yeah I think we have enough shrinkers all over drm to actually design something solid here. There's also the i915 shrinker and some kinda shrinker in ttm too. So we are definitely past the "have 3 examples to make sure you design something solid" rule of thumb. I also have a bit an idea that we could try to glue the shmem shrinker into ttm, at least at a very high level that's something that would make some sense. > > 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? I'm still confused why drivers need to know the difference between evition and purging. Or maybe I'm confused again. -Daniel
On 5/9/22 16:49, Daniel Vetter wrote: > On Fri, May 06, 2022 at 03:10:43AM +0300, Dmitry Osipenko wrote: >> On 5/5/22 11:34, Thomas Zimmermann wrote: >>> Hi >>> >>> Am 18.04.22 um 00:37 schrieb Dmitry Osipenko: >>>> Introduce a common DRM SHMEM shrinker. It allows to reduce code >>>> duplication among DRM drivers that implement theirs own shrinkers. >>>> This is initial version of the shrinker that covers basic needs of >>>> GPU drivers, both purging and eviction of shmem objects are supported. >>>> >>>> This patch is based on a couple ideas borrowed from Rob's Clark MSM >>>> shrinker and Thomas' Zimmermann variant of SHMEM shrinker. >>>> >>>> In order to start using DRM SHMEM shrinker drivers should: >>>> >>>> 1. Implement new purge(), evict() + swap_in() GEM callbacks. >>>> 2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device). >>>> 3. Use drm_gem_shmem_set_purgeable_and_evictable(shmem) and alike API >>>>    functions to activate shrinking of GEMs. >>> >>> Honestly speaking, after reading the patch and the discussion here I >>> really don't like where all tis is going. The interfaces and >>> implementation are overengineered. Descisions about evicting and >>> purging should be done by the memory manager. For the most part, it's >>> none of the driver's business. >> >> Daniel mostly suggesting to make interface more flexible for future >> drivers, so we won't need to re-do it later on. My version of the >> interface is based on what drivers need today. >> >> Why do you think it's a problem to turn shmem helper into the simple >> generic memory manager? I don't see how it's better to have drivers >> duplicating the exactly same efforts and making different mistakes. >> >> The shmem shrinker implementation is mostly based on the freedreno's >> shrinker and it's very easy to enable generic shrinker for VirtIO and >> Panfrost drivers. I think in the future freedreno and other drivers >> could switch to use drm shmem instead of open coding the memory management. > > Yeah I think we have enough shrinkers all over drm to actually design > something solid here. > > There's also the i915 shrinker and some kinda shrinker in ttm too. So we > are definitely past the "have 3 examples to make sure you design something > solid" rule of thumb. > > I also have a bit an idea that we could try to glue the shmem shrinker > into ttm, at least at a very high level that's something that would make > some sense. Before gluing the shmem shrinker into ttm, the drivers should be switched to ttm? Or do you mean something else by the gluing? Perhaps it should be possible to have a common drm-shrinker helper that will do the basic-common things like tracking the eviction size and check whether BO is exported or locked, but we shouldn't consider doing this for now. For the starter more reasonable should be to create a common shrinker base for drivers that use drm-shmem, IMO. >>> 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'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. The drm-shmem only handles shmem pages, not the mappings of these pages.
On Tue, May 10, 2022 at 04:47:52PM +0300, Dmitry Osipenko wrote: > On 5/9/22 16:49, Daniel Vetter wrote: > > On Fri, May 06, 2022 at 03:10:43AM +0300, Dmitry Osipenko wrote: > >> On 5/5/22 11:34, Thomas Zimmermann wrote: > >>> Hi > >>> > >>> Am 18.04.22 um 00:37 schrieb Dmitry Osipenko: > >>>> Introduce a common DRM SHMEM shrinker. It allows to reduce code > >>>> duplication among DRM drivers that implement theirs own shrinkers. > >>>> This is initial version of the shrinker that covers basic needs of > >>>> GPU drivers, both purging and eviction of shmem objects are supported. > >>>> > >>>> This patch is based on a couple ideas borrowed from Rob's Clark MSM > >>>> shrinker and Thomas' Zimmermann variant of SHMEM shrinker. > >>>> > >>>> In order to start using DRM SHMEM shrinker drivers should: > >>>> > >>>> 1. Implement new purge(), evict() + swap_in() GEM callbacks. > >>>> 2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device). > >>>> 3. Use drm_gem_shmem_set_purgeable_and_evictable(shmem) and alike API > >>>>    functions to activate shrinking of GEMs. > >>> > >>> Honestly speaking, after reading the patch and the discussion here I > >>> really don't like where all tis is going. The interfaces and > >>> implementation are overengineered. Descisions about evicting and > >>> purging should be done by the memory manager. For the most part, it's > >>> none of the driver's business. > >> > >> Daniel mostly suggesting to make interface more flexible for future > >> drivers, so we won't need to re-do it later on. My version of the > >> interface is based on what drivers need today. > >> > >> Why do you think it's a problem to turn shmem helper into the simple > >> generic memory manager? I don't see how it's better to have drivers > >> duplicating the exactly same efforts and making different mistakes. > >> > >> The shmem shrinker implementation is mostly based on the freedreno's > >> shrinker and it's very easy to enable generic shrinker for VirtIO and > >> Panfrost drivers. I think in the future freedreno and other drivers > >> could switch to use drm shmem instead of open coding the memory management. > > > > Yeah I think we have enough shrinkers all over drm to actually design > > something solid here. > > > > There's also the i915 shrinker and some kinda shrinker in ttm too. So we > > are definitely past the "have 3 examples to make sure you design something > > solid" rule of thumb. > > > > I also have a bit an idea that we could try to glue the shmem shrinker > > into ttm, at least at a very high level that's something that would make > > some sense. > > Before gluing the shmem shrinker into ttm, the drivers should be > switched to ttm? Or do you mean something else by the gluing? No, drivers which don't need ttm shouldn't be forced to use it. > Perhaps it should be possible to have a common drm-shrinker helper that > will do the basic-common things like tracking the eviction size and > check whether BO is exported or locked, but we shouldn't consider doing > this for now. For the starter more reasonable should be to create a > common shrinker base for drivers that use drm-shmem, IMO. Yeah that might be the more practical approach. But really this was just an aside, absolutely no need to worry about this for now. I just wanted to point out that there really is a lot of use for this. > >>> 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. > > 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 drm-shmem only handles shmem pages, not the mappings of these pages. Yeah that's why you need an evict callback into the driver. That part is clear. -Daniel
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'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.
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. > >>> 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. -Daniel
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.
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. I just don't think that encouraging drivers to distinguish between evict/purge is a good idea for almost all of them. -Daniel
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. Thank you all for taking time to look at this patchset. I'm preparing the new version.
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
On 5/19/22 17:13, Daniel Vetter wrote: > 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. I already reworked patches like you suggested to use single evict() cb and etc. But those are minor things, the lockings are more important. I'm now having more h/w on my hands and yesterday found that Lima driver was getting deadlock using the new dma-buf locking convention that I'm working on, so I'm now re-testing all thoroughly and will send out v6 once will be confident in it.
On Thu, Apr 28, 2022 at 11:20 AM Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > 27.04.2022 18:03, Daniel Vetter wrote: > >> ... > >>>> @@ -172,6 +172,41 @@ struct drm_gem_object_funcs { > >>>> * This is optional but necessary for mmap support. > >>>> */ > >>>> const struct vm_operations_struct *vm_ops; > >>>> + > >>>> + /** > >>>> + * @purge: > >>>> + * > >>>> + * Releases the GEM object's allocated backing storage to the > >>>> system. > >>>> + * > >>>> + * Returns the number of pages that have been freed by purging > >>>> the GEM object. > >>>> + * > >>>> + * This callback is used by the GEM shrinker. > >>>> + */ > >>>> + unsigned long (*purge)(struct drm_gem_object *obj); > > > > Hm I feel like drivers shouldn't need to know the difference here? > > > > Like shmem helpers can track what's purgeable, and for eviction/purging > > the driver callback should do the same? > > > > The only difference is when we try to re-reserve the backing storage. When > > the object has been evicted that should suceed, but when the object is > > purged that will fail. > > > > That's the difference between evict and purge for drivers? > > When buffer is purged, we can permanently release the backing storage > and the reserved IOV space, re-using the freed space by new BOs. > > When buffer is evicted, the BO's IOV should be kept reserved and the > re-reservation of the backing storage should succeed. > > >>>> + > >>>> + /** > >>>> + * @evict: > >>>> + * > >>>> + * Unpins the GEM object's allocated backing storage, allowing > >>>> shmem pages > >>>> + * to be swapped out. > >>> > >>> What's the difference to the existing unpin() callback? > >> > >> Drivers need to do more than just unpinning pages when GEMs are evicted. > >> Unpinning is only a part of the eviction process. I'll improve the > >> doc-comment in v5. > >> > >> For example, for VirtIO-GPU driver we need to to detach host from the > >> guest's memory before pages are evicted [1]. > >> > >> [1] > >> https://gitlab.collabora.com/dmitry.osipenko/linux-kernel-rd/-/blob/932eb03198bce3a21353b09ab71e95f1c19b84c2/drivers/gpu/drm/virtio/virtgpu_object.c#L145 > >> > >> In case of Panfrost driver, we will need to remove mappings before pages > >> are evicted. > > > > It might be good to align this with ttm, otoh that all works quite a bit > > differently for ttm since ttm supports buffer moves and a lot more fancy > > stuff. > > > > I'm bringing this up since I have this fancy idea that eventually we could > > glue shmem helpers into ttm in some cases for managing buffers when they > > sit in system memory (as opposed to vram). > > I'll take a look at ttm for v6. > > >>>> + * > >>>> + * Returns the number of pages that have been unpinned. > >>>> + * > >>>> + * This callback is used by the GEM shrinker. > >>>> + */ > >>>> + unsigned long (*evict)(struct drm_gem_object *obj); > >>>> + > >>>> + /** > >>>> + * @swap_in: > >>>> + * > >>>> + * Pins GEM object's allocated backing storage if it was > >>>> previously evicted, > >>>> + * moving swapped out pages back to memory. > >>>> + * > >>>> + * Returns 0 on success, or -errno on error. > >>>> + * > >>>> + * This callback is used by the GEM shrinker. > >>>> + */ > >>>> + int (*swap_in)(struct drm_gem_object *obj); > >>> > >>> Why do you need swap_in()? This can be done on-demand as part of a pin > >>> or vmap operation. > >> > >> Similarly to the unpinning, the pining of pages is only a part of what > >> needs to be done for GPU drivers. Besides of returning pages back to > >> memory, we also need to make them accessible to GPU and this is a > >> driver-specific process. This why we need the additional callbacks. > > > > This is a bit much midlayer. The way this works in ttm is you reserve all > > the objects you need (which makes sure they're physically available > > again), and then the driver goes through and makes sure the page tables > > are all set up again. > > > > Once you get towards gpu vm that's really the only approach, since your > > swap_in has no idea for which vm it needs to restore pagetables (and > > restoring it for all is a bit meh). > > > > If drivers want to optimize this they can adjust/set any tracking > > information from their evict callback as needed. > > In practice, majority of BOs have only one mapping. Only shared BOs > usually have extra mappings and shared BOs aren't evictable. > > When memory pages are gone, then all the GPU mappings also should be > gone. Perhaps it's indeed won't be a bad idea to move out the restoring > of h/w VMs from the swap_in() and make drivers to handle the restoring > by themselves, so swap_in() will be only about restoring the pages. I'll > try to improve it in v6. > > >>>> }; > >>>> /** > >>>> diff --git a/include/drm/drm_gem_shmem_helper.h > >>>> b/include/drm/drm_gem_shmem_helper.h > >>>> index 70889533962a..a65557b446e6 100644 > >>>> --- a/include/drm/drm_gem_shmem_helper.h > >>>> +++ b/include/drm/drm_gem_shmem_helper.h > >>>> @@ -6,6 +6,7 @@ > >>>> #include <linux/fs.h> > >>>> #include <linux/mm.h> > >>>> #include <linux/mutex.h> > >>>> +#include <linux/shrinker.h> > >>>> #include <drm/drm_file.h> > >>>> #include <drm/drm_gem.h> > >>>> @@ -15,8 +16,18 @@ > >>>> struct dma_buf_attachment; > >>>> struct drm_mode_create_dumb; > >>>> struct drm_printer; > >>>> +struct drm_device; > >>>> struct sg_table; > >>>> +enum drm_gem_shmem_pages_state { > >>>> + DRM_GEM_SHMEM_PAGES_STATE_PURGED = -2, > >>>> + DRM_GEM_SHMEM_PAGES_STATE_EVICTED = -1, > >>>> + DRM_GEM_SHMEM_PAGES_STATE_UNPINNED = 0, > >>>> + DRM_GEM_SHMEM_PAGES_STATE_PINNED = 1, > >>>> + DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE = 2, > >>>> + DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE = 3, > >>>> +}; > >>> > >>> These states can be detected by looking at the vmap and pin refcounts. > >>> No need to store them explicitly. > >> > >> I'll try to revisit this, but I was finding that it's much more > >> difficult to follow and debug code without the explicit states. > > > > purgeable/purged needs some state, but pinned shouldn't be duplicated, so > > I concur here a bit. > > > >>> In your patch, they also come with a > >>> big zoo of trivial helpers. None of that seems necessary AFAICT. > >> > >> There are couple functions which could be squashed, although this may > >> hurt readability of the code a tad. I'll try to take another look at > >> this for v5. > >> > >>> What's the difference between purge and evict BTW? > >> > >> The evicted pages are moved out from memory to a SWAP partition or file. > >> > >> The purged pages are destroyed permanently. > >> > >>>> + > >>>> /** > >>>> * struct drm_gem_shmem_object - GEM object backed by shmem > >>>> */ > >>>> @@ -43,8 +54,8 @@ struct drm_gem_shmem_object { > >>>> * @madv: State for madvise > >>>> * > >>>> * 0 is active/inuse. > >>>> + * 1 is not-needed/can-be-purged > >>>> * A negative value is the object is purged. > >>>> - * Positive values are driver specific and not used by the helpers. > >>>> */ > >>>> int madv; > >>>> @@ -91,6 +102,40 @@ struct drm_gem_shmem_object { > >>>> * @map_wc: map object write-combined (instead of using shmem > >>>> defaults). > >>>> */ > >>>> bool map_wc; > >>>> + > >>>> + /** > >>>> + * @eviction_disable_count: > >>>> + * > >>>> + * The shmem pages are disallowed to be evicted by the memory > >>>> shrinker > >>>> + * while count is non-zero. Used internally by memory shrinker. > >>>> + */ > >>>> + unsigned int eviction_disable_count; > >>>> + > >>>> + /** > >>>> + * @purging_disable_count: > >>>> + * > >>>> + * The shmem pages are disallowed to be purged by the memory > >>>> shrinker > >>>> + * while count is non-zero. Used internally by memory shrinker. > >>>> + */ > >>>> + unsigned int purging_disable_count; > > > > What are these disable counts for? > > Some of BO types should stay pinned permanently, this applies to both > VirtIO and Panfrost drivers that make use of the generic shrinker in > this patchset. Hence I made objects unpurgeable and unevictable by default. > > Initially the idea of these counts was to allow drivers to explicitly > disable purging and eviction, and do it multiple times. If driver > disables eviction in two different places in the code, then we need to > track the eviction-disable count. > > In the v5 of this patchset drivers don't need to explicitly disable > shrinking anymore, they only need to enable it. The counts are also used > internally by DRM SHMEM core to track the vmappings and pinnings, but > perhaps pages_use_count could be used for that instead. I'll revisit it > for v6. > > > The way purgeable works in other drivers is that userspace sets purgeable > > or not, and it's up to userspace to not make a mess of this. > > > > There's also some interactions, and I guess a bunch of drivers get this > > wrong in funny ways. Not sure how to best clean this up. > > > > - Once you have a shrinker/dynamic memory management you should _not_ pin > > pages, except when it's truly permanent like for scanout. Instead > > drivers should attach dma_fence to the dma_resv to denote in-flight > > access. > > By default pages are pinned when drm_gem_shmem_get_pages_sgt() is > invoked by drivers during of BO creation time. > > We could declare that pages_use_count=1 means the pages are allowed to > be evicted and purged if shrinker is enabled. Then the further > drm_gem_shmem_pin/vmap() calls will bump the pages_use_count, > disallowing the eviction and purging, like you're suggesting, and we > won't need the explicit counts. > > > - A pinned buffer object is not allowed to be put into purgeable state, > > and a bo in purgeable state should not be allowed to be pinned. > > > > - Drivers need to hold dma_resv_lock for long enough in their command > > submission, i.e. from the point where the reserve the buffers and make > > sure that mappings exists, to the point where the request is submitted > > to hw or drm/sched and fences are installed. > > > > But I think a lot of current shmem users just pin as part of execbuf, so > > this won't work quite so well right out of the box. > > The current shmem users assume that BO is pinned permanently once it has > been created. > > > Anyway with that design I don't think there should ever be a need to > > disable shrinking. > > To me what you described mostly matches to what I did in the v5. > > >>>> + > >>>> + /** > >>>> + * @pages_state: Current state of shmem pages. Used internally by > >>>> + * memory shrinker. > >>>> + */ > >>>> + enum drm_gem_shmem_pages_state pages_state; > >>>> + > >>>> + /** > >>>> + * @evicted: True if shmem pages were evicted by the memory > >>>> shrinker. > >>>> + * Used internally by memory shrinker. > >>>> + */ > >>>> + bool evicted; > >>>> + > >>>> + /** > >>>> + * @pages_shrinkable: True if shmem pages can be evicted or purged > >>>> + * by the memory shrinker. Used internally by memory shrinker. > >>>> + */ > >>>> + bool pages_shrinkable; > >>> > >>> As commented before, this state can be foundby looking at existing > >>> fields. No need to store it separately. > >> > >> When we're transitioning from "evictable" to a "purgeable" state, we > >> must not add pages twice to the "shrinkable_count" variable. Hence this > >> is not a state, but a variable which prevents the double accounting of > >> the pages. Please see drm_gem_shmem_add_pages_to_shrinker() in this patch. > >> > >> Perhaps something like "pages_accounted_by_shrinker" could be a better > >> name for the variable. I'll revisit this for v5. > > > > Hm not sure we need to account this? Usually the shrinker just counts when > > it's asked to do so, not practively maintain that count. Once you start > > shrinking burning cpu time is generally not too terrible. > > We could count pages on demand by walking up the "evictable" list, but > then the shrinker's lock needs to be taken by the > drm_gem_shmem_shrinker_count_objects() to protect the list. > > Previously Rob Clark said that the profiling of freedreno's shrinker > showed that it's worthwhile to reduce the locks as much as possible, > including the case of counting shrinkable objects. Sorry I missed this earlier, but danvet is giving some bad advice here ;-) You *really* need count_objects() to be lockless and fast, ie. no list iteration. It doesn't have to return the "perfect" value, so it is ok if it is racy / not-atomic / etc. Otherwise you will have bad system performance issues when you start hitting do_shrink_slab() on many threads at once. BR, -R
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 3ecef571eff3..3838fb8d6f3a 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -88,6 +88,13 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private) INIT_LIST_HEAD(&shmem->madv_list); + /* + * Eviction and purging are disabled by default, shmem user must enable + * them explicitly using drm_gem_shmem_set_evictable/purgeable(). + */ + shmem->eviction_disable_count = 1; + shmem->purging_disable_count = 1; + if (!private) { /* * Our buffers are kept pinned, so allocating them @@ -126,6 +133,107 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t } EXPORT_SYMBOL_GPL(drm_gem_shmem_create); +static void +drm_gem_shmem_add_pages_to_shrinker(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 (!shmem->pages_shrinkable) { + WARN_ON(gem_shrinker->shrinkable_count + page_count < page_count); + gem_shrinker->shrinkable_count += page_count; + shmem->pages_shrinkable = true; + } +} + +static void +drm_gem_shmem_remove_pages_from_shrinker(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 (shmem->pages_shrinkable) { + WARN_ON(gem_shrinker->shrinkable_count < page_count); + gem_shrinker->shrinkable_count -= page_count; + shmem->pages_shrinkable = false; + } +} + +static void +drm_gem_shmem_set_pages_state_locked(struct drm_gem_shmem_object *shmem, + enum drm_gem_shmem_pages_state new_state) +{ + struct drm_gem_object *obj = &shmem->base; + struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker; + + lockdep_assert_held(&gem_shrinker->lock); + lockdep_assert_held(&obj->resv->lock.base); + + if (new_state >= DRM_GEM_SHMEM_PAGES_STATE_PINNED) { + if (drm_gem_shmem_is_evictable(shmem)) + new_state = DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE; + + if (drm_gem_shmem_is_purgeable(shmem)) + new_state = DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE; + + if (!shmem->pages) + new_state = DRM_GEM_SHMEM_PAGES_STATE_UNPINNED; + + if (shmem->evicted) + new_state = DRM_GEM_SHMEM_PAGES_STATE_EVICTED; + } + + if (shmem->pages_state == new_state) + return; + + switch (new_state) { + case DRM_GEM_SHMEM_PAGES_STATE_UNPINNED: + case DRM_GEM_SHMEM_PAGES_STATE_PURGED: + drm_gem_shmem_remove_pages_from_shrinker(shmem); + list_del_init(&shmem->madv_list); + break; + + case DRM_GEM_SHMEM_PAGES_STATE_PINNED: + drm_gem_shmem_remove_pages_from_shrinker(shmem); + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_active); + break; + + case DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE: + drm_gem_shmem_add_pages_to_shrinker(shmem); + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_purgeable); + break; + + case DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE: + drm_gem_shmem_add_pages_to_shrinker(shmem); + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_evictable); + break; + + case DRM_GEM_SHMEM_PAGES_STATE_EVICTED: + drm_gem_shmem_remove_pages_from_shrinker(shmem); + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_evicted); + break; + } + + shmem->pages_state = new_state; +} + +static void +drm_gem_shmem_set_pages_state(struct drm_gem_shmem_object *shmem, + enum drm_gem_shmem_pages_state new_state) +{ + struct drm_gem_object *obj = &shmem->base; + struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker; + + if (!gem_shrinker) + return; + + mutex_lock(&gem_shrinker->lock); + drm_gem_shmem_set_pages_state_locked(shmem, new_state); + mutex_unlock(&gem_shrinker->lock); +} + /** * drm_gem_shmem_free - Free resources associated with a shmem GEM object * @shmem: shmem GEM object to free @@ -137,6 +245,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, -1); + WARN_ON(shmem->vmap_use_count); if (obj->import_attach) { @@ -148,7 +259,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) sg_free_table(shmem->sgt); kfree(shmem->sgt); } - if (shmem->pages) + if (shmem->pages_use_count) drm_gem_shmem_put_pages(shmem); } @@ -159,18 +270,226 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) } EXPORT_SYMBOL_GPL(drm_gem_shmem_free); -static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem) +static void drm_gem_shmem_update_pages_state_locked(struct drm_gem_shmem_object *shmem) +{ + struct drm_gem_object *obj = &shmem->base; + struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker; + enum drm_gem_shmem_pages_state new_state; + + if (!gem_shrinker || obj->import_attach) + return; + + mutex_lock(&gem_shrinker->lock); + + if (!shmem->madv) + new_state = DRM_GEM_SHMEM_PAGES_STATE_PINNED; + else if (shmem->madv > 0) + new_state = DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE; + else + new_state = DRM_GEM_SHMEM_PAGES_STATE_PURGED; + + drm_gem_shmem_set_pages_state_locked(shmem, new_state); + + mutex_unlock(&gem_shrinker->lock); +} + +static void drm_gem_shmem_update_pages_state(struct drm_gem_shmem_object *shmem) +{ + dma_resv_lock(shmem->base.resv, NULL); + drm_gem_shmem_update_pages_state_locked(shmem); + dma_resv_unlock(shmem->base.resv); +} + +static int +drm_gem_shmem_set_evictable_locked(struct drm_gem_shmem_object *shmem) +{ + int ret = 0; + + WARN_ON_ONCE(!shmem->eviction_disable_count--); + + if (shmem->madv < 0) + ret = -ENOMEM; + + drm_gem_shmem_update_pages_state_locked(shmem); + + return ret; +} + +static int +drm_gem_shmem_set_unevictable_locked(struct drm_gem_shmem_object *shmem) +{ + struct drm_gem_object *obj = &shmem->base; + int err; + + if (shmem->madv < 0) + return -ENOMEM; + + if (shmem->evicted) { + err = obj->funcs->swap_in(obj); + if (err) + return err; + } + + shmem->eviction_disable_count++; + + drm_gem_shmem_update_pages_state_locked(shmem); + + return 0; +} + +static int +drm_gem_shmem_set_purgeable_locked(struct drm_gem_shmem_object *shmem) +{ + int ret = 0; + + WARN_ON_ONCE(!shmem->purging_disable_count--); + + if (shmem->madv < 0) + ret = -ENOMEM; + + drm_gem_shmem_update_pages_state_locked(shmem); + + return ret; +} + +/** + * drm_gem_shmem_set_purgeable() - Make GEM purgeable by memory shrinker + * @shmem: shmem GEM object + * + * Tell memory shrinker that this GEM can be purged. Initially purging is + * disabled for all GEMs. Each set_pureable() call must have corresponding + * set_unpureable() call. If GEM was purged, then -ENOMEM is returned. + * + * Returns: + * 0 on success or a negative error code on failure. + */ +int drm_gem_shmem_set_purgeable(struct drm_gem_shmem_object *shmem) +{ + int ret; + + dma_resv_lock(shmem->base.resv, NULL); + ret = drm_gem_shmem_set_purgeable_locked(shmem); + dma_resv_unlock(shmem->base.resv); + + return ret; +} +EXPORT_SYMBOL_GPL(drm_gem_shmem_set_purgeable); + +static int +drm_gem_shmem_set_unpurgeable_locked(struct drm_gem_shmem_object *shmem) +{ + if (shmem->madv < 0) + return -ENOMEM; + + shmem->purging_disable_count++; + + drm_gem_shmem_update_pages_state_locked(shmem); + + return 0; +} + +static int +drm_gem_shmem_set_purgeable_and_evictable_locked(struct drm_gem_shmem_object *shmem) +{ + int ret; + + ret = drm_gem_shmem_set_evictable_locked(shmem); + if (!ret) { + ret = drm_gem_shmem_set_purgeable_locked(shmem); + if (ret) + drm_gem_shmem_set_unevictable_locked(shmem); + } + + return ret; +} + +static int +drm_gem_shmem_set_unpurgeable_and_unevictable_locked(struct drm_gem_shmem_object *shmem) +{ + int ret; + + ret = drm_gem_shmem_set_unpurgeable_locked(shmem); + if (!ret) { + ret = drm_gem_shmem_set_unevictable_locked(shmem); + if (ret) + drm_gem_shmem_set_purgeable_locked(shmem); + } + + return ret; +} + +/** + * drm_gem_shmem_set_purgeable_and_evictable() - Make GEM unpurgeable and + * unevictable by memory shrinker + * @shmem: shmem GEM object + * + * Tell memory shrinker that this GEM can't be purged and evicted. Each + * set_purgeable_and_evictable() call must have corresponding + * unpurgeable_and_unevictable() call. If GEM was purged, then -ENOMEM + * is returned. + * + * Returns: + * 0 on success or a negative error code on failure. + */ +int drm_gem_shmem_set_purgeable_and_evictable(struct drm_gem_shmem_object *shmem) +{ + int ret; + + dma_resv_lock(shmem->base.resv, NULL); + ret = drm_gem_shmem_set_purgeable_and_evictable_locked(shmem); + dma_resv_unlock(shmem->base.resv); + + return ret; +} +EXPORT_SYMBOL_GPL(drm_gem_shmem_set_purgeable_and_evictable); + +/** + * drm_gem_shmem_set_unpurgeable_and_unevictable() - Make GEM purgeable and + * evictable by memory shrinker + * @shmem: shmem GEM object + * + * Tell memory shrinker that this GEM can be purged and evicted. Each + * unpurgeable_and_unevictable() call must have corresponding + * set_purgeable_and_evictable() call. If GEM was purged, then -ENOMEM + * is returned. + * + * Returns: + * 0 on success or a negative error code on failure. + */ +int drm_gem_shmem_set_unpurgeable_and_unevictable(struct drm_gem_shmem_object *shmem) +{ + int ret; + + ret = dma_resv_lock_interruptible(shmem->base.resv, NULL); + if (ret) + return ret; + + ret = drm_gem_shmem_set_unpurgeable_and_unevictable_locked(shmem); + dma_resv_unlock(shmem->base.resv); + + return ret; +} +EXPORT_SYMBOL_GPL(drm_gem_shmem_set_unpurgeable_and_unevictable); + +static int +drm_gem_shmem_acquire_pages_locked(struct drm_gem_shmem_object *shmem) { struct drm_gem_object *obj = &shmem->base; struct page **pages; - if (shmem->pages_use_count++ > 0) + if (shmem->madv < 0) { + WARN_ON(shmem->pages); + return -ENOMEM; + } + + if (shmem->pages) { + WARN_ON(!shmem->evicted); return 0; + } pages = drm_gem_get_pages(obj); if (IS_ERR(pages)) { DRM_DEBUG_KMS("Failed to get pages (%ld)\n", PTR_ERR(pages)); - shmem->pages_use_count = 0; return PTR_ERR(pages); } @@ -189,6 +508,25 @@ static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem) return 0; } +static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem) +{ + int err; + + if (shmem->madv < 0) + return -ENOMEM; + + if (shmem->pages_use_count++ > 0) + return 0; + + err = drm_gem_shmem_acquire_pages_locked(shmem); + if (err) { + shmem->pages_use_count = 0; + return err; + } + + return 0; +} + /* * drm_gem_shmem_get_pages - Allocate backing pages for a shmem GEM object * @shmem: shmem GEM object @@ -209,21 +547,38 @@ int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem) if (ret) return ret; ret = drm_gem_shmem_get_pages_locked(shmem); + + drm_gem_shmem_update_pages_state_locked(shmem); + dma_resv_unlock(shmem->base.resv); return ret; } EXPORT_SYMBOL(drm_gem_shmem_get_pages); -static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) +static void drm_gem_shmem_get_pages_no_fail(struct drm_gem_shmem_object *shmem) { - struct drm_gem_object *obj = &shmem->base; + WARN_ON(shmem->base.import_attach); - if (WARN_ON_ONCE(!shmem->pages_use_count)) - return; + dma_resv_lock(shmem->base.resv, NULL); - if (--shmem->pages_use_count > 0) + if (drm_gem_shmem_get_pages_locked(shmem)) + shmem->pages_use_count++; + + drm_gem_shmem_update_pages_state_locked(shmem); + + dma_resv_unlock(shmem->base.resv); +} + +static void +drm_gem_shmem_release_pages_locked(struct drm_gem_shmem_object *shmem) +{ + struct drm_gem_object *obj = &shmem->base; + + if (!shmem->pages) { + WARN_ON(!shmem->evicted && shmem->madv >= 0); return; + } #ifdef CONFIG_X86 if (shmem->map_wc) @@ -236,6 +591,21 @@ static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) shmem->pages = NULL; } +static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) +{ + struct drm_gem_object *obj = &shmem->base; + + lockdep_assert_held(&obj->resv->lock.base); + + if (WARN_ON(!shmem->pages_use_count)) + return; + + if (--shmem->pages_use_count > 0) + return; + + drm_gem_shmem_release_pages_locked(shmem); +} + /* * drm_gem_shmem_put_pages - Decrease use count on the backing pages for a shmem GEM object * @shmem: shmem GEM object @@ -246,6 +616,7 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem) { dma_resv_lock(shmem->base.resv, NULL); drm_gem_shmem_put_pages_locked(shmem); + drm_gem_shmem_update_pages_state_locked(shmem); dma_resv_unlock(shmem->base.resv); } EXPORT_SYMBOL(drm_gem_shmem_put_pages); @@ -262,9 +633,21 @@ EXPORT_SYMBOL(drm_gem_shmem_put_pages); */ int drm_gem_shmem_pin(struct drm_gem_shmem_object *shmem) { + int err; + WARN_ON(shmem->base.import_attach); - return drm_gem_shmem_get_pages(shmem); + err = drm_gem_shmem_set_unpurgeable_and_unevictable(shmem); + if (err) + return err; + + err = drm_gem_shmem_get_pages(shmem); + if (err) { + drm_gem_shmem_set_purgeable_and_evictable(shmem); + return err; + } + + return 0; } EXPORT_SYMBOL(drm_gem_shmem_pin); @@ -280,6 +663,7 @@ void drm_gem_shmem_unpin(struct drm_gem_shmem_object *shmem) WARN_ON(shmem->base.import_attach); drm_gem_shmem_put_pages(shmem); + drm_gem_shmem_set_purgeable_and_evictable(shmem); } EXPORT_SYMBOL(drm_gem_shmem_unpin); @@ -359,7 +743,18 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem, ret = dma_resv_lock_interruptible(shmem->base.resv, NULL); if (ret) return ret; + + ret = drm_gem_shmem_set_unpurgeable_and_unevictable_locked(shmem); + if (ret) + goto unlock; + ret = drm_gem_shmem_vmap_locked(shmem, map); + if (ret) + drm_gem_shmem_set_purgeable_and_evictable_locked(shmem); + else + drm_gem_shmem_update_pages_state_locked(shmem); + +unlock: dma_resv_unlock(shmem->base.resv); return ret; @@ -404,9 +799,9 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem, { dma_resv_lock(shmem->base.resv, NULL); drm_gem_shmem_vunmap_locked(shmem, map); + drm_gem_shmem_update_pages_state_locked(shmem); + drm_gem_shmem_set_purgeable_and_evictable_locked(shmem); dma_resv_unlock(shmem->base.resv); - - drm_gem_shmem_update_purgeable_status(shmem); } EXPORT_SYMBOL(drm_gem_shmem_vunmap); @@ -447,29 +842,140 @@ int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv) madv = shmem->madv; + drm_gem_shmem_update_pages_state_locked(shmem); + dma_resv_unlock(shmem->base.resv); return (madv >= 0); } EXPORT_SYMBOL(drm_gem_shmem_madvise); -void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem) +/** + * drm_gem_shmem_swap_in_pages_locked() - Moves shmem pages back to memory + * @shmem: shmem GEM object + * + * This function moves pages back to memory if they were previously evicted + * by the memory shrinker. + * + * Returns: + * 0 on success or a negative error code on failure. + */ +int drm_gem_shmem_swap_in_pages_locked(struct drm_gem_shmem_object *shmem) +{ + struct drm_gem_object *obj = &shmem->base; + struct sg_table *sgt; + int ret; + + lockdep_assert_held(&obj->resv->lock.base); + + if (shmem->evicted) { + ret = drm_gem_shmem_acquire_pages_locked(shmem); + if (ret) + return ret; + + sgt = drm_gem_shmem_get_sg_table(shmem); + if (IS_ERR(sgt)) + return PTR_ERR(sgt); + + ret = dma_map_sgtable(obj->dev->dev, sgt, + DMA_BIDIRECTIONAL, 0); + if (ret) { + sg_free_table(sgt); + kfree(sgt); + return ret; + } + + shmem->sgt = sgt; + shmem->evicted = false; + shmem->pages_state = DRM_GEM_SHMEM_PAGES_STATE_PINNED; + + drm_gem_shmem_update_pages_state_locked(shmem); + } + + return shmem->pages ? 0 : -ENOMEM; +} +EXPORT_SYMBOL_GPL(drm_gem_shmem_swap_in_pages_locked); + +/** + * drm_gem_shmem_swap_in_locked() - Moves shmem GEM back to memory + * @shmem: shmem GEM object + * + * This function moves shmem GEM back to memory if it was previously evicted + * by the memory shrinker. The GEM is ready to use on success. + * + * Returns: + * 0 on success or a negative error code on failure. + */ +int drm_gem_shmem_swap_in_locked(struct drm_gem_shmem_object *shmem) +{ + struct drm_gem_object *obj = &shmem->base; + + lockdep_assert_held(&obj->resv->lock.base); + + if (shmem->evicted) + return obj->funcs->swap_in(obj); + + return 0; +} +EXPORT_SYMBOL_GPL(drm_gem_shmem_swap_in_locked); + +static void drm_gem_shmem_unpin_pages_locked(struct drm_gem_shmem_object *shmem) { struct drm_gem_object *obj = &shmem->base; struct drm_device *dev = obj->dev; - WARN_ON(!drm_gem_shmem_is_purgeable(shmem)); + if (shmem->evicted) + return; dma_unmap_sgtable(dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0); + drm_gem_shmem_release_pages_locked(shmem); + drm_vma_node_unmap(&obj->vma_node, dev->anon_inode->i_mapping); + sg_free_table(shmem->sgt); kfree(shmem->sgt); shmem->sgt = NULL; +} - drm_gem_shmem_put_pages_locked(shmem); +/** + * drm_gem_shmem_evict_locked - Evict shmem pages + * @shmem: shmem GEM object + * + * This function unpins shmem pages, allowing them to be swapped out from + * memory. + */ +void drm_gem_shmem_evict_locked(struct drm_gem_shmem_object *shmem) +{ + struct drm_gem_object *obj = &shmem->base; - shmem->madv = -1; + lockdep_assert_held(&obj->resv->lock.base); - drm_vma_node_unmap(&obj->vma_node, dev->anon_inode->i_mapping); + WARN_ON(!drm_gem_shmem_is_evictable(shmem)); + WARN_ON(shmem->madv < 0); + WARN_ON(shmem->evicted); + + drm_gem_shmem_unpin_pages_locked(shmem); + + shmem->evicted = true; + drm_gem_shmem_set_pages_state(shmem, DRM_GEM_SHMEM_PAGES_STATE_EVICTED); +} +EXPORT_SYMBOL_GPL(drm_gem_shmem_evict_locked); + +/** + * drm_gem_shmem_purge_locked - Purge shmem pages + * @shmem: shmem GEM object + * + * This function permanently releases shmem pages. + */ +void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem) +{ + struct drm_gem_object *obj = &shmem->base; + + lockdep_assert_held(&obj->resv->lock.base); + + WARN_ON(!drm_gem_shmem_is_purgeable(shmem)); + WARN_ON(shmem->madv < 0); + + drm_gem_shmem_unpin_pages_locked(shmem); drm_gem_free_mmap_offset(obj); /* Our goal here is to return as much of the memory as @@ -480,6 +986,9 @@ void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem) shmem_truncate_range(file_inode(obj->filp), 0, (loff_t)-1); invalidate_mapping_pages(file_inode(obj->filp)->i_mapping, 0, (loff_t)-1); + + shmem->madv = -1; + drm_gem_shmem_set_pages_state(shmem, DRM_GEM_SHMEM_PAGES_STATE_PURGED); } EXPORT_SYMBOL(drm_gem_shmem_purge_locked); @@ -543,22 +1052,31 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf) vm_fault_t ret; struct page *page; pgoff_t page_offset; + bool pages_inactive; + int err; /* We don't use vmf->pgoff since that has the fake offset */ page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT; dma_resv_lock(shmem->base.resv, NULL); - if (page_offset >= num_pages || - WARN_ON_ONCE(!shmem->pages) || - shmem->madv < 0) { + pages_inactive = shmem->pages_state < DRM_GEM_SHMEM_PAGES_STATE_PINNED; + WARN_ON_ONCE(!shmem->pages ^ pages_inactive); + + if (page_offset >= num_pages || (!shmem->pages && !shmem->evicted)) { ret = VM_FAULT_SIGBUS; } else { + err = drm_gem_shmem_swap_in_locked(shmem); + if (err) { + ret = VM_FAULT_OOM; + goto unlock; + } + page = shmem->pages[page_offset]; ret = vmf_insert_pfn(vma, vmf->address, page_to_pfn(page)); } - +unlock: dma_resv_unlock(shmem->base.resv); return ret; @@ -568,13 +1086,8 @@ static void drm_gem_shmem_vm_open(struct vm_area_struct *vma) { struct drm_gem_object *obj = vma->vm_private_data; struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); - int ret; - - WARN_ON(shmem->base.import_attach); - - ret = drm_gem_shmem_get_pages(shmem); - WARN_ON_ONCE(ret != 0); + drm_gem_shmem_get_pages_no_fail(shmem); drm_gem_vm_open(vma); } @@ -716,6 +1229,8 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem) shmem->sgt = sgt; + drm_gem_shmem_update_pages_state(shmem); + return sgt; err_free_sgt: @@ -762,6 +1277,202 @@ 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 = READ_ONCE(gem_shrinker->shrinkable_count); + + if (count >= SHRINK_EMPTY) + return SHRINK_EMPTY - 1; + + return count ?: SHRINK_EMPTY; +} + +static unsigned long +drm_gem_shmem_shrinker_run_objects_scan(struct shrinker *shrinker, + unsigned long nr_to_scan, + bool *lock_contention, + bool evict) +{ + struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker); + struct drm_gem_shmem_object *shmem; + struct list_head still_in_list; + struct drm_gem_object *obj; + unsigned long freed = 0; + struct list_head *lru; + size_t page_count; + + INIT_LIST_HEAD(&still_in_list); + + mutex_lock(&gem_shrinker->lock); + + if (evict) + lru = &gem_shrinker->lru_evictable; + else + lru = &gem_shrinker->lru_purgeable; + + while (freed < nr_to_scan) { + shmem = list_first_entry_or_null(lru, typeof(*shmem), madv_list); + if (!shmem) + break; + + obj = &shmem->base; + page_count = obj->size >> PAGE_SHIFT; + list_move_tail(&shmem->madv_list, &still_in_list); + + if (evict && get_nr_swap_pages() < page_count) + continue; + + /* + * 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)) { + *lock_contention |= true; + goto shrinker_lock; + } + + /* prevent racing with the dma-buf exporting */ + if (!mutex_trylock(&gem_shrinker->dev->object_name_lock)) { + *lock_contention |= true; + goto resv_unlock; + } + + /* check whether h/w uses this object */ + if (!dma_resv_test_signaled(obj->resv, DMA_RESV_USAGE_WRITE)) + goto object_name_unlock; + + /* GEM may've become unpurgeable while shrinker was unlocked */ + if (evict) { + if (!drm_gem_shmem_is_evictable(shmem)) + goto object_name_unlock; + } else { + if (!drm_gem_shmem_is_purgeable(shmem)) + goto object_name_unlock; + } + + if (evict) + freed += obj->funcs->evict(obj); + else + freed += obj->funcs->purge(obj); +object_name_unlock: + mutex_unlock(&gem_shrinker->dev->object_name_lock); +resv_unlock: + dma_resv_unlock(obj->resv); +shrinker_lock: + drm_gem_object_put(&shmem->base); + mutex_lock(&gem_shrinker->lock); + } + + list_splice_tail(&still_in_list, lru); + + mutex_unlock(&gem_shrinker->lock); + + return freed; +} + +static unsigned long +drm_gem_shmem_shrinker_scan_objects(struct shrinker *shrinker, + struct shrink_control *sc) +{ + unsigned long nr_to_scan = sc->nr_to_scan; + bool lock_contention = false; + unsigned long freed; + + /* purge as many objects as we can */ + freed = drm_gem_shmem_shrinker_run_objects_scan(shrinker, nr_to_scan, + &lock_contention, false); + nr_to_scan -= freed; + + /* evict as many objects as we can */ + if (freed < nr_to_scan) + freed += drm_gem_shmem_shrinker_run_objects_scan(shrinker, + nr_to_scan, + &lock_contention, + true); + + return (!freed && !lock_contention) ? SHRINK_STOP : freed; +} + +/** + * drm_gem_shmem_shrinker_register() - Register shmem shrinker + * @dev: DRM device + * + * Returns: + * 0 on success or a negative error code on failure. + */ +int drm_gem_shmem_shrinker_register(struct drm_device *dev) +{ + struct drm_gem_shmem_shrinker *gem_shrinker; + int err; + + if (WARN_ON(dev->shmem_shrinker)) + return -EBUSY; + + gem_shrinker = kzalloc(sizeof(*gem_shrinker), GFP_KERNEL); + if (!gem_shrinker) + return -ENOMEM; + + gem_shrinker->base.count_objects = drm_gem_shmem_shrinker_count_objects; + gem_shrinker->base.scan_objects = drm_gem_shmem_shrinker_scan_objects; + gem_shrinker->base.seeks = DEFAULT_SEEKS; + gem_shrinker->dev = dev; + + INIT_LIST_HEAD(&gem_shrinker->lru_purgeable); + INIT_LIST_HEAD(&gem_shrinker->lru_evictable); + INIT_LIST_HEAD(&gem_shrinker->lru_evicted); + INIT_LIST_HEAD(&gem_shrinker->lru_active); + mutex_init(&gem_shrinker->lock); + + dev->shmem_shrinker = gem_shrinker; + + err = register_shrinker(&gem_shrinker->base); + if (err) { + dev->shmem_shrinker = NULL; + kfree(gem_shrinker); + return err; + } + + return 0; +} +EXPORT_SYMBOL_GPL(drm_gem_shmem_shrinker_register); + +/** + * drm_gem_shmem_shrinker_unregister() - Unregister shmem shrinker + * @dev: DRM device + */ +void drm_gem_shmem_shrinker_unregister(struct drm_device *dev) +{ + struct drm_gem_shmem_shrinker *gem_shrinker = dev->shmem_shrinker; + + if (gem_shrinker) { + unregister_shrinker(&gem_shrinker->base); + WARN_ON(!list_empty(&gem_shrinker->lru_purgeable)); + WARN_ON(!list_empty(&gem_shrinker->lru_evictable)); + WARN_ON(!list_empty(&gem_shrinker->lru_evicted)); + WARN_ON(!list_empty(&gem_shrinker->lru_active)); + mutex_destroy(&gem_shrinker->lock); + dev->shmem_shrinker = NULL; + kfree(gem_shrinker); + } +} +EXPORT_SYMBOL_GPL(drm_gem_shmem_shrinker_unregister); + MODULE_DESCRIPTION("DRM SHMEM memory-management helpers"); MODULE_IMPORT_NS(DMA_BUF); MODULE_LICENSE("GPL v2"); diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h index 9923c7a6885e..929546cad894 100644 --- a/include/drm/drm_device.h +++ b/include/drm/drm_device.h @@ -16,6 +16,7 @@ struct drm_vblank_crtc; struct drm_vma_offset_manager; struct drm_vram_mm; struct drm_fb_helper; +struct drm_gem_shmem_shrinker; struct inode; @@ -277,6 +278,9 @@ struct drm_device { /** @vram_mm: VRAM MM memory manager */ struct drm_vram_mm *vram_mm; + /** @shmem_shrinker: SHMEM GEM memory shrinker */ + struct drm_gem_shmem_shrinker *shmem_shrinker; + /** * @switch_power_state: * diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 9d7c61a122dc..390d1ce08ed3 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -172,6 +172,41 @@ struct drm_gem_object_funcs { * This is optional but necessary for mmap support. */ const struct vm_operations_struct *vm_ops; + + /** + * @purge: + * + * Releases the GEM object's allocated backing storage to the system. + * + * Returns the number of pages that have been freed by purging the GEM object. + * + * This callback is used by the GEM shrinker. + */ + unsigned long (*purge)(struct drm_gem_object *obj); + + /** + * @evict: + * + * Unpins the GEM object's allocated backing storage, allowing shmem pages + * to be swapped out. + * + * Returns the number of pages that have been unpinned. + * + * This callback is used by the GEM shrinker. + */ + unsigned long (*evict)(struct drm_gem_object *obj); + + /** + * @swap_in: + * + * Pins GEM object's allocated backing storage if it was previously evicted, + * moving swapped out pages back to memory. + * + * Returns 0 on success, or -errno on error. + * + * This callback is used by the GEM shrinker. + */ + int (*swap_in)(struct drm_gem_object *obj); }; /** diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h index 70889533962a..a65557b446e6 100644 --- a/include/drm/drm_gem_shmem_helper.h +++ b/include/drm/drm_gem_shmem_helper.h @@ -6,6 +6,7 @@ #include <linux/fs.h> #include <linux/mm.h> #include <linux/mutex.h> +#include <linux/shrinker.h> #include <drm/drm_file.h> #include <drm/drm_gem.h> @@ -15,8 +16,18 @@ struct dma_buf_attachment; struct drm_mode_create_dumb; struct drm_printer; +struct drm_device; struct sg_table; +enum drm_gem_shmem_pages_state { + DRM_GEM_SHMEM_PAGES_STATE_PURGED = -2, + DRM_GEM_SHMEM_PAGES_STATE_EVICTED = -1, + DRM_GEM_SHMEM_PAGES_STATE_UNPINNED = 0, + DRM_GEM_SHMEM_PAGES_STATE_PINNED = 1, + DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE = 2, + DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE = 3, +}; + /** * struct drm_gem_shmem_object - GEM object backed by shmem */ @@ -43,8 +54,8 @@ struct drm_gem_shmem_object { * @madv: State for madvise * * 0 is active/inuse. + * 1 is not-needed/can-be-purged * A negative value is the object is purged. - * Positive values are driver specific and not used by the helpers. */ int madv; @@ -91,6 +102,40 @@ struct drm_gem_shmem_object { * @map_wc: map object write-combined (instead of using shmem defaults). */ bool map_wc; + + /** + * @eviction_disable_count: + * + * The shmem pages are disallowed to be evicted by the memory shrinker + * while count is non-zero. Used internally by memory shrinker. + */ + unsigned int eviction_disable_count; + + /** + * @purging_disable_count: + * + * The shmem pages are disallowed to be purged by the memory shrinker + * while count is non-zero. Used internally by memory shrinker. + */ + unsigned int purging_disable_count; + + /** + * @pages_state: Current state of shmem pages. Used internally by + * memory shrinker. + */ + enum drm_gem_shmem_pages_state pages_state; + + /** + * @evicted: True if shmem pages were evicted by the memory shrinker. + * Used internally by memory shrinker. + */ + bool evicted; + + /** + * @pages_shrinkable: True if shmem pages can be evicted or purged + * by the memory shrinker. Used internally by memory shrinker. + */ + bool pages_shrinkable; }; #define to_drm_gem_shmem_obj(obj) \ @@ -111,15 +156,33 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv); +int drm_gem_shmem_set_purgeable(struct drm_gem_shmem_object *shmem); +int drm_gem_shmem_set_purgeable_and_evictable(struct drm_gem_shmem_object *shmem); +int drm_gem_shmem_set_unpurgeable_and_unevictable(struct drm_gem_shmem_object *shmem); + +static inline bool drm_gem_shmem_is_evictable(struct drm_gem_shmem_object *shmem) +{ + return (shmem->madv >= 0) && !shmem->eviction_disable_count && + shmem->base.funcs->evict && shmem->base.funcs->swap_in && + !shmem->vmap_use_count && !shmem->base.dma_buf && + !shmem->base.import_attach && shmem->sgt; +} + static inline bool drm_gem_shmem_is_purgeable(struct drm_gem_shmem_object *shmem) { - return (shmem->madv > 0) && - !shmem->vmap_use_count && shmem->sgt && - !shmem->base.dma_buf && !shmem->base.import_attach; + return (shmem->madv > 0) && !shmem->purging_disable_count && + !shmem->vmap_use_count && shmem->base.funcs->purge && + !shmem->base.dma_buf && !shmem->base.import_attach && + shmem->sgt; } -void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem); +int drm_gem_shmem_swap_in_pages_locked(struct drm_gem_shmem_object *shmem); +int drm_gem_shmem_swap_in_locked(struct drm_gem_shmem_object *shmem); + +void drm_gem_shmem_evict_locked(struct drm_gem_shmem_object *shmem); + bool drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem); +void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem); struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_shmem_object *shmem); struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem); @@ -262,6 +325,38 @@ static inline int drm_gem_shmem_object_mmap(struct drm_gem_object *obj, struct v return drm_gem_shmem_mmap(shmem, vma); } +/** + * struct drm_gem_shmem_shrinker - Generic memory shrinker for shmem GEMs + */ +struct drm_gem_shmem_shrinker { + /** @base: Shrinker for purging shmem GEM objects */ + struct shrinker base; + + /** @lock: Protects @lru_* */ + struct mutex lock; + + /** @lru_purgeable: List of shmem GEM objects available for purging */ + struct list_head lru_purgeable; + + /** @lru_active: List of active shmem GEM objects */ + struct list_head lru_active; + + /** @lru_evictable: List of shmem GEM objects that can be evicted */ + struct list_head lru_evictable; + + /** @lru_evicted: List of evicted shmem GEM objects */ + struct list_head lru_evicted; + + /** @dev: DRM device that uses this shrinker */ + struct drm_device *dev; + + /** @shrinkable_count: Count of shmem GEM pages to be purged and evicted */ + u64 shrinkable_count; +}; + +int drm_gem_shmem_shrinker_register(struct drm_device *dev); +void drm_gem_shmem_shrinker_unregister(struct drm_device *dev); + /* * Driver ops */