Re: [PATCH 10/10] xe: create a flag to enable memcg accounting for XE as well.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Dumb buffer allocations bypass memory cgroup accounting.
- [High] Internal userspace-triggered buffer allocations (seqno_bo and cgp_bo) bypass memory cgroup accounting.
- [High] Page tables dynamically allocated in asynchronous contexts bypass memory cgroup accounting.
--

commit d0632a50e763992c9a7f84d8a817feb26c078e3a
Author: Maarten Lankhorst <dev@lankhorst.se>
Date:   Mon Jul 6 05:22:39 2026 +0000

    xe: create a flag to enable memcg accounting for XE as well.
    
    This adds support for memcg accounting to ttm object used by xe driver.
    
    Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
    Signed-off-by: Dave Airlie <airlied@redhat.com>

> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 62f1da04ca2b1..e016f0314d861 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c

[ ... ]

> @@ -2384,6 +2385,9 @@ struct xe_bo *xe_bo_init_locked(struct xe_device *xe, struct xe_bo *bo,
>  	placement = (type == ttm_bo_type_sg ||
>  		     bo->flags & XE_BO_FLAG_DEFER_BACKING) ? &sys_placement :
>  		&bo->placement;
> +
> +	if (bo->flags & XE_BO_FLAG_ACCOUNTED)
> +		ttm_bo_set_cgroup(&bo->ttm, get_obj_cgroup_from_current());

[Severity: High]
Will get_obj_cgroup_from_current() correctly account memory when page tables
are allocated dynamically?

Page tables are sometimes created asynchronously in worker threads, such as
during SVM page fault handling or preempt rebinds via xe_pt_create(). In these
cases, "current" refers to the kworker thread rather than the user process.

Since set_active_memcg() isn't used to restore the original user's memcg
context in the worker, will get_obj_cgroup_from_current() evaluate against
the worker's cgroup and bypass the user's memcg limits?

>  	err = ttm_bo_init_reserved(&xe->ttm, &bo->ttm, type,
>  				   placement, alignment,
>  				   &ctx, NULL, resv, xe_ttm_bo_destroy);

[ ... ]

> @@ -3361,7 +3365,7 @@ int xe_gem_create_ioctl(struct drm_device *dev, void *data,
>  	if (XE_IOCTL_DBG(xe, args->size & ~PAGE_MASK))
>  		return -EINVAL;
>  
> -	bo_flags = 0;
> +	bo_flags = XE_BO_FLAG_ACCOUNTED;

[Severity: High]
Does xe_bo_dumb_create() also need to set this flag?

When userspace calls DRM_IOCTL_MODE_CREATE_DUMB, it is handled by
xe_bo_dumb_create(), which calls xe_bo_create_user():

	bo = xe_bo_create_user(xe, NULL, args->size,
			       DRM_XE_GEM_CPU_CACHING_WC,
			       XE_BO_FLAG_VRAM_IF_DGFX(xe_device_get_root_tile(xe)) |
			       XE_BO_FLAG_FORCE_WC |
			       XE_BO_FLAG_NEEDS_CPU_ACCESS, NULL);

This appears to bypass the newly added memcg accounting because
XE_BO_FLAG_ACCOUNTED is not passed in the bitmask here.

>  	if (args->flags & DRM_XE_GEM_CREATE_FLAG_DEFER_BACKING)
>  		bo_flags |= XE_BO_FLAG_DEFER_BACKING;
>  

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
> index a4292a11391d3..61aa5033a2920 100644
> --- a/drivers/gpu/drm/xe/xe_lrc.c
> +++ b/drivers/gpu/drm/xe/xe_lrc.c
> @@ -1661,7 +1661,8 @@ static int xe_lrc_init(struct xe_lrc *lrc, struct xe_hw_engine *hwe, struct xe_v
>  		   XE_BO_FLAG_GGTT_INVALIDATE;
>  
>  	if ((vm && vm->xef) || init_flags & XE_LRC_CREATE_USER_CTX) /* userspace */
> -		bo_flags |= XE_BO_FLAG_PINNED_LATE_RESTORE | XE_BO_FLAG_FORCE_USER_VRAM;
> +		bo_flags |= XE_BO_FLAG_PINNED_LATE_RESTORE | XE_BO_FLAG_FORCE_USER_VRAM |
> +			    XE_BO_FLAG_ACCOUNTED;
>  
>  	bo = xe_bo_create_pin_map_novm(xe, tile, bo_size,
>  				       ttm_bo_type_kernel,

[Severity: High]
Should we also add XE_BO_FLAG_ACCOUNTED to the auxiliary buffers created for
userspace queues?

Just below this allocation in xe_lrc_init(), seqno_bo is allocated without
the flag:

	bo = xe_bo_create_pin_map_novm(xe, tile, PAGE_SIZE,
				       ttm_bo_type_kernel,
				       XE_BO_FLAG_GGTT |
				       XE_BO_FLAG_GGTT_INVALIDATE |
				       XE_BO_FLAG_SYSTEM, false);

Similarly, in xe_exec_queue_group_init(), cgp_bo is also allocated for
userspace queues without XE_BO_FLAG_ACCOUNTED:

	bo = xe_bo_create_pin_map_novm(xe, tile, SZ_4K, ttm_bo_type_kernel,
				       XE_BO_FLAG_VRAM_IF_DGFX(tile) |
				       XE_BO_FLAG_PINNED_LATE_RESTORE |
				       XE_BO_FLAG_FORCE_USER_VRAM |
				       XE_BO_FLAG_GGTT_INVALIDATE |
				       XE_BO_FLAG_GGTT, false);

If these are triggered by DRM_IOCTL_XE_EXEC_QUEUE_CREATE, could an application
spawn many execution queues and consume unaccounted memory?

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