Re: [PATCH] drm/xe: Keep ulls alive while servicing ops.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] drm/xe: ulls_used refcount leak on error in vm_bind_ioctl_ops_create()
- [Medium] drm/xe: premature xe_migrate_ulls_leave() in vm_bind_ioctl_ops_create()
- [Medium] drm/xe: ULLS exits while ulls_used > 0 in xe_migrate_ulls_exit()
- [Medium] drm/xe: race between xe_migrate_ulls_leave() and xe_migrate_ulls_enter()
- [Medium] drm/xe: concurrent xe_migrate_ulls_enter() returns before ULLS enabled
--

commit 12e9880121b0cd4ae8ab769d216e3524d057e97e
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/xe: Keep ulls alive while servicing ops.

This commit introduces an atomic ulls_used reference counter and a paired
xe_migrate_ulls_leave() helper to keep ULLS mode active across pagefault and
SVM prefetch operations.

> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> index 19c37eb315893..308bc52e1be52 100644
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c

[ ... ]

> @@ -692,6 +696,12 @@ void xe_migrate_ulls_enter(struct xe_migrate *m)
>  	if (!IS_DGFX(xe) || !m->ulls.exit_ms)
>  		return;
>  
> +	/* Bump */
> +	if (atomic_inc_return(&m->ulls_used) > 1)
> +		return;
> +
> +	cancel_delayed_work_sync(&m->ulls.exit_work);
> +
>  job_alloc:
>  	if (alloc) {
>  		/*

[Severity: Medium]
What happens if a second caller enters xe_migrate_ulls_enter() in
drivers/gpu/drm/xe/xe_migrate.c while the first caller is still transitioning
m->ulls_used from 0 to 1?

The first caller runs cancel_delayed_work_sync(&m->ulls.exit_work) and
allocates the scheduler job via xe_sched_job_create() outside m->job_mutex
before acquiring m->job_mutex to push the ULLS_ENTER job and set
m->ulls.enabled = true:

drivers/gpu/drm/xe/xe_migrate.c:xe_migrate_ulls_enter() {
	if (atomic_inc_return(&m->ulls_used) > 1)
		return;

	cancel_delayed_work_sync(&m->ulls.exit_work);

job_alloc:
	if (alloc) {
		job = xe_sched_job_create(m->q, batch_addr);
		if (WARN_ON_ONCE(IS_ERR(job)))
			return;		/* Not fatal */
	}

	mutex_lock(&m->job_mutex);
	if (!xe_migrate_ulls_enabled(m)) {
		if (!job) {
			alloc = true;
			mutex_unlock(&m->job_mutex);
			goto job_alloc;
		}
		...
		xe_migrate_ulls_toggle_enable(m, true);
	}
}

During that window (or if xe_sched_job_create() fails and returns early), a
concurrent caller of xe_migrate_ulls_enter() sees
atomic_inc_return(&m->ulls_used) > 1 and returns immediately before ULLS is
actually enabled. Could that concurrent caller then acquire m->job_mutex in
xe_migrate_job_push() first, see xe_migrate_ulls_enabled(m) == false, and
submit its migration jobs with ULLS_NONE instead of ULLS_ACTIVE?

> @@ -725,10 +735,30 @@ void xe_migrate_ulls_enter(struct xe_migrate *m)
>  	}
>  	if (job)
>  		xe_sched_job_put(job);
> +	mutex_unlock(&m->job_mutex);
> +}
> +
> +/**
> + * xe_migrate_ulls_leave() - Leave ULLS mode
> + * @m: The migration context.
> + *
> + * Leaves the critical part of the migration context, it may continue to be enabled.
> + * Pairs with xe_migrate_ulls_enter().
> + */
> +void xe_migrate_ulls_leave(struct xe_migrate *m)
> +{
> +	struct xe_device *xe = tile_to_xe(m->tile);
> +
> +	if (!IS_DGFX(xe) || !m->ulls.exit_ms)
> +		return;
> +
> +	if (atomic_dec_return(&m->ulls_used))
> +		return;
> +
> +	guard(mutex)(&m->job_mutex);
>  	if (xe_migrate_ulls_enabled(m))
>  		mod_delayed_work(system_percpu_wq, &m->ulls.exit_work,
>  				 ULLS_EXIT_JIFFIES(m));
> -	mutex_unlock(&m->job_mutex);
>  }

[Severity: Medium]
Can m->ulls.exit_work still fire and disable ULLS while m->ulls_used is
non-zero?

In drivers/gpu/drm/xe/xe_migrate.c, xe_migrate_job_push() unconditionally
re-arms m->ulls.exit_work whenever a migration job is pushed:

drivers/gpu/drm/xe/xe_migrate.c:xe_migrate_job_push() {
	if (xe_migrate_ulls_enabled(m)) {
		ulls = ULLS_ACTIVE;
		mod_delayed_work(system_percpu_wq, &m->ulls.exit_work,
				 ULLS_EXIT_JIFFIES(m));
	}
}

And when the worker runs, xe_migrate_ulls_exit() only checks whether m->q is
idle without checking m->ulls_used:

drivers/gpu/drm/xe/xe_migrate.c:xe_migrate_ulls_exit() {
	scoped_guard(mutex, &m->job_mutex) {
		if (xe_exec_queue_is_idle(m->q, 1)) {
			fence = __xe_migrate_job_push(m, job, ULLS_EXIT);
			dma_fence_put(fence);

			xe_pm_runtime_put(xe);	/* Pairs with PM get in enter */
			xe_migrate_ulls_toggle_enable(m, false);
			...
		}
	}
}

If a migration job finishes on the GPU while the CPU is still servicing an
operation (for example, waiting on CPU page faults or locks in
xe_pagefault_service()) for longer than exit_ms before submitting the next
migration job, won't xe_migrate_ulls_exit() exit ULLS mode while
m->ulls_used > 0? Once m->ulls.enabled is false while m->ulls_used > 0, both
the remaining jobs in the current operation and any overlapping callers of
xe_migrate_ulls_enter() (which return early on
atomic_inc_return(&m->ulls_used) > 1) will run without ULLS enabled.

Also, in xe_migrate_ulls_leave(), atomic_dec_return(&m->ulls_used) happens
before acquiring m->job_mutex, and m->ulls_used is not re-checked under
m->job_mutex:

CPU0                                     CPU1
xe_migrate_ulls_leave(m)
  atomic_dec_return(&m->ulls_used) == 0
                                         xe_migrate_ulls_enter(m)
                                           atomic_inc_return(&m->ulls_used) == 1
                                           cancel_delayed_work_sync(&m->ulls.exit_work)
                                           mutex_lock(&m->job_mutex)
                                           /* sees m->ulls.enabled == true */
                                           mutex_unlock(&m->job_mutex)
  guard(mutex)(&m->job_mutex);
  mod_delayed_work(..., &m->ulls.exit_work, ...);

Could this race leave m->ulls.exit_work armed while CPU1 holds an active
m->ulls_used reference?

> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index dfbabfb19cbba..aac89b3714791 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -2603,7 +2603,7 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_vma_ops *vops,
>  				if (addr)
>  					goto alloc_next_range;
>  				else
> -					goto print_op_label;
> +					goto ulls_leave;
>  			}
>  
>  			if (IS_ERR(svm_range)) {
> @@ -2649,8 +2649,10 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_vma_ops *vops,
>  			if (need_put)
>  				xe_svm_range_put(svm_range);
>  
> +ulls_leave:
> +			for_each_tile(tile, vm->xe, id)
> +				xe_migrate_ulls_leave(tile->migrate);
>  		}
> -print_op_label:
>  		print_op(vm->xe, __op);
>  	}

[Severity: High]
Does vm_bind_ioctl_ops_create() in drivers/gpu/drm/xe/xe_vm.c leak the
tile->migrate->ulls_used reference count on all tiles if an error occurs
after entering ULLS?

When handling DRM_GPUVA_OP_PREFETCH on a CPU address mirror VMA,
vm_bind_ioctl_ops_create() calls xe_migrate_ulls_enter(tile->migrate) for
each tile before the alloc_next_range loop:

drivers/gpu/drm/xe/xe_vm.c:vm_bind_ioctl_ops_create() {
			for_each_tile(tile, vm->xe, id) {
				xe_migrate_ulls_enter(tile->migrate);
				tile_mask |= 0x1 << id;
			}
			...
alloc_next_range:
			svm_range = xe_svm_range_find_or_insert(vm, addr, vma, &ctx);
			...
			if (IS_ERR(svm_range)) {
				err = PTR_ERR(svm_range);
				goto unwind_prefetch_ops;
			}
			...
			err = xa_alloc(&op->prefetch_range.range,
				       &i, svm_range, xa_limit_32b,
				       GFP_KERNEL);

			if (err) {
				xe_svm_range_put(svm_range);
				goto unwind_prefetch_ops;
			}
}

If xe_svm_range_find_or_insert() returns an error other than -ENOENT or if
xa_alloc() fails, execution jumps to unwind_prefetch_ops, bypassing the
ulls_leave label. Because unwind_prefetch_ops does not call
xe_migrate_ulls_leave(), won't tile->migrate->ulls_used remain permanently
elevated on every tile, preventing m->ulls.exit_work from being scheduled and
leaking the runtime PM reference acquired in xe_migrate_ulls_enter()?

[Severity: Medium]
Is vm_bind_ioctl_ops_create() in drivers/gpu/drm/xe/xe_vm.c calling
xe_migrate_ulls_leave() before the actual prefetch migrations and page-table
binds take place?

In vm_bind_ioctl_ops_create(), the DRM_GPUVA_OP_PREFETCH loop only looks up
or inserts CPU-side xe_svm_range structures and records them in
op->prefetch_range.range before jumping to ulls_leave and dropping
tile->migrate->ulls_used back to 0.

The actual SVM prefetch work (migrating pages to device memory and updating
page tables) happens later in xe_vm_bind_ioctl():

drivers/gpu/drm/xe/xe_vm.c:xe_vm_bind_ioctl() {
	...
	err = vm_bind_ioctl_ops_prefetch_ranges(vm, &vops);
	if (err)
		goto unwind_ops;

	fence = vm_bind_ioctl_ops_execute(vm, &vops);
	...
}

Should the ULLS reference be held across vm_bind_ioctl_ops_prefetch_ranges()
and vm_bind_ioctl_ops_execute() rather than being released at the end of
vm_bind_ioctl_ops_create()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102185834.136370-2-dev@lankhorst.se?part=1