[PATCH] drm/xe/pxp: Don't kill queues while holding the spinlock

[PATCH] drm/xe/pxp: Don't kill queues while holding the spinlock

[Daniele Ceraolo Spurio]

xe_exec_queue_kill can sleep, so we can't call it from under the lock.
We can instead move the queues to a separate list and then kill them all
after we release the lock.

Since being in the list is used to track whether RPM cleanup is needed,
we can no longer defer that to queue_destroy, so we perform it
immediately instead.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes: f8caa80154c4 ("drm/xe/pxp: Add PXP queue tracking and session start")
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/xe/xe_pxp.c | 58 ++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_pxp.c b/drivers/gpu/drm/xe/xe_pxp.c
index 3cd3f83e86b0..bca2872ba07a 100644
--- a/drivers/gpu/drm/xe/xe_pxp.c
+++ b/drivers/gpu/drm/xe/xe_pxp.c
@@ -665,23 +665,15 @@ int xe_pxp_exec_queue_add(struct xe_pxp *pxp, struct xe_exec_queue *q)
 	return ret;
 }
 
-/**
- * xe_pxp_exec_queue_remove - remove a queue from the PXP list
- * @pxp: the xe->pxp pointer (it will be NULL if PXP is disabled)
- * @q: the queue to remove from the list
- *
- * If PXP is enabled and the exec_queue is in the list, the queue will be
- * removed from the list and its PM reference will be released. It is safe to
- * call this function multiple times for the same queue.
- */
-void xe_pxp_exec_queue_remove(struct xe_pxp *pxp, struct xe_exec_queue *q)
+static void __pxp_exec_queue_remove(struct xe_pxp *pxp, struct xe_exec_queue *q, bool lock)
 {
 	bool need_pm_put = false;
 
 	if (!xe_pxp_is_enabled(pxp))
 		return;
 
-	spin_lock_irq(&pxp->queues.lock);
+	if (lock)
+		spin_lock_irq(&pxp->queues.lock);
 
 	if (!list_empty(&q->pxp.link)) {
 		list_del_init(&q->pxp.link);
@@ -690,36 +682,54 @@ void xe_pxp_exec_queue_remove(struct xe_pxp *pxp, struct xe_exec_queue *q)
 
 	q->pxp.type = DRM_XE_PXP_TYPE_NONE;
 
-	spin_unlock_irq(&pxp->queues.lock);
+	if (lock)
+		spin_unlock_irq(&pxp->queues.lock);
 
 	if (need_pm_put)
 		xe_pm_runtime_put(pxp->xe);
 }
 
+/**
+ * xe_pxp_exec_queue_remove - remove a queue from the PXP list
+ * @pxp: the xe->pxp pointer (it will be NULL if PXP is disabled)
+ * @q: the queue to remove from the list
+ *
+ * If PXP is enabled and the exec_queue is in the list, the queue will be
+ * removed from the list and its PM reference will be released. It is safe to
+ * call this function multiple times for the same queue.
+ */
+void xe_pxp_exec_queue_remove(struct xe_pxp *pxp, struct xe_exec_queue *q)
+{
+	__pxp_exec_queue_remove(pxp, q, true);
+}
+
 static void pxp_invalidate_queues(struct xe_pxp *pxp)
 {
 	struct xe_exec_queue *tmp, *q;
+	LIST_HEAD(to_clean);
 
 	spin_lock_irq(&pxp->queues.lock);
 
-	/*
-	 * Removing a queue from the PXP list requires a put of the RPM ref that
-	 * the queue holds to keep the PXP session alive, which can't be done
-	 * under spinlock. Since it is safe to kill a queue multiple times, we
-	 * can leave the invalid queue in the list for now and postpone the
-	 * removal and associated RPM put to when the queue is destroyed.
-	 */
-	list_for_each_entry(tmp, &pxp->queues.list, pxp.link) {
-		q = xe_exec_queue_get_unless_zero(tmp);
-
+	list_for_each_entry_safe(q, tmp, &pxp->queues.list, pxp.link) {
+		q = xe_exec_queue_get_unless_zero(q);
 		if (!q)
 			continue;
 
+		list_move_tail(&q->pxp.link, &to_clean);
+	}
+	spin_unlock_irq(&pxp->queues.lock);
+
+	list_for_each_entry_safe(q, tmp, &to_clean, pxp.link) {
 		xe_exec_queue_kill(q);
+
+		/*
+		 * We hold a ref to the queue so there is no risk of racing with
+		 * the calls to exec_queue_remove coming from exec_queue_destroy.
+		 */
+		__pxp_exec_queue_remove(pxp, q, false);
+
 		xe_exec_queue_put(q);
 	}
-
-	spin_unlock_irq(&pxp->queues.lock);
 }
 
 /**
-- 
2.43.0

✓ CI.Patch_applied: success for drm/xe/pxp: Don't kill queues while holding the spinlock

[Patchwork]

== Series Details ==

Series: drm/xe/pxp: Don't kill queues while holding the spinlock
URL   : https://patchwork.freedesktop.org/series/144763/
State : success

== Summary ==

=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: fd03884f35d1 drm-tip: 2025y-02m-12d-22h-21m-23s UTC integration manifest
=== git am output follows ===
Applying: drm/xe/pxp: Don't kill queues while holding the spinlock

✗ CI.checkpatch: warning for drm/xe/pxp: Don't kill queues while holding the spinlock

[Patchwork]

== Series Details ==

Series: drm/xe/pxp: Don't kill queues while holding the spinlock
URL   : https://patchwork.freedesktop.org/series/144763/
State : warning

== Summary ==

+ KERNEL=/kernel
+ git clone https://gitlab.freedesktop.org/drm/maintainer-tools mt
Cloning into 'mt'...
warning: redirecting to https://gitlab.freedesktop.org/drm/maintainer-tools.git/
+ git -C mt rev-list -n1 origin/master
22f9cda3436b4fe965b5c5f31d2f2c1bcb483189
+ cd /kernel
+ git config --global --add safe.directory /kernel
+ git log -n1
commit 9596ea6bddfed5614c3bee87cb133c0b63462407
Author: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Date:   Wed Feb 12 16:40:32 2025 -0800

    drm/xe/pxp: Don't kill queues while holding the spinlock
    
    xe_exec_queue_kill can sleep, so we can't call it from under the lock.
    We can instead move the queues to a separate list and then kill them all
    after we release the lock.
    
    Since being in the list is used to track whether RPM cleanup is needed,
    we can no longer defer that to queue_destroy, so we perform it
    immediately instead.
    
    Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
    Fixes: f8caa80154c4 ("drm/xe/pxp: Add PXP queue tracking and session start")
    Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
    Cc: John Harrison <John.C.Harrison@Intel.com>
+ /mt/dim checkpatch fd03884f35d18372b0f7abb2fd33c55cee599bc3 drm-intel
9596ea6bddfe drm/xe/pxp: Don't kill queues while holding the spinlock
-:14: WARNING:BAD_REPORTED_BY_LINK: Reported-by: should be immediately followed by Closes: with a URL to the report
#14: 
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes: f8caa80154c4 ("drm/xe/pxp: Add PXP queue tracking and session start")

total: 0 errors, 1 warnings, 0 checks, 93 lines checked

✗ CI.KUnit: failure for drm/xe/pxp: Don't kill queues while holding the spinlock

[Patchwork]

== Series Details ==

Series: drm/xe/pxp: Don't kill queues while holding the spinlock
URL   : https://patchwork.freedesktop.org/series/144763/
State : failure

== Summary ==

+ trap cleanup EXIT
+ /kernel/tools/testing/kunit/kunit.py run --kunitconfig /kernel/drivers/gpu/drm/xe/.kunitconfig
ERROR:root:../scripts/Makefile.build:41: ../drivers/gpu/drm/i2c/Makefile: No such file or directory
make[7]: *** No rule to make target '../drivers/gpu/drm/i2c/Makefile'.  Stop.
make[6]: *** [../scripts/Makefile.build:465: drivers/gpu/drm/i2c] Error 2
make[6]: *** Waiting for unfinished jobs....
../lib/iomap.c:156:5: warning: no previous prototype for ‘ioread64_lo_hi’ [-Wmissing-prototypes]
  156 | u64 ioread64_lo_hi(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~
../lib/iomap.c:163:5: warning: no previous prototype for ‘ioread64_hi_lo’ [-Wmissing-prototypes]
  163 | u64 ioread64_hi_lo(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~
../lib/iomap.c:170:5: warning: no previous prototype for ‘ioread64be_lo_hi’ [-Wmissing-prototypes]
  170 | u64 ioread64be_lo_hi(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~~~
../lib/iomap.c:178:5: warning: no previous prototype for ‘ioread64be_hi_lo’ [-Wmissing-prototypes]
  178 | u64 ioread64be_hi_lo(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~~~
../lib/iomap.c:264:6: warning: no previous prototype for ‘iowrite64_lo_hi’ [-Wmissing-prototypes]
  264 | void iowrite64_lo_hi(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~
../lib/iomap.c:272:6: warning: no previous prototype for ‘iowrite64_hi_lo’ [-Wmissing-prototypes]
  272 | void iowrite64_hi_lo(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~
../lib/iomap.c:280:6: warning: no previous prototype for ‘iowrite64be_lo_hi’ [-Wmissing-prototypes]
  280 | void iowrite64be_lo_hi(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~~~
../lib/iomap.c:288:6: warning: no previous prototype for ‘iowrite64be_hi_lo’ [-Wmissing-prototypes]
  288 | void iowrite64be_hi_lo(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~~~
make[5]: *** [../scripts/Makefile.build:465: drivers/gpu/drm] Error 2
make[4]: *** [../scripts/Makefile.build:465: drivers/gpu] Error 2
make[3]: *** [../scripts/Makefile.build:465: drivers] Error 2
make[2]: *** [/kernel/Makefile:1994: .] Error 2
make[1]: *** [/kernel/Makefile:251: __sub-make] Error 2
make: *** [Makefile:251: __sub-make] Error 2

[00:47:39] Configuring KUnit Kernel ...
Generating .config ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
[00:47:43] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
Building with:
$ make all compile_commands.json ARCH=um O=.kunit --jobs=48
+ cleanup
++ stat -c %u:%g /kernel
+ chown -R 1003:1003 /kernel

Re: [PATCH] drm/xe/pxp: Don't kill queues while holding the spinlock

[Matthew Brost]

On Wed, Feb 12, 2025 at 04:40:32PM -0800, Daniele Ceraolo Spurio wrote:
> xe_exec_queue_kill can sleep, so we can't call it from under the lock.
> We can instead move the queues to a separate list and then kill them all
> after we release the lock.
> 
> Since being in the list is used to track whether RPM cleanup is needed,
> we can no longer defer that to queue_destroy, so we perform it
> immediately instead.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: f8caa80154c4 ("drm/xe/pxp: Add PXP queue tracking and session start")
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Patch LGTM but can this actually happen though? i.e. Can or do we enable
PXP on LR queues?

Also as a follow should be add a might_sleep() to xe_exec_queue_kill to
catch this type of bug immediately?

Anyways patch does LGTM:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> Cc: John Harrison <John.C.Harrison@Intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pxp.c | 58 ++++++++++++++++++++++---------------
>  1 file changed, 34 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pxp.c b/drivers/gpu/drm/xe/xe_pxp.c
> index 3cd3f83e86b0..bca2872ba07a 100644
> --- a/drivers/gpu/drm/xe/xe_pxp.c
> +++ b/drivers/gpu/drm/xe/xe_pxp.c
> @@ -665,23 +665,15 @@ int xe_pxp_exec_queue_add(struct xe_pxp *pxp, struct xe_exec_queue *q)
>  	return ret;
>  }
>  
> -/**
> - * xe_pxp_exec_queue_remove - remove a queue from the PXP list
> - * @pxp: the xe->pxp pointer (it will be NULL if PXP is disabled)
> - * @q: the queue to remove from the list
> - *
> - * If PXP is enabled and the exec_queue is in the list, the queue will be
> - * removed from the list and its PM reference will be released. It is safe to
> - * call this function multiple times for the same queue.
> - */
> -void xe_pxp_exec_queue_remove(struct xe_pxp *pxp, struct xe_exec_queue *q)
> +static void __pxp_exec_queue_remove(struct xe_pxp *pxp, struct xe_exec_queue *q, bool lock)
>  {
>  	bool need_pm_put = false;
>  
>  	if (!xe_pxp_is_enabled(pxp))
>  		return;
>  
> -	spin_lock_irq(&pxp->queues.lock);
> +	if (lock)
> +		spin_lock_irq(&pxp->queues.lock);
>  
>  	if (!list_empty(&q->pxp.link)) {
>  		list_del_init(&q->pxp.link);
> @@ -690,36 +682,54 @@ void xe_pxp_exec_queue_remove(struct xe_pxp *pxp, struct xe_exec_queue *q)
>  
>  	q->pxp.type = DRM_XE_PXP_TYPE_NONE;
>  
> -	spin_unlock_irq(&pxp->queues.lock);
> +	if (lock)
> +		spin_unlock_irq(&pxp->queues.lock);
>  
>  	if (need_pm_put)
>  		xe_pm_runtime_put(pxp->xe);
>  }
>  
> +/**
> + * xe_pxp_exec_queue_remove - remove a queue from the PXP list
> + * @pxp: the xe->pxp pointer (it will be NULL if PXP is disabled)
> + * @q: the queue to remove from the list
> + *
> + * If PXP is enabled and the exec_queue is in the list, the queue will be
> + * removed from the list and its PM reference will be released. It is safe to
> + * call this function multiple times for the same queue.
> + */
> +void xe_pxp_exec_queue_remove(struct xe_pxp *pxp, struct xe_exec_queue *q)
> +{
> +	__pxp_exec_queue_remove(pxp, q, true);
> +}
> +
>  static void pxp_invalidate_queues(struct xe_pxp *pxp)
>  {
>  	struct xe_exec_queue *tmp, *q;
> +	LIST_HEAD(to_clean);
>  
>  	spin_lock_irq(&pxp->queues.lock);
>  
> -	/*
> -	 * Removing a queue from the PXP list requires a put of the RPM ref that
> -	 * the queue holds to keep the PXP session alive, which can't be done
> -	 * under spinlock. Since it is safe to kill a queue multiple times, we
> -	 * can leave the invalid queue in the list for now and postpone the
> -	 * removal and associated RPM put to when the queue is destroyed.
> -	 */
> -	list_for_each_entry(tmp, &pxp->queues.list, pxp.link) {
> -		q = xe_exec_queue_get_unless_zero(tmp);
> -
> +	list_for_each_entry_safe(q, tmp, &pxp->queues.list, pxp.link) {
> +		q = xe_exec_queue_get_unless_zero(q);
>  		if (!q)
>  			continue;
>  
> +		list_move_tail(&q->pxp.link, &to_clean);
> +	}
> +	spin_unlock_irq(&pxp->queues.lock);
> +
> +	list_for_each_entry_safe(q, tmp, &to_clean, pxp.link) {
>  		xe_exec_queue_kill(q);
> +
> +		/*
> +		 * We hold a ref to the queue so there is no risk of racing with
> +		 * the calls to exec_queue_remove coming from exec_queue_destroy.
> +		 */
> +		__pxp_exec_queue_remove(pxp, q, false);
> +
>  		xe_exec_queue_put(q);
>  	}
> -
> -	spin_unlock_irq(&pxp->queues.lock);
>  }
>  
>  /**
> -- 
> 2.43.0
> 

Re: [PATCH] drm/xe/pxp: Don't kill queues while holding the spinlock

[Dan Carpenter]

On Wed, Feb 12, 2025 at 05:26:55PM -0800, Matthew Brost wrote:
> On Wed, Feb 12, 2025 at 04:40:32PM -0800, Daniele Ceraolo Spurio wrote:
> > xe_exec_queue_kill can sleep, so we can't call it from under the lock.
> > We can instead move the queues to a separate list and then kill them all
> > after we release the lock.
> > 
> > Since being in the list is used to track whether RPM cleanup is needed,
> > we can no longer defer that to queue_destroy, so we perform it
> > immediately instead.
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Fixes: f8caa80154c4 ("drm/xe/pxp: Add PXP queue tracking and session start")
> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> Patch LGTM but can this actually happen though? i.e. Can or do we enable
> PXP on LR queues?
> 

This isn't really an answer to your question, but when I reported this
bug I didn't notice the if (xe_vm_in_preempt_fence_mode()) check in
xe_vm_remove_compute_exec_queue().  So it's possible that this was a
false positive?

> Also as a follow should be add a might_sleep() to xe_exec_queue_kill to
> catch this type of bug immediately?

There is a might_sleep() in down_write().  If this is a real bug that
would have caught it.  The problem is that people don't generally test
with CONFIG_DEBUG_ATOMIC_SLEEP so the might_sleep() calls are turned off.

regards,
dan carpenter

Re: [PATCH] drm/xe/pxp: Don't kill queues while holding the spinlock

[Daniele Ceraolo Spurio]

On 2/12/2025 10:42 PM, Dan Carpenter wrote:
> On Wed, Feb 12, 2025 at 05:26:55PM -0800, Matthew Brost wrote:
>> On Wed, Feb 12, 2025 at 04:40:32PM -0800, Daniele Ceraolo Spurio wrote:
>>> xe_exec_queue_kill can sleep, so we can't call it from under the lock.
>>> We can instead move the queues to a separate list and then kill them all
>>> after we release the lock.
>>>
>>> Since being in the list is used to track whether RPM cleanup is needed,
>>> we can no longer defer that to queue_destroy, so we perform it
>>> immediately instead.
>>>
>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>> Fixes: f8caa80154c4 ("drm/xe/pxp: Add PXP queue tracking and session start")
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Patch LGTM but can this actually happen though? i.e. Can or do we enable
>> PXP on LR queues?
>>
> This isn't really an answer to your question, but when I reported this
> bug I didn't notice the if (xe_vm_in_preempt_fence_mode()) check in
> xe_vm_remove_compute_exec_queue().  So it's possible that this was a
> false positive?

We currently don't have a use-case where we need a vm in 
preempt_fence_mode for a queue that uses PXP, but I didn't block the 
combination because there is a chance we might want to use it in the 
future (compute PXP is supported by the HW, even if we don't currently 
support it in Xe), so a user can still set things up that way.

>
>> Also as a follow should be add a might_sleep() to xe_exec_queue_kill to
>> catch this type of bug immediately?
> There is a might_sleep() in down_write().  If this is a real bug that
> would have caught it.  The problem is that people don't generally test
> with CONFIG_DEBUG_ATOMIC_SLEEP so the might_sleep() calls are turned off.

We do have CONFIG_DEBUG_ATOMIC_SLEEP enabled in CI (and I have it 
locally since I use the CI config), but since PXP + preempt_fence_mode 
is not an expected use-case we don't have any tests that cover that 
combination, so we return early from that 
xe_vm_remove_compute_exec_queue() and don't hit the 
down_write/might_sleep. I'll see if I can add a test to cover it, as 
there might be other issues I've missed.
Also, I don't think it'd be right to add a might_sleep at the top of the 
exec_queue_kill() function either, because if a caller is sure that 
xe_vm_in_preempt_fence_mode() is false they should be allowed to 
call exec_queue_kill() from atomic context.

Thanks,
Daniele

>
> regards,
> dan carpenter
>

Re: [PATCH] drm/xe/pxp: Don't kill queues while holding the spinlock

[Matthew Brost]

On Thu, Feb 13, 2025 at 09:23:26AM -0800, Daniele Ceraolo Spurio wrote:
> 
> 
> On 2/12/2025 10:42 PM, Dan Carpenter wrote:
> > On Wed, Feb 12, 2025 at 05:26:55PM -0800, Matthew Brost wrote:
> > > On Wed, Feb 12, 2025 at 04:40:32PM -0800, Daniele Ceraolo Spurio wrote:
> > > > xe_exec_queue_kill can sleep, so we can't call it from under the lock.
> > > > We can instead move the queues to a separate list and then kill them all
> > > > after we release the lock.
> > > > 
> > > > Since being in the list is used to track whether RPM cleanup is needed,
> > > > we can no longer defer that to queue_destroy, so we perform it
> > > > immediately instead.
> > > > 
> > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > > Fixes: f8caa80154c4 ("drm/xe/pxp: Add PXP queue tracking and session start")
> > > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > Patch LGTM but can this actually happen though? i.e. Can or do we enable
> > > PXP on LR queues?
> > > 
> > This isn't really an answer to your question, but when I reported this
> > bug I didn't notice the if (xe_vm_in_preempt_fence_mode()) check in
> > xe_vm_remove_compute_exec_queue().  So it's possible that this was a
> > false positive?
> 
> We currently don't have a use-case where we need a vm in preempt_fence_mode
> for a queue that uses PXP, but I didn't block the combination because there
> is a chance we might want to use it in the future (compute PXP is supported
> by the HW, even if we don't currently support it in Xe), so a user can still
> set things up that way.
> 
> > 
> > > Also as a follow should be add a might_sleep() to xe_exec_queue_kill to
> > > catch this type of bug immediately?
> > There is a might_sleep() in down_write().  If this is a real bug that
> > would have caught it.  The problem is that people don't generally test
> > with CONFIG_DEBUG_ATOMIC_SLEEP so the might_sleep() calls are turned off.
> 
> We do have CONFIG_DEBUG_ATOMIC_SLEEP enabled in CI (and I have it locally
> since I use the CI config), but since PXP + preempt_fence_mode is not an
> expected use-case we don't have any tests that cover that combination, so we
> return early from that xe_vm_remove_compute_exec_queue() and don't hit the
> down_write/might_sleep. I'll see if I can add a test to cover it, as there
> might be other issues I've missed.
> Also, I don't think it'd be right to add a might_sleep at the top of the
> exec_queue_kill() function either, because if a caller is sure that
> xe_vm_in_preempt_fence_mode() is false they should be allowed to
> call exec_queue_kill() from atomic context.

I see what you are saying here but allowing something 'like we know we
not preempt queue so it is safe to kill in an atomic conetxt' seems
risky and a very odd thing to support. IMO we just make it clear that
this function can't be called in an atomic context.

We likely have some upcoming TLB invalidation changes too which I think
will move all queues to a per VM list with the list being protected by a
sleeping lock. Removal from this list should likely be done in kill too.
This is speculation however.

I agree some test cases for preempt queues and PXP would be a good idea
if this isn't explictly disallowed at the IOCTL level.

Matt

> 
> Thanks,
> Daniele
> 
> > 
> > regards,
> > dan carpenter
> > 
> 

Re: [PATCH] drm/xe/pxp: Don't kill queues while holding the spinlock

[Daniele Ceraolo Spurio]

On 2/13/2025 12:19 PM, Matthew Brost wrote:
> On Thu, Feb 13, 2025 at 09:23:26AM -0800, Daniele Ceraolo Spurio wrote:
>>
>> On 2/12/2025 10:42 PM, Dan Carpenter wrote:
>>> On Wed, Feb 12, 2025 at 05:26:55PM -0800, Matthew Brost wrote:
>>>> On Wed, Feb 12, 2025 at 04:40:32PM -0800, Daniele Ceraolo Spurio wrote:
>>>>> xe_exec_queue_kill can sleep, so we can't call it from under the lock.
>>>>> We can instead move the queues to a separate list and then kill them all
>>>>> after we release the lock.
>>>>>
>>>>> Since being in the list is used to track whether RPM cleanup is needed,
>>>>> we can no longer defer that to queue_destroy, so we perform it
>>>>> immediately instead.
>>>>>
>>>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>>>> Fixes: f8caa80154c4 ("drm/xe/pxp: Add PXP queue tracking and session start")
>>>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>> Patch LGTM but can this actually happen though? i.e. Can or do we enable
>>>> PXP on LR queues?
>>>>
>>> This isn't really an answer to your question, but when I reported this
>>> bug I didn't notice the if (xe_vm_in_preempt_fence_mode()) check in
>>> xe_vm_remove_compute_exec_queue().  So it's possible that this was a
>>> false positive?
>> We currently don't have a use-case where we need a vm in preempt_fence_mode
>> for a queue that uses PXP, but I didn't block the combination because there
>> is a chance we might want to use it in the future (compute PXP is supported
>> by the HW, even if we don't currently support it in Xe), so a user can still
>> set things up that way.
>>
>>>> Also as a follow should be add a might_sleep() to xe_exec_queue_kill to
>>>> catch this type of bug immediately?
>>> There is a might_sleep() in down_write().  If this is a real bug that
>>> would have caught it.  The problem is that people don't generally test
>>> with CONFIG_DEBUG_ATOMIC_SLEEP so the might_sleep() calls are turned off.
>> We do have CONFIG_DEBUG_ATOMIC_SLEEP enabled in CI (and I have it locally
>> since I use the CI config), but since PXP + preempt_fence_mode is not an
>> expected use-case we don't have any tests that cover that combination, so we
>> return early from that xe_vm_remove_compute_exec_queue() and don't hit the
>> down_write/might_sleep. I'll see if I can add a test to cover it, as there
>> might be other issues I've missed.
>> Also, I don't think it'd be right to add a might_sleep at the top of the
>> exec_queue_kill() function either, because if a caller is sure that
>> xe_vm_in_preempt_fence_mode() is false they should be allowed to
>> call exec_queue_kill() from atomic context.
> I see what you are saying here but allowing something 'like we know we
> not preempt queue so it is safe to kill in an atomic conetxt' seems
> risky and a very odd thing to support. IMO we just make it clear that
> this function can't be called in an atomic context.
>
> We likely have some upcoming TLB invalidation changes too which I think
> will move all queues to a per VM list with the list being protected by a
> sleeping lock. Removal from this list should likely be done in kill too.
> This is speculation however.
>
> I agree some test cases for preempt queues and PXP would be a good idea
> if this isn't explictly disallowed at the IOCTL level.

I have a test written locally and I've managed to repro the atomic sleep 
issue. Unfortunately, I have found a second issue with a locking 
inversion: we take pxp->mutex under the vm lock when we create an 
exec_queue that uses PXP, while here we do the opposite with the kill() 
taking the vm lock under pxp mutex. Not sure yet which of the 2 sides is 
easier to fix and therefore if this patch needs an update, so I'll hold 
the merge for now until I have a clearer idea.

Daniele

>
> Matt
>
>> Thanks,
>> Daniele
>>
>>> regards,
>>> dan carpenter
>>>

Re: [PATCH] drm/xe/pxp: Don't kill queues while holding the spinlock

[Matthew Brost]

On Tue, Feb 18, 2025 at 04:38:34PM -0800, Daniele Ceraolo Spurio wrote:
> 
> 
> On 2/13/2025 12:19 PM, Matthew Brost wrote:
> > On Thu, Feb 13, 2025 at 09:23:26AM -0800, Daniele Ceraolo Spurio wrote:
> > > 
> > > On 2/12/2025 10:42 PM, Dan Carpenter wrote:
> > > > On Wed, Feb 12, 2025 at 05:26:55PM -0800, Matthew Brost wrote:
> > > > > On Wed, Feb 12, 2025 at 04:40:32PM -0800, Daniele Ceraolo Spurio wrote:
> > > > > > xe_exec_queue_kill can sleep, so we can't call it from under the lock.
> > > > > > We can instead move the queues to a separate list and then kill them all
> > > > > > after we release the lock.
> > > > > > 
> > > > > > Since being in the list is used to track whether RPM cleanup is needed,
> > > > > > we can no longer defer that to queue_destroy, so we perform it
> > > > > > immediately instead.
> > > > > > 
> > > > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > > > > Fixes: f8caa80154c4 ("drm/xe/pxp: Add PXP queue tracking and session start")
> > > > > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > > > Patch LGTM but can this actually happen though? i.e. Can or do we enable
> > > > > PXP on LR queues?
> > > > > 
> > > > This isn't really an answer to your question, but when I reported this
> > > > bug I didn't notice the if (xe_vm_in_preempt_fence_mode()) check in
> > > > xe_vm_remove_compute_exec_queue().  So it's possible that this was a
> > > > false positive?
> > > We currently don't have a use-case where we need a vm in preempt_fence_mode
> > > for a queue that uses PXP, but I didn't block the combination because there
> > > is a chance we might want to use it in the future (compute PXP is supported
> > > by the HW, even if we don't currently support it in Xe), so a user can still
> > > set things up that way.
> > > 
> > > > > Also as a follow should be add a might_sleep() to xe_exec_queue_kill to
> > > > > catch this type of bug immediately?
> > > > There is a might_sleep() in down_write().  If this is a real bug that
> > > > would have caught it.  The problem is that people don't generally test
> > > > with CONFIG_DEBUG_ATOMIC_SLEEP so the might_sleep() calls are turned off.
> > > We do have CONFIG_DEBUG_ATOMIC_SLEEP enabled in CI (and I have it locally
> > > since I use the CI config), but since PXP + preempt_fence_mode is not an
> > > expected use-case we don't have any tests that cover that combination, so we
> > > return early from that xe_vm_remove_compute_exec_queue() and don't hit the
> > > down_write/might_sleep. I'll see if I can add a test to cover it, as there
> > > might be other issues I've missed.
> > > Also, I don't think it'd be right to add a might_sleep at the top of the
> > > exec_queue_kill() function either, because if a caller is sure that
> > > xe_vm_in_preempt_fence_mode() is false they should be allowed to
> > > call exec_queue_kill() from atomic context.
> > I see what you are saying here but allowing something 'like we know we
> > not preempt queue so it is safe to kill in an atomic conetxt' seems
> > risky and a very odd thing to support. IMO we just make it clear that
> > this function can't be called in an atomic context.
> > 
> > We likely have some upcoming TLB invalidation changes too which I think
> > will move all queues to a per VM list with the list being protected by a
> > sleeping lock. Removal from this list should likely be done in kill too.
> > This is speculation however.
> > 
> > I agree some test cases for preempt queues and PXP would be a good idea
> > if this isn't explictly disallowed at the IOCTL level.
> 
> I have a test written locally and I've managed to repro the atomic sleep
> issue. Unfortunately, I have found a second issue with a locking inversion:
> we take pxp->mutex under the vm lock when we create an exec_queue that uses
> PXP, while here we do the opposite with the kill() taking the vm lock under
> pxp mutex. Not sure yet which of the 2 sides is easier to fix and therefore
> if this patch needs an update, so I'll hold the merge for now until I have a
> clearer idea.
> 

I'd suggest q->pxp.link to pxp->kill.list + a worker to do the kill
then. The kill is already async so likely not a big to deal make it
more async.

Matt 

> Daniele
> 
> > 
> > Matt
> > 
> > > Thanks,
> > > Daniele
> > > 
> > > > regards,
> > > > dan carpenter
> > > > 
> 

Re: [PATCH] drm/xe/pxp: Don't kill queues while holding the spinlock

[Matthew Brost]

On Tue, Feb 18, 2025 at 07:18:30PM -0800, Matthew Brost wrote:
> On Tue, Feb 18, 2025 at 04:38:34PM -0800, Daniele Ceraolo Spurio wrote:
> > 
> > 
> > On 2/13/2025 12:19 PM, Matthew Brost wrote:
> > > On Thu, Feb 13, 2025 at 09:23:26AM -0800, Daniele Ceraolo Spurio wrote:
> > > > 
> > > > On 2/12/2025 10:42 PM, Dan Carpenter wrote:
> > > > > On Wed, Feb 12, 2025 at 05:26:55PM -0800, Matthew Brost wrote:
> > > > > > On Wed, Feb 12, 2025 at 04:40:32PM -0800, Daniele Ceraolo Spurio wrote:
> > > > > > > xe_exec_queue_kill can sleep, so we can't call it from under the lock.
> > > > > > > We can instead move the queues to a separate list and then kill them all
> > > > > > > after we release the lock.
> > > > > > > 
> > > > > > > Since being in the list is used to track whether RPM cleanup is needed,
> > > > > > > we can no longer defer that to queue_destroy, so we perform it
> > > > > > > immediately instead.
> > > > > > > 
> > > > > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > > > > > Fixes: f8caa80154c4 ("drm/xe/pxp: Add PXP queue tracking and session start")
> > > > > > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > > > > Patch LGTM but can this actually happen though? i.e. Can or do we enable
> > > > > > PXP on LR queues?
> > > > > > 
> > > > > This isn't really an answer to your question, but when I reported this
> > > > > bug I didn't notice the if (xe_vm_in_preempt_fence_mode()) check in
> > > > > xe_vm_remove_compute_exec_queue().  So it's possible that this was a
> > > > > false positive?
> > > > We currently don't have a use-case where we need a vm in preempt_fence_mode
> > > > for a queue that uses PXP, but I didn't block the combination because there
> > > > is a chance we might want to use it in the future (compute PXP is supported
> > > > by the HW, even if we don't currently support it in Xe), so a user can still
> > > > set things up that way.
> > > > 
> > > > > > Also as a follow should be add a might_sleep() to xe_exec_queue_kill to
> > > > > > catch this type of bug immediately?
> > > > > There is a might_sleep() in down_write().  If this is a real bug that
> > > > > would have caught it.  The problem is that people don't generally test
> > > > > with CONFIG_DEBUG_ATOMIC_SLEEP so the might_sleep() calls are turned off.
> > > > We do have CONFIG_DEBUG_ATOMIC_SLEEP enabled in CI (and I have it locally
> > > > since I use the CI config), but since PXP + preempt_fence_mode is not an
> > > > expected use-case we don't have any tests that cover that combination, so we
> > > > return early from that xe_vm_remove_compute_exec_queue() and don't hit the
> > > > down_write/might_sleep. I'll see if I can add a test to cover it, as there
> > > > might be other issues I've missed.
> > > > Also, I don't think it'd be right to add a might_sleep at the top of the
> > > > exec_queue_kill() function either, because if a caller is sure that
> > > > xe_vm_in_preempt_fence_mode() is false they should be allowed to
> > > > call exec_queue_kill() from atomic context.
> > > I see what you are saying here but allowing something 'like we know we
> > > not preempt queue so it is safe to kill in an atomic conetxt' seems
> > > risky and a very odd thing to support. IMO we just make it clear that
> > > this function can't be called in an atomic context.
> > > 
> > > We likely have some upcoming TLB invalidation changes too which I think
> > > will move all queues to a per VM list with the list being protected by a
> > > sleeping lock. Removal from this list should likely be done in kill too.
> > > This is speculation however.
> > > 
> > > I agree some test cases for preempt queues and PXP would be a good idea
> > > if this isn't explictly disallowed at the IOCTL level.
> > 
> > I have a test written locally and I've managed to repro the atomic sleep
> > issue. Unfortunately, I have found a second issue with a locking inversion:
> > we take pxp->mutex under the vm lock when we create an exec_queue that uses
> > PXP, while here we do the opposite with the kill() taking the vm lock under
> > pxp mutex. Not sure yet which of the 2 sides is easier to fix and therefore
> > if this patch needs an update, so I'll hold the merge for now until I have a
> > clearer idea.
> > 
> 
> I'd suggest q->pxp.link to pxp->kill.list + a worker to do the kill
> then. The kill is already async so likely not a big to deal make it
> more async.
> 

Or if we want to make this generic... Add xe_exec_queue_kill_async with
the same idea as above but in non-PXP specific way.

Matt

> Matt 
> 
> > Daniele
> > 
> > > 
> > > Matt
> > > 
> > > > Thanks,
> > > > Daniele
> > > > 
> > > > > regards,
> > > > > dan carpenter
> > > > > 
> > 

Re: [PATCH] drm/xe/pxp: Don't kill queues while holding the spinlock

[Daniele Ceraolo Spurio]

On 2/18/2025 7:20 PM, Matthew Brost wrote:
> On Tue, Feb 18, 2025 at 07:18:30PM -0800, Matthew Brost wrote:
>> On Tue, Feb 18, 2025 at 04:38:34PM -0800, Daniele Ceraolo Spurio wrote:
>>>
>>> On 2/13/2025 12:19 PM, Matthew Brost wrote:
>>>> On Thu, Feb 13, 2025 at 09:23:26AM -0800, Daniele Ceraolo Spurio wrote:
>>>>> On 2/12/2025 10:42 PM, Dan Carpenter wrote:
>>>>>> On Wed, Feb 12, 2025 at 05:26:55PM -0800, Matthew Brost wrote:
>>>>>>> On Wed, Feb 12, 2025 at 04:40:32PM -0800, Daniele Ceraolo Spurio wrote:
>>>>>>>> xe_exec_queue_kill can sleep, so we can't call it from under the lock.
>>>>>>>> We can instead move the queues to a separate list and then kill them all
>>>>>>>> after we release the lock.
>>>>>>>>
>>>>>>>> Since being in the list is used to track whether RPM cleanup is needed,
>>>>>>>> we can no longer defer that to queue_destroy, so we perform it
>>>>>>>> immediately instead.
>>>>>>>>
>>>>>>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>>>>>>> Fixes: f8caa80154c4 ("drm/xe/pxp: Add PXP queue tracking and session start")
>>>>>>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>>>>> Patch LGTM but can this actually happen though? i.e. Can or do we enable
>>>>>>> PXP on LR queues?
>>>>>>>
>>>>>> This isn't really an answer to your question, but when I reported this
>>>>>> bug I didn't notice the if (xe_vm_in_preempt_fence_mode()) check in
>>>>>> xe_vm_remove_compute_exec_queue().  So it's possible that this was a
>>>>>> false positive?
>>>>> We currently don't have a use-case where we need a vm in preempt_fence_mode
>>>>> for a queue that uses PXP, but I didn't block the combination because there
>>>>> is a chance we might want to use it in the future (compute PXP is supported
>>>>> by the HW, even if we don't currently support it in Xe), so a user can still
>>>>> set things up that way.
>>>>>
>>>>>>> Also as a follow should be add a might_sleep() to xe_exec_queue_kill to
>>>>>>> catch this type of bug immediately?
>>>>>> There is a might_sleep() in down_write().  If this is a real bug that
>>>>>> would have caught it.  The problem is that people don't generally test
>>>>>> with CONFIG_DEBUG_ATOMIC_SLEEP so the might_sleep() calls are turned off.
>>>>> We do have CONFIG_DEBUG_ATOMIC_SLEEP enabled in CI (and I have it locally
>>>>> since I use the CI config), but since PXP + preempt_fence_mode is not an
>>>>> expected use-case we don't have any tests that cover that combination, so we
>>>>> return early from that xe_vm_remove_compute_exec_queue() and don't hit the
>>>>> down_write/might_sleep. I'll see if I can add a test to cover it, as there
>>>>> might be other issues I've missed.
>>>>> Also, I don't think it'd be right to add a might_sleep at the top of the
>>>>> exec_queue_kill() function either, because if a caller is sure that
>>>>> xe_vm_in_preempt_fence_mode() is false they should be allowed to
>>>>> call exec_queue_kill() from atomic context.
>>>> I see what you are saying here but allowing something 'like we know we
>>>> not preempt queue so it is safe to kill in an atomic conetxt' seems
>>>> risky and a very odd thing to support. IMO we just make it clear that
>>>> this function can't be called in an atomic context.
>>>>
>>>> We likely have some upcoming TLB invalidation changes too which I think
>>>> will move all queues to a per VM list with the list being protected by a
>>>> sleeping lock. Removal from this list should likely be done in kill too.
>>>> This is speculation however.
>>>>
>>>> I agree some test cases for preempt queues and PXP would be a good idea
>>>> if this isn't explictly disallowed at the IOCTL level.
>>> I have a test written locally and I've managed to repro the atomic sleep
>>> issue. Unfortunately, I have found a second issue with a locking inversion:
>>> we take pxp->mutex under the vm lock when we create an exec_queue that uses
>>> PXP, while here we do the opposite with the kill() taking the vm lock under
>>> pxp mutex. Not sure yet which of the 2 sides is easier to fix and therefore
>>> if this patch needs an update, so I'll hold the merge for now until I have a
>>> clearer idea.
>>>
>> I'd suggest q->pxp.link to pxp->kill.list + a worker to do the kill
>> then. The kill is already async so likely not a big to deal make it
>> more async.
>>
> Or if we want to make this generic... Add xe_exec_queue_kill_async with
> the same idea as above but in non-PXP specific way.

I've considered that, but I think I can more easily just move the kill() 
out of the pxp->mutex and remove the problem that way. I'm testing that 
solution now, will post it later if I don't hit any errors.

Daniele

>
> Matt
>
>> Matt
>>
>>> Daniele
>>>
>>>> Matt
>>>>
>>>>> Thanks,
>>>>> Daniele
>>>>>
>>>>>> regards,
>>>>>> dan carpenter
>>>>>>