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 <[email protected]> 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/[email protected]?part=2
