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 <[email protected]>
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 <[email protected]>
    Signed-off-by: Dave Airlie <[email protected]>

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

Reply via email to