RE: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout.

From: He, Roger
Date: Fri Feb 23 2018 - 22:37:06 EST




-----Original Message-----
From: Christian KÃnig [mailto:ckoenig.leichtzumerken@xxxxxxxxx]
Sent: Friday, February 23, 2018 8:06 PM
To: He, Roger <Hongbo.He@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout.

Am 23.02.2018 um 10:46 schrieb He, Roger:
>
> -----Original Message-----
> From: dri-devel [mailto:dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx] On
> Behalf Of Christian K?nig
> Sent: Tuesday, February 20, 2018 8:58 PM
> To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout.
>
> This solves the problem that when we swapout a BO from a domain we sometimes couldn't make room for it because holding the lock blocks all other BOs with this reservation object.
>
> Signed-off-by: Christian KÃnig <christian.koenig@xxxxxxx>
> ---
> drivers/gpu/drm/ttm/ttm_bo.c | 33 ++++++++++++++++-----------------
> 1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
> b/drivers/gpu/drm/ttm/ttm_bo.c index d90b1cf10b27..3a44c2ee4155 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -713,31 +713,30 @@ bool ttm_bo_eviction_valuable(struct
> ttm_buffer_object *bo, EXPORT_SYMBOL(ttm_bo_eviction_valuable);
>
> /**
> - * Check the target bo is allowable to be evicted or swapout, including cases:
> - *
> - * a. if share same reservation object with ctx->resv, have
> assumption
> - * reservation objects should already be locked, so not lock again
> and
> - * return true directly when either the opreation
> allow_reserved_eviction
> - * or the target bo already is in delayed free list;
> - *
> - * b. Otherwise, trylock it.
> + * Check if the target bo is allowed to be evicted or swapedout.
> */
> static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
> - struct ttm_operation_ctx *ctx, bool *locked)
> + struct ttm_operation_ctx *ctx,
> + bool *locked)
> {
> - bool ret = false;
> + /* First check if we can lock it */
> + *locked = reservation_object_trylock(bo->resv);
> + if (*locked)
> + return true;
>
> - *locked = false;
> + /* Check if it's locked because it is part of the current operation
> +*/
> if (bo->resv == ctx->resv) {
> reservation_object_assert_held(bo->resv);
> - if (ctx->allow_reserved_eviction || !list_empty(&bo->ddestroy))
> - ret = true;
> - } else {
> - *locked = reservation_object_trylock(bo->resv);
> - ret = *locked;
> + return ctx->allow_reserved_eviction ||
> + !list_empty(&bo->ddestroy);
> }
>
> - return ret;
> + /* Check if it's locked because it was already evicted */
> + if (ww_mutex_is_owned_by(&bo->resv->lock, NULL))
> + return true;
>
> For the special case: when command submission with Per-VM-BO enabled,
> All BOs a/b/c are always valid BO. After the validation of BOs a and
> b, when validation of BO c, is it possible to return true and then evict BO a and b by mistake ?
> Because a/b/c share same task_struct.

No, that's why I check the context as well. BOs explicitly reserved have a non NULL context while BOs trylocked for swapout have a NULL context.

When BOs have a non NULL context only when command submission and reserved by ttm_eu_re6serve_buffers .
But for Per-VM-BO a/b/c they always are not in BO list, so they will be not reserved and have always NULL context.
So above case also can happen. Anything missing here?

Thanks
Roger(Hongbo.He)
>
> + /* Some other thread is using it, don't touch it */
> + return false;
> }
>
> static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
> --
> 2.14.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel