Re: [PATCH 02/10] ttm: add a memcg accounting flag to the alloc/populate APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added `memcg_account` flag is not propagated to the TT restore path, causing GPU memory to escape memory cgroup limits after being swapped out and restored.
--

commit 915e66ed2150a0401b7163c79a1f518da0005591
Author: Dave Airlie <airlied@redhat.com>

    ttm: add a memcg accounting flag to the alloc/populate APIs
    
This commit adds a memcg_account flag to the TTM populate and allocate APIs
to prepare for future memcg accounting. It updates driver callbacks to
accept the new parameter.

> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index 278bbe7a11add..e4dbf4c93091c 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
[ ... ]
> @@ -935,7 +937,7 @@ int ttm_pool_restore_and_alloc(struct ttm_pool *pool, struct ttm_tt *tt,
>  			return 0;
>  	}
>  
> -	return __ttm_pool_alloc(pool, tt, ctx, &alloc, restore);
> +	return __ttm_pool_alloc(pool, tt, false, ctx, &alloc, restore);

[Severity: High]
Does this hardcoded false cause memory cgroup limits to be bypassed for
restored pages?

When a TT object is swapped out (backed up), its pages are freed and
uncharged from the memcg. When swapped back in, the pages are re-allocated,
but because ttm_pool_restore_and_alloc() hardcodes false for the
memcg_account parameter, they are not re-charged. 

This causes a bypass of memory cgroup limits for restored pages, which
could allow an application to allocate GPU memory, trigger swap, and bypass
the restrictions upon restore.

>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 4c80bac676229..20a10a174d1d3 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
[ ... ]
> @@ -536,6 +536,7 @@ static struct ttm_tt *xe_ttm_tt_create(struct ttm_buffer_object *ttm_bo,
>  }
>  
>  static int xe_ttm_tt_populate(struct ttm_device *ttm_dev, struct ttm_tt *tt,
> +			      bool memcg_account,
>  			      struct ttm_operation_ctx *ctx)
>  {
>  	struct xe_ttm_tt *xe_tt = container_of(tt, struct xe_ttm_tt, ttm);
> @@ -553,7 +554,7 @@ static int xe_ttm_tt_populate(struct ttm_device *ttm_dev, struct ttm_tt *tt,
>  		err = ttm_tt_restore(ttm_dev, tt, ctx);

[Severity: High]
Will we need to update ttm_tt_restore() to accept the memcg_account
parameter as well?

Since ttm_tt_restore() doesn't accept the new memcg_account parameter,
drivers like xe here receive the flag but cannot pass it down when
restoring a backed-up TT object, losing the accounting information.

>  	} else {
>  		ttm_tt_clear_backed_up(tt);
> -		err = ttm_pool_alloc(&ttm_dev->pool, tt, ctx);
> +		err = ttm_pool_alloc(&ttm_dev->pool, tt, memcg_account, ctx);
>  	}
>  	if (err)
>  		return err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706024122.853329-1-airlied@gmail.com?part=2