lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b35506de99be38f560709660b10667ca9f386181.camel@mailbox.org>
Date: Thu, 04 Sep 2025 13:12:10 +0200
From: Philipp Stanner <phasta@...lbox.org>
To: Christian König <christian.koenig@....com>, Philipp
 Stanner <phasta@...nel.org>, Lyude Paul <lyude@...hat.com>, Danilo
 Krummrich <dakr@...nel.org>,  David Airlie <airlied@...il.com>, Simona
 Vetter <simona@...ll.ch>, Sumit Semwal <sumit.semwal@...aro.org>
Cc: dri-devel@...ts.freedesktop.org, nouveau@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] Revert "drm/nouveau: Remove waitque for sched
 teardown"

On Thu, 2025-09-04 at 12:27 +0200, Christian König wrote:
> On 01.09.25 10:31, Philipp Stanner wrote:
> > This reverts:
> > 
> > commit bead88002227 ("drm/nouveau: Remove waitque for sched teardown")
> > commit 5f46f5c7af8c ("drm/nouveau: Add new callback for scheduler teardown")
> > 
> > from the drm/sched teardown leak fix series:
> > 
> > https://lore.kernel.org/dri-devel/20250710125412.128476-2-phasta@kernel.org/
> > 
> > The aforementioned series removed a blocking waitqueue from
> > nouveau_sched_fini(). It was mistakenly assumed that this waitqueue only
> > prevents jobs from leaking, which the series fixed.
> > 
> > The waitqueue, however, also guarantees that all VM_BIND related jobs
> > are finished in order, cleaning up mappings in the GPU's MMU. These jobs
> > must be executed sequentially. Without the waitqueue, this is no longer
> > guaranteed, because entity and scheduler teardown can race with each
> > other.
> 
> That sounds like exactly the kind of issues I tried to catch with the recent dma_fence changes.

Link? :)

> 
> Going to keep working on that and potentially using this here as blueprint for something it should catch.

This is more like a nouveau-specific issue. The problem is that
unmapping mappings in the GPU's MMU must be done in a specific order,
and all the unmappings must be performed, not canceled.

For EXEC jobs, it's perfectly fine to cancel pending jobs, remove the
waitqueue and just rush through drm_sched_fini().

I don't know the issue you're describing, but I don't think a feature
in dma_fence could help with that specific Nouveau problem. dma_fence
can't force the driver to submit jobs in a specific order or to wait
until they're all completed.

Grüße
P.

> 
> Regards,
> Christian.
> 
> > 
> > Revert all patches related to the waitqueue removal.
> > 
> > Fixes: bead88002227 ("drm/nouveau: Remove waitque for sched teardown")
> > Suggested-by: Danilo Krummrich <dakr@...nel.org>
> > Signed-off-by: Philipp Stanner <phasta@...nel.org>
> > ---
> > Changes in v2:
> >   - Don't revert commit 89b2675198ab ("drm/nouveau: Make fence container helper usable driver-wide")
> >   - Add Fixes-tag
> > ---
> >  drivers/gpu/drm/nouveau/nouveau_fence.c | 15 -----------
> >  drivers/gpu/drm/nouveau/nouveau_fence.h |  1 -
> >  drivers/gpu/drm/nouveau/nouveau_sched.c | 35 ++++++++++---------------
> >  drivers/gpu/drm/nouveau/nouveau_sched.h |  9 ++++---
> >  drivers/gpu/drm/nouveau/nouveau_uvmm.c  |  8 +++---
> >  5 files changed, 24 insertions(+), 44 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c
> > index 9f345a008717..869d4335c0f4 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c
> > @@ -240,21 +240,6 @@ nouveau_fence_emit(struct nouveau_fence *fence)
> >  	return ret;
> >  }
> >  
> > -void
> > -nouveau_fence_cancel(struct nouveau_fence *fence)
> > -{
> > -	struct nouveau_fence_chan *fctx = nouveau_fctx(fence);
> > -	unsigned long flags;
> > -
> > -	spin_lock_irqsave(&fctx->lock, flags);
> > -	if (!dma_fence_is_signaled_locked(&fence->base)) {
> > -		dma_fence_set_error(&fence->base, -ECANCELED);
> > -		if (nouveau_fence_signal(fence))
> > -			nvif_event_block(&fctx->event);
> > -	}
> > -	spin_unlock_irqrestore(&fctx->lock, flags);
> > -}
> > -
> >  bool
> >  nouveau_fence_done(struct nouveau_fence *fence)
> >  {
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.h b/drivers/gpu/drm/nouveau/nouveau_fence.h
> > index 9957a919bd38..183dd43ecfff 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_fence.h
> > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.h
> > @@ -29,7 +29,6 @@ void nouveau_fence_unref(struct nouveau_fence **);
> >  
> >  int  nouveau_fence_emit(struct nouveau_fence *);
> >  bool nouveau_fence_done(struct nouveau_fence *);
> > -void nouveau_fence_cancel(struct nouveau_fence *fence);
> >  int  nouveau_fence_wait(struct nouveau_fence *, bool lazy, bool intr);
> >  int  nouveau_fence_sync(struct nouveau_bo *, struct nouveau_channel *, bool exclusive, bool intr);
> >  
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.c b/drivers/gpu/drm/nouveau/nouveau_sched.c
> > index 0cc0bc9f9952..e60f7892f5ce 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_sched.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_sched.c
> > @@ -11,7 +11,6 @@
> >  #include "nouveau_exec.h"
> >  #include "nouveau_abi16.h"
> >  #include "nouveau_sched.h"
> > -#include "nouveau_chan.h"
> >  
> >  #define NOUVEAU_SCHED_JOB_TIMEOUT_MS		10000
> >  
> > @@ -122,9 +121,11 @@ nouveau_job_done(struct nouveau_job *job)
> >  {
> >  	struct nouveau_sched *sched = job->sched;
> >  
> > -	spin_lock(&sched->job_list.lock);
> > +	spin_lock(&sched->job.list.lock);
> >  	list_del(&job->entry);
> > -	spin_unlock(&sched->job_list.lock);
> > +	spin_unlock(&sched->job.list.lock);
> > +
> > +	wake_up(&sched->job.wq);
> >  }
> >  
> >  void
> > @@ -305,9 +306,9 @@ nouveau_job_submit(struct nouveau_job *job)
> >  	}
> >  
> >  	/* Submit was successful; add the job to the schedulers job list. */
> > -	spin_lock(&sched->job_list.lock);
> > -	list_add(&job->entry, &sched->job_list.head);
> > -	spin_unlock(&sched->job_list.lock);
> > +	spin_lock(&sched->job.list.lock);
> > +	list_add(&job->entry, &sched->job.list.head);
> > +	spin_unlock(&sched->job.list.lock);
> >  
> >  	drm_sched_job_arm(&job->base);
> >  	job->done_fence = dma_fence_get(&job->base.s_fence->finished);
> > @@ -392,23 +393,10 @@ nouveau_sched_free_job(struct drm_sched_job *sched_job)
> >  	nouveau_job_fini(job);
> >  }
> >  
> > -static void
> > -nouveau_sched_cancel_job(struct drm_sched_job *sched_job)
> > -{
> > -	struct nouveau_fence *fence;
> > -	struct nouveau_job *job;
> > -
> > -	job = to_nouveau_job(sched_job);
> > -	fence = to_nouveau_fence(job->done_fence);
> > -
> > -	nouveau_fence_cancel(fence);
> > -}
> > -
> >  static const struct drm_sched_backend_ops nouveau_sched_ops = {
> >  	.run_job = nouveau_sched_run_job,
> >  	.timedout_job = nouveau_sched_timedout_job,
> >  	.free_job = nouveau_sched_free_job,
> > -	.cancel_job = nouveau_sched_cancel_job,
> >  };
> >  
> >  static int
> > @@ -458,8 +446,9 @@ nouveau_sched_init(struct nouveau_sched *sched, struct nouveau_drm *drm,
> >  		goto fail_sched;
> >  
> >  	mutex_init(&sched->mutex);
> > -	spin_lock_init(&sched->job_list.lock);
> > -	INIT_LIST_HEAD(&sched->job_list.head);
> > +	spin_lock_init(&sched->job.list.lock);
> > +	INIT_LIST_HEAD(&sched->job.list.head);
> > +	init_waitqueue_head(&sched->job.wq);
> >  
> >  	return 0;
> >  
> > @@ -493,12 +482,16 @@ nouveau_sched_create(struct nouveau_sched **psched, struct nouveau_drm *drm,
> >  	return 0;
> >  }
> >  
> > +
> >  static void
> >  nouveau_sched_fini(struct nouveau_sched *sched)
> >  {
> >  	struct drm_gpu_scheduler *drm_sched = &sched->base;
> >  	struct drm_sched_entity *entity = &sched->entity;
> >  
> > +	rmb(); /* for list_empty to work without lock */
> > +	wait_event(sched->job.wq, list_empty(&sched->job.list.head));
> > +
> >  	drm_sched_entity_fini(entity);
> >  	drm_sched_fini(drm_sched);
> >  
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.h b/drivers/gpu/drm/nouveau/nouveau_sched.h
> > index b98c3f0bef30..20cd1da8db73 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_sched.h
> > +++ b/drivers/gpu/drm/nouveau/nouveau_sched.h
> > @@ -103,9 +103,12 @@ struct nouveau_sched {
> >  	struct mutex mutex;
> >  
> >  	struct {
> > -		struct list_head head;
> > -		spinlock_t lock;
> > -	} job_list;
> > +		struct {
> > +			struct list_head head;
> > +			spinlock_t lock;
> > +		} list;
> > +		struct wait_queue_head wq;
> > +	} job;
> >  };
> >  
> >  int nouveau_sched_create(struct nouveau_sched **psched, struct nouveau_drm *drm,
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > index d94a85509176..79eefdfd08a2 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > @@ -1019,8 +1019,8 @@ bind_validate_map_sparse(struct nouveau_job *job, u64 addr, u64 range)
> >  	u64 end = addr + range;
> >  
> >  again:
> > -	spin_lock(&sched->job_list.lock);
> > -	list_for_each_entry(__job, &sched->job_list.head, entry) {
> > +	spin_lock(&sched->job.list.lock);
> > +	list_for_each_entry(__job, &sched->job.list.head, entry) {
> >  		struct nouveau_uvmm_bind_job *bind_job = to_uvmm_bind_job(__job);
> >  
> >  		list_for_each_op(op, &bind_job->ops) {
> > @@ -1030,7 +1030,7 @@ bind_validate_map_sparse(struct nouveau_job *job, u64 addr, u64 range)
> >  
> >  				if (!(end <= op_addr || addr >= op_end)) {
> >  					nouveau_uvmm_bind_job_get(bind_job);
> > -					spin_unlock(&sched->job_list.lock);
> > +					spin_unlock(&sched->job.list.lock);
> >  					wait_for_completion(&bind_job->complete);
> >  					nouveau_uvmm_bind_job_put(bind_job);
> >  					goto again;
> > @@ -1038,7 +1038,7 @@ bind_validate_map_sparse(struct nouveau_job *job, u64 addr, u64 range)
> >  			}
> >  		}
> >  	}
> > -	spin_unlock(&sched->job_list.lock);
> > +	spin_unlock(&sched->job.list.lock);
> >  }
> >  
> >  static int
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ