[2/3] drm/ttm: remove swap LRU v2

Instead evict round robin from each devices SYSTEM and TT domain.

v2: reorder num_pages access reported by Dan's script

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c        | 33 ++--------------
 drivers/gpu/drm/ttm/ttm_bo_util.c   |  1 -
 drivers/gpu/drm/ttm/ttm_device.c    | 60 +++++++++++++++++++++--------
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c |  2 +-
 include/drm/ttm/ttm_bo_api.h        |  1 -
 include/drm/ttm/ttm_bo_driver.h     |  1 -
 include/drm/ttm/ttm_device.h        |  7 +---
 7 files changed, 52 insertions(+), 53 deletions(-)

Hi "Christian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm-tip/drm-tip]
[cannot apply to drm-intel/for-linux-next drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next linus/master drm/drm-next v5.12-rc3 next-20210315]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Christian-K-nig/drm-ttm-move-swapout-logic-around/20210316-000551
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: i386-randconfig-s002-20210315 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-277-gc089cd2d-dirty
        # https://github.com/0day-ci/linux/commit/70ae63f3a85b9791dfcf38034c304aedda122e7b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Christian-K-nig/drm-ttm-move-swapout-logic-around/20210316-000551
        git checkout 70ae63f3a85b9791dfcf38034c304aedda122e7b
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
   drivers/gpu/drm/ttm/ttm_device.c:42:1: sparse: sparse: symbol 'ttm_global_mutex' was not declared. Should it be static?
   drivers/gpu/drm/ttm/ttm_device.c:43:10: sparse: sparse: symbol 'ttm_glob_use_count' was not declared. Should it be static?
>> drivers/gpu/drm/ttm/ttm_device.c:125:6: sparse: sparse: context imbalance in 'ttm_device_swapout' - wrong count at exit

vim +/ttm_device_swapout +125 drivers/gpu/drm/ttm/ttm_device.c

   124	
 > 125	long ttm_device_swapout(struct ttm_device *bdev, struct ttm_operation_ctx *ctx,
   126				gfp_t gfp_flags)
   127	{
   128		struct ttm_global *glob = &ttm_glob;
   129		struct ttm_resource_manager *man;
   130		struct ttm_buffer_object *bo;
   131		unsigned i, j;
   132		int ret;
   133	
   134		spin_lock(&glob->lru_lock);
   135		for (i = TTM_PL_SYSTEM; i < TTM_NUM_MEM_TYPES; ++i) {
   136			man = ttm_manager_type(bdev, i);
   137			if (!man || !man->use_tt)
   138				continue;
   139	
   140			for (j = 0; j < TTM_MAX_BO_PRIORITY; ++j) {
   141				list_for_each_entry(bo, &man->lru[j], lru) {
   142					long num_pages;
   143	
   144					if (!bo->ttm ||
   145					    bo->ttm->page_flags & TTM_PAGE_FLAG_SG ||
   146					    bo->ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)
   147						continue;
   148	
   149					num_pages = bo->ttm->num_pages;
   150					ret = ttm_bo_swapout(bo, ctx, gfp_flags);
   151					/* ttm_bo_swapout has dropped the lru_lock */
   152					if (!ret)
   153						return num_pages;
   154					if (ret != -EBUSY)
   155						return ret;
   156				}
   157			}
   158		}
   159		spin_unlock(&glob->lru_lock);
   160		return 0;
   161	}
   162	EXPORT_SYMBOL(ttm_device_swapout);
   163	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

On Mon, 15 Mar 2021 at 16:04, Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Instead evict round robin from each devices SYSTEM and TT domain.
>
> v2: reorder num_pages access reported by Dan's script
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/ttm/ttm_bo.c        | 33 ++--------------
>  drivers/gpu/drm/ttm/ttm_bo_util.c   |  1 -
>  drivers/gpu/drm/ttm/ttm_device.c    | 60 +++++++++++++++++++++--------
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c |  2 +-
>  include/drm/ttm/ttm_bo_api.h        |  1 -
>  include/drm/ttm/ttm_bo_driver.h     |  1 -
>  include/drm/ttm/ttm_device.h        |  7 +---
>  7 files changed, 52 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 56d2e38af273..a1be88be357b 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -73,7 +73,6 @@ static void ttm_bo_del_from_lru(struct ttm_buffer_object *bo)
>  {
>         struct ttm_device *bdev = bo->bdev;
>
> -       list_del_init(&bo->swap);
>         list_del_init(&bo->lru);
>
>         if (bdev->funcs->del_from_lru_notify)
> @@ -104,16 +103,6 @@ void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo,
>
>         man = ttm_manager_type(bdev, mem->mem_type);
>         list_move_tail(&bo->lru, &man->lru[bo->priority]);
> -       if (man->use_tt && bo->ttm &&
> -           !(bo->ttm->page_flags & (TTM_PAGE_FLAG_SG |
> -                                    TTM_PAGE_FLAG_SWAPPED))) {
> -               struct list_head *swap;
> -
> -               swap = &ttm_glob.swap_lru[bo->priority];
> -               list_move_tail(&bo->swap, swap);
> -       } else {
> -               list_del_init(&bo->swap);
> -       }
>
>         if (bdev->funcs->del_from_lru_notify)
>                 bdev->funcs->del_from_lru_notify(bo);
> @@ -128,9 +117,6 @@ void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo,
>                         ttm_bo_bulk_move_set_pos(&bulk->vram[bo->priority], bo);
>                         break;
>                 }
> -               if (bo->ttm && !(bo->ttm->page_flags &
> -                                (TTM_PAGE_FLAG_SG | TTM_PAGE_FLAG_SWAPPED)))
> -                       ttm_bo_bulk_move_set_pos(&bulk->swap[bo->priority], bo);
>         }
>  }
>  EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
> @@ -168,20 +154,6 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>                 list_bulk_move_tail(&man->lru[i], &pos->first->lru,
>                                     &pos->last->lru);
>         }
> -
> -       for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> -               struct ttm_lru_bulk_move_pos *pos = &bulk->swap[i];
> -               struct list_head *lru;
> -
> -               if (!pos->first)
> -                       continue;
> -
> -               dma_resv_assert_held(pos->first->base.resv);
> -               dma_resv_assert_held(pos->last->base.resv);
> -
> -               lru = &ttm_glob.swap_lru[i];
> -               list_bulk_move_tail(lru, &pos->first->swap, &pos->last->swap);
> -       }
>  }
>  EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail);
>
> @@ -1058,7 +1030,6 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
>         kref_init(&bo->kref);
>         INIT_LIST_HEAD(&bo->lru);
>         INIT_LIST_HEAD(&bo->ddestroy);
> -       INIT_LIST_HEAD(&bo->swap);
>         bo->bdev = bdev;
>         bo->type = type;
>         bo->mem.mem_type = TTM_PL_SYSTEM;
> @@ -1193,6 +1164,10 @@ int ttm_bo_swapout(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx,
>         bool locked;
>         int ret;
>
> +       if (!bo->ttm || bo->ttm->page_flags & (TTM_PAGE_FLAG_SG |
> +                                              TTM_PAGE_FLAG_SWAPPED))
> +               return false;
> +

return 0; ?

Seems inconsistent to return zero here and not drop the lru lock? Or
maybe turn this into a programmer error, since the current caller
already checks for the above?

>         if (!ttm_bo_evict_swapout_allowable(bo, ctx, &locked, NULL))
>                 return -EBUSY;
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index 031e5819fec4..a2a17c84ceb3 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -303,7 +303,6 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo,
>         atomic_inc(&ttm_glob.bo_count);
>         INIT_LIST_HEAD(&fbo->base.ddestroy);
>         INIT_LIST_HEAD(&fbo->base.lru);
> -       INIT_LIST_HEAD(&fbo->base.swap);
>         fbo->base.moving = NULL;
>         drm_vma_node_reset(&fbo->base.base.vma_node);
>
> diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c
> index dfc2a7e4e490..2c280fb1e992 100644
> --- a/drivers/gpu/drm/ttm/ttm_device.c
> +++ b/drivers/gpu/drm/ttm/ttm_device.c
> @@ -67,7 +67,6 @@ static int ttm_global_init(void)
>         unsigned long num_pages;
>         struct sysinfo si;
>         int ret = 0;
> -       unsigned i;
>
>         mutex_lock(&ttm_global_mutex);
>         if (++ttm_glob_use_count > 1)
> @@ -90,8 +89,6 @@ static int ttm_global_init(void)
>                 goto out;
>         }
>
> -       for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i)
> -               INIT_LIST_HEAD(&glob->swap_lru[i]);
>         INIT_LIST_HEAD(&glob->device_list);
>         atomic_set(&glob->bo_count, 0);
>
> @@ -109,27 +106,60 @@ static int ttm_global_init(void)
>  long ttm_global_swapout(struct ttm_operation_ctx *ctx, gfp_t gfp_flags)
>  {
>         struct ttm_global *glob = &ttm_glob;
> +       struct ttm_device *bdev;
> +       int ret = -EBUSY;
> +
> +       mutex_lock(&ttm_global_mutex);
> +       list_for_each_entry(bdev, &glob->device_list, device_list) {
> +               ret = ttm_device_swapout(bdev, ctx, gfp_flags);

Mixing int and long for num_pages.

Does ttm enforce a maximum page count somewhere for object sizes?
Something like INT_MAX, since it doesn't look like ttm is consistently
using the same type(unsigned long?) when representing the number of
pages for an object?

> +               if (ret > 0) {
> +                       list_move_tail(&bdev->device_list, &glob->device_list);
> +                       break;
> +               }
> +       }
> +       mutex_unlock(&ttm_global_mutex);
> +       return ret;
> +}
> +EXPORT_SYMBOL(ttm_global_swapout);
> +
> +long ttm_device_swapout(struct ttm_device *bdev, struct ttm_operation_ctx *ctx,
> +                       gfp_t gfp_flags)
> +{
> +       struct ttm_global *glob = &ttm_glob;
> +       struct ttm_resource_manager *man;
>         struct ttm_buffer_object *bo;
> -       unsigned i;
> +       unsigned i, j;
>         int ret;
>
>         spin_lock(&glob->lru_lock);
> -       for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> -               list_for_each_entry(bo, &glob->swap_lru[i], swap) {
> -                       uint32_t num_pages = bo->ttm->num_pages;
> -
> -                       ret = ttm_bo_swapout(bo, ctx, gfp_flags);
> -                       /* ttm_bo_swapout has dropped the lru_lock */
> -                       if (!ret)
> -                               return num_pages;
> -                       if (ret != -EBUSY)
> -                               return ret;
> +       for (i = TTM_PL_SYSTEM; i < TTM_NUM_MEM_TYPES; ++i) {
> +               man = ttm_manager_type(bdev, i);
> +               if (!man || !man->use_tt)
> +                       continue;
> +
> +               for (j = 0; j < TTM_MAX_BO_PRIORITY; ++j) {
> +                       list_for_each_entry(bo, &man->lru[j], lru) {
> +                               long num_pages;
> +
> +                               if (!bo->ttm ||
> +                                   bo->ttm->page_flags & TTM_PAGE_FLAG_SG ||
> +                                   bo->ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)
> +                                       continue;
> +
> +                               num_pages = bo->ttm->num_pages;
> +                               ret = ttm_bo_swapout(bo, ctx, gfp_flags);
> +                               /* ttm_bo_swapout has dropped the lru_lock */
> +                               if (!ret)
> +                                       return num_pages;
> +                               if (ret != -EBUSY)
> +                                       return ret;
> +                       }
>                 }
>         }
>         spin_unlock(&glob->lru_lock);
>         return 0;
>  }
> -EXPORT_SYMBOL(ttm_global_swapout);
> +EXPORT_SYMBOL(ttm_device_swapout);
>
>  static void ttm_init_sysman(struct ttm_device *bdev)
>  {
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index b991422e156c..0e82b0662d9e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1371,7 +1371,7 @@ static int vmw_pm_freeze(struct device *kdev)
>         vmw_execbuf_release_pinned_bo(dev_priv);
>         vmw_resource_evict_all(dev_priv);
>         vmw_release_device_early(dev_priv);
> -       while (ttm_global_swapout(&ctx, GFP_KERNEL) > 0);
> +       while (ttm_device_swapout(&dev_priv->bdev, &ctx, GFP_KERNEL) == 0);

Is this the intended behaviour? ttm_device_swapout() still just
returns num_pages if it swapped something out. I assume this wants to
keep swapping stuff out, until it can't anymore. Or am I missing
something?

>         if (dev_priv->enable_fb)
>                 vmw_fifo_resource_dec(dev_priv);
>         if (atomic_read(&dev_priv->num_fifo_resources) != 0) {
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 5044ac330858..3587f660e8f4 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -144,7 +144,6 @@ struct ttm_buffer_object {
>
>         struct list_head lru;
>         struct list_head ddestroy;
> -       struct list_head swap;
>
>         /**
>          * Members protected by a bo reservation.
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 8959c0075cfd..d007feef7676 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -69,7 +69,6 @@ struct ttm_lru_bulk_move_pos {
>  struct ttm_lru_bulk_move {
>         struct ttm_lru_bulk_move_pos tt[TTM_MAX_BO_PRIORITY];
>         struct ttm_lru_bulk_move_pos vram[TTM_MAX_BO_PRIORITY];
> -       struct ttm_lru_bulk_move_pos swap[TTM_MAX_BO_PRIORITY];
>  };
>
>  /*
> diff --git a/include/drm/ttm/ttm_device.h b/include/drm/ttm/ttm_device.h
> index 6a0b267d4fe6..cda6efb4c34b 100644
> --- a/include/drm/ttm/ttm_device.h
> +++ b/include/drm/ttm/ttm_device.h
> @@ -63,11 +63,6 @@ extern struct ttm_global {
>          */
>         struct list_head device_list;
>
> -       /**
> -        * Protected by the lru_lock.
> -        */
> -       struct list_head swap_lru[TTM_MAX_BO_PRIORITY];
> -
>         /**
>          * Internal protection.
>          */
> @@ -298,6 +293,8 @@ struct ttm_device {
>  };
>
>  long ttm_global_swapout(struct ttm_operation_ctx *ctx, gfp_t gfp_flags);
> +long ttm_device_swapout(struct ttm_device *bdev, struct ttm_operation_ctx *ctx,
> +                      gfp_t gfp_flags);
>
>  static inline struct ttm_resource_manager *
>  ttm_manager_type(struct ttm_device *bdev, int mem_type)
> --
> 2.25.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Am 15.03.21 um 19:54 schrieb Matthew Auld:
> On Mon, 15 Mar 2021 at 16:04, Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>> [SNIP]
>> @@ -1193,6 +1164,10 @@ int ttm_bo_swapout(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx,
>>          bool locked;
>>          int ret;
>>
>> +       if (!bo->ttm || bo->ttm->page_flags & (TTM_PAGE_FLAG_SG |
>> +                                              TTM_PAGE_FLAG_SWAPPED))
>> +               return false;
>> +
> return 0; ?
>
> Seems inconsistent to return zero here and not drop the lru lock? Or
> maybe turn this into a programmer error, since the current caller
> already checks for the above?

Thanks, that is just an artifact from rebasing and should be removed.

>> [SNIP]
>>
>> @@ -109,27 +106,60 @@ static int ttm_global_init(void)
>>   long ttm_global_swapout(struct ttm_operation_ctx *ctx, gfp_t gfp_flags)
>>   {
>>          struct ttm_global *glob = &ttm_glob;
>> +       struct ttm_device *bdev;
>> +       int ret = -EBUSY;
>> +
>> +       mutex_lock(&ttm_global_mutex);
>> +       list_for_each_entry(bdev, &glob->device_list, device_list) {
>> +               ret = ttm_device_swapout(bdev, ctx, gfp_flags);
> Mixing int and long for num_pages.
>
> Does ttm enforce a maximum page count somewhere for object sizes?

We should use 32 bit values for the number of pages in TTM, even signed 
values allow for 8TB large BOs.

And I really hope that we can get rid of the BO approach in general 
before we ever come close to that limit.

> Something like INT_MAX, since it doesn't look like ttm is consistently
> using the same type(unsigned long?) when representing the number of
> pages for an object?

I should probably add a check for that in the tt code, yes.

> [SNIP]
>   static void ttm_init_sysman(struct ttm_device *bdev)
>   {
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index b991422e156c..0e82b0662d9e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1371,7 +1371,7 @@ static int vmw_pm_freeze(struct device *kdev)
>          vmw_execbuf_release_pinned_bo(dev_priv);
>          vmw_resource_evict_all(dev_priv);
>          vmw_release_device_early(dev_priv);
> -       while (ttm_global_swapout(&ctx, GFP_KERNEL) > 0);
> +       while (ttm_device_swapout(&dev_priv->bdev, &ctx, GFP_KERNEL) == 0);
> Is this the intended behaviour? ttm_device_swapout() still just
> returns num_pages if it swapped something out. I assume this wants to
> keep swapping stuff out, until it can't anymore. Or am I missing
> something?

Indeed that's a mix up. Thanks for pointing that out.

Christian.