Re: [PATCH] drm/amdkfd: Fixed kfd_process cleanup on module exit.

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 08.03.23 um 17:17 schrieb Felix Kuehling:
> On 2023-03-08 04:07, Christian König wrote:
> > Am 07.03.23 um 16:28 schrieb Belanger, David:
> > > [AMD Official Use Only - General]
> > >
> > >
> > > The test case is a python program that will load the driver, do some 
> > > operations, then unload the driver.
> >
> > What do you mean with unloading the driver? Removing the module? Or 
> > destroying the device?
> >
> > > When the driver exists, there is still the python process space 
> > > around holding on the address space.
> > > When the python process space exits, the mmu_notifier gets called 
> > > but the driver has already been unloaded.
> > >
> > > The goal of the fix is to address case where there could be 
> > > outstanding address space / worker threads for process
> > > cleanup that needs to be cleared/completed at exit time.
> >
> > Yeah and when the module is unloaded this is a completely futile effort.
> >
> > The general upstream approach is to take references on the struct 
> > device and module and prevent unloading as long as those references 
> > exists.
>
> That's not how it always works. In case of RCU callbacks, the 
> documented strategy is to use rcu_barrier in the module exit function 
> to ensure the grace period and all callbacks have completed 
> (https://www.kernel.org/doc/html/latest/RCU/rcubarrier.html). 
> mmu_notifier_synchronize is meant to do something similar for pending 
> mmu_notifier_put work 
> (https://elixir.bootlin.com/linux/v6.2.2/source/mm/mmu_notifier.c#L1116).
>
> But this implies that we need to call mmu_notifier_put for all the MMU 
> notifiers registered by the module first. I think closing /dev/kfd 
> drops the module reference count, but the MMU notifiers we register 
> for process cleanup persist until the address space is destroyed. We 
> need to trigger that cleanup for any processes that still exist in 
> that state when the module is unloaded. Or we need to find a way to 
> increment the module refcount for every process that registers a KFD 
> cleanup MMU notifier.

The later is what I've meant. Cleaning up when the module unloads is 
certainly possible as well, but harder to get right.

And I don't really see an use case that we should do the cleanup way.

Regards,
Christian.

> Regards,
>   Felix
>
>
> > The device might be non-functional any more (because for example of 
> > hot plug), but the driver should never be unloaded before the python 
> > program exits.
> >
> > Regards,
> > Christian.
> >
> > > Regards,
> > > David B.
> > >
> > > > -----Original Message-----
> > > > From: Koenig, Christian <Christian.Koenig@xxxxxxx>
> > > > Sent: Tuesday, March 7, 2023 2:05 AM
> > > > To: Belanger, David <David.Belanger@xxxxxxx>; amd-
> > > > gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > Subject: Re: [PATCH] drm/amdkfd: Fixed kfd_process cleanup on module
> > > > exit.
> > > >
> > > > Am 06.03.23 um 22:58 schrieb David Belanger:
> > > > > Handle case when module is unloaded (kfd_exit) before a process space
> > > > > (mm_struct) is released.
> > > > Well that should never ever happen in the first place. It sounds 
> > > > like we are
> > > > missing grabbing module references.
> > > >
> > > > Regards,
> > > > Christian.
> > > >
> > > > > Signed-off-by: David Belanger <david.belanger@xxxxxxx>
> > > > > ---
> > > > >    drivers/gpu/drm/amd/amdkfd/kfd_module.c  |  4 ++
> > > > >    drivers/gpu/drm/amd/amdkfd/kfd_process.c | 57
> > > > ++++++++++++++++++++++++
> > > > >    2 files changed, 61 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c
> > > > > b/drivers/gpu/drm/amd/amdkfd/kfd_module.c
> > > > > index 09b966dc3768..8ef4bd9e4f7d 100644
> > > > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c
> > > > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c
> > > > > @@ -26,6 +26,9 @@
> > > > >    #include "kfd_priv.h"
> > > > >    #include "amdgpu_amdkfd.h"
> > > > >
> > > > > +void kfd_cleanup_processes(void);
> > > > > +
> > > > > +
> > > > >    static int kfd_init(void)
> > > > >    {
> > > > >        int err;
> > > > > @@ -77,6 +80,7 @@ static int kfd_init(void)
> > > > >
> > > > >    static void kfd_exit(void)
> > > > >    {
> > > > > +    kfd_cleanup_processes();
> > > > >        kfd_debugfs_fini();
> > > > >        kfd_process_destroy_wq();
> > > > >        kfd_procfs_shutdown();
> > > > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > > > > b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > > > > index ebabe92f7edb..b5b28a32639d 100644
> > > > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > > > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > > > > @@ -1181,6 +1181,17 @@ static void 
> > > > > kfd_process_notifier_release(struct
> > > > mmu_notifier *mn,
> > > > >            return;
> > > > >
> > > > >        mutex_lock(&kfd_processes_mutex);
> > > > > +    /*
> > > > > +     * Do early return if p is not in the table.
> > > > > +     *
> > > > > +     * This could potentially happen if this function is called 
> > > > > concurrently
> > > > > +     * by mmu_notifier and by kfd_cleanup_pocesses.
> > > > > +     *
> > > > > +     */
> > > > > +    if (!hash_hashed(&p->kfd_processes)) {
> > > > > +        mutex_unlock(&kfd_processes_mutex);
> > > > > +        return;
> > > > > +    }
> > > > >        hash_del_rcu(&p->kfd_processes);
> > > > >        mutex_unlock(&kfd_processes_mutex);
> > > > >        synchronize_srcu(&kfd_processes_srcu);
> > > > > @@ -1200,6 +1211,52 @@ static const struct mmu_notifier_ops
> > > > kfd_process_mmu_notifier_ops = {
> > > > >        .free_notifier = kfd_process_free_notifier,
> > > > >    };
> > > > >
> > > > > +
> > > > > +void kfd_cleanup_processes(void)
> > > > > +{
> > > > > +    struct kfd_process *p;
> > > > > +    unsigned int temp;
> > > > > +
> > > > > +    /*
> > > > > +     * Iterate over remaining processes in table, calling 
> > > > > notifier release
> > > > > +     * to free up notifier and process resources.
> > > > > +     *
> > > > > +     * This code handles the case when driver is unloaded before all
> > > > mm_struct
> > > > > +     * are released.
> > > > > +     */
> > > > > +    int idx = srcu_read_lock(&kfd_processes_srcu);
> > > > > +
> > > > > +    hash_for_each_rcu(kfd_processes_table, temp, p, kfd_processes) {
> > > > > +        if (p) {
> > > > > +            /*
> > > > > +             * Obtain a reference on p to avoid a late
> > > > mmu_notifier release
> > > > > +             * call triggering freeing the process.
> > > > > +             */
> > > > > +
> > > > > +            kref_get(&p->ref);
> > > > > +
> > > > > +            srcu_read_unlock(&kfd_processes_srcu, idx);
> > > > > +
> > > > > + kfd_process_notifier_release(&p->mmu_notifier, p-
> > > > > mm);
> > > > > +
> > > > > +            kfd_unref_process(p);
> > > > > +
> > > > > +            idx = srcu_read_lock(&kfd_processes_srcu);
> > > > > +        }
> > > > > +    }
> > > > > +    srcu_read_unlock(&kfd_processes_srcu, idx);
> > > > > +
> > > > > +    /*
> > > > > +     * Must be called after all mmu_notifier_put are done and before
> > > > > +     * kfd_process_wq is released.
> > > > > +     *
> > > > > +     * Ensures that all outstanding free_notifier gets called, 
> > > > > triggering the
> > > > release
> > > > > +     * of the process.
> > > > > +     */
> > > > > +    mmu_notifier_synchronize();
> > > > > +}
> > > > > +
> > > > > +
> > > > >    static int kfd_process_init_cwsr_apu(struct kfd_process *p, 
> > > > > struct file
> > > > *filep)
> > > > >    {
> > > > >        unsigned long  offset;