[v3,hmm,10/11] drm/amdkfd: use mmu_notifier_put
diff mbox series

Message ID 20190806231548.25242-11-jgg@ziepe.ca
State Accepted
Delegated to: Jason Gunthorpe
Headers show
Series
  • Add mmu_notifier_get/put for managing mmu notifier registrations
Related show

Commit Message

Jason Gunthorpe Aug. 6, 2019, 11:15 p.m. UTC
From: Jason Gunthorpe <jgg@mellanox.com>

The sequence of mmu_notifier_unregister_no_release(),
mmu_notifier_call_srcu() is identical to mmu_notifier_put() with the
free_notifier callback.

As this is the last user of those APIs, converting it means we can drop
them.

Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  3 ---
 drivers/gpu/drm/amd/amdkfd/kfd_process.c | 10 ++++------
 2 files changed, 4 insertions(+), 9 deletions(-)

I'm really not sure what this is doing, but it is very strange to have a
release with no other callback. It would be good if this would change to use
get as well.

Comments

Kuehling, Felix Aug. 6, 2019, 11:47 p.m. UTC | #1
On 2019-08-06 19:15, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> The sequence of mmu_notifier_unregister_no_release(),
> mmu_notifier_call_srcu() is identical to mmu_notifier_put() with the
> free_notifier callback.
>
> As this is the last user of those APIs, converting it means we can drop
> them.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>

Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  3 ---
>   drivers/gpu/drm/amd/amdkfd/kfd_process.c | 10 ++++------
>   2 files changed, 4 insertions(+), 9 deletions(-)
>
> I'm really not sure what this is doing, but it is very strange to have a
> release with no other callback. It would be good if this would change to use
> get as well.
KFD uses the MMU notifier to detect process termination and free all the 
resources associated with the process. This was first added for APUs 
where the IOMMUv2 is set up to perform address translations using the 
CPU page table for device memory access. That's where the association of 
KFD process resources with the lifetime of the mm_struct comes from.

Regards,
   Felix


>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 3933fb6a371efb..9450e20d17093b 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -686,9 +686,6 @@ struct kfd_process {
>   	/* We want to receive a notification when the mm_struct is destroyed */
>   	struct mmu_notifier mmu_notifier;
>   
> -	/* Use for delayed freeing of kfd_process structure */
> -	struct rcu_head	rcu;
> -
>   	unsigned int pasid;
>   	unsigned int doorbell_index;
>   
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index c06e6190f21ffa..e5e326f2f2675e 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -486,11 +486,9 @@ static void kfd_process_ref_release(struct kref *ref)
>   	queue_work(kfd_process_wq, &p->release_work);
>   }
>   
> -static void kfd_process_destroy_delayed(struct rcu_head *rcu)
> +static void kfd_process_free_notifier(struct mmu_notifier *mn)
>   {
> -	struct kfd_process *p = container_of(rcu, struct kfd_process, rcu);
> -
> -	kfd_unref_process(p);
> +	kfd_unref_process(container_of(mn, struct kfd_process, mmu_notifier));
>   }
>   
>   static void kfd_process_notifier_release(struct mmu_notifier *mn,
> @@ -542,12 +540,12 @@ static void kfd_process_notifier_release(struct mmu_notifier *mn,
>   
>   	mutex_unlock(&p->mutex);
>   
> -	mmu_notifier_unregister_no_release(&p->mmu_notifier, mm);
> -	mmu_notifier_call_srcu(&p->rcu, &kfd_process_destroy_delayed);
> +	mmu_notifier_put(&p->mmu_notifier);
>   }
>   
>   static const struct mmu_notifier_ops kfd_process_mmu_notifier_ops = {
>   	.release = kfd_process_notifier_release,
> +	.free_notifier = kfd_process_free_notifier,
>   };
>   
>   static int kfd_process_init_cwsr_apu(struct kfd_process *p, struct file *filep)
Jason Gunthorpe Aug. 7, 2019, 11:42 a.m. UTC | #2
On Tue, Aug 06, 2019 at 11:47:44PM +0000, Kuehling, Felix wrote:
> On 2019-08-06 19:15, Jason Gunthorpe wrote:
> > From: Jason Gunthorpe <jgg@mellanox.com>
> >
> > The sequence of mmu_notifier_unregister_no_release(),
> > mmu_notifier_call_srcu() is identical to mmu_notifier_put() with the
> > free_notifier callback.
> >
> > As this is the last user of those APIs, converting it means we can drop
> > them.
> >
> > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> 
> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> 
> >   drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  3 ---
> >   drivers/gpu/drm/amd/amdkfd/kfd_process.c | 10 ++++------
> >   2 files changed, 4 insertions(+), 9 deletions(-)
> >
> > I'm really not sure what this is doing, but it is very strange to have a
> > release with no other callback. It would be good if this would change to use
> > get as well.
> KFD uses the MMU notifier to detect process termination and free all the 
> resources associated with the process. This was first added for APUs 
> where the IOMMUv2 is set up to perform address translations using the 
> CPU page table for device memory access. That's where the association of 
> KFD process resources with the lifetime of the mm_struct comes from.

When all the HW objects that could do DMA to this process are
destroyed then the mmu notififer should be torn down. The module
should remain locked until the DMA objects are destroyed.

I'm still unclear why this is needed, the IOMMU for PASID already has
notififers, and already blocks access when the mm_struct goes away,
why add a second layer of tracking?

Jason

Patch
diff mbox series

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 3933fb6a371efb..9450e20d17093b 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -686,9 +686,6 @@  struct kfd_process {
 	/* We want to receive a notification when the mm_struct is destroyed */
 	struct mmu_notifier mmu_notifier;
 
-	/* Use for delayed freeing of kfd_process structure */
-	struct rcu_head	rcu;
-
 	unsigned int pasid;
 	unsigned int doorbell_index;
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index c06e6190f21ffa..e5e326f2f2675e 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -486,11 +486,9 @@  static void kfd_process_ref_release(struct kref *ref)
 	queue_work(kfd_process_wq, &p->release_work);
 }
 
-static void kfd_process_destroy_delayed(struct rcu_head *rcu)
+static void kfd_process_free_notifier(struct mmu_notifier *mn)
 {
-	struct kfd_process *p = container_of(rcu, struct kfd_process, rcu);
-
-	kfd_unref_process(p);
+	kfd_unref_process(container_of(mn, struct kfd_process, mmu_notifier));
 }
 
 static void kfd_process_notifier_release(struct mmu_notifier *mn,
@@ -542,12 +540,12 @@  static void kfd_process_notifier_release(struct mmu_notifier *mn,
 
 	mutex_unlock(&p->mutex);
 
-	mmu_notifier_unregister_no_release(&p->mmu_notifier, mm);
-	mmu_notifier_call_srcu(&p->rcu, &kfd_process_destroy_delayed);
+	mmu_notifier_put(&p->mmu_notifier);
 }
 
 static const struct mmu_notifier_ops kfd_process_mmu_notifier_ops = {
 	.release = kfd_process_notifier_release,
+	.free_notifier = kfd_process_free_notifier,
 };
 
 static int kfd_process_init_cwsr_apu(struct kfd_process *p, struct file *filep)