* [PATCH] drm/amdkfd: Restore all process on post reset @ 2021-07-28 17:31 Eric Huang 2021-07-30 21:26 ` Felix Kuehling 0 siblings, 1 reply; 4+ messages in thread From: Eric Huang @ 2021-07-28 17:31 UTC (permalink / raw) To: amd-gfx; +Cc: Eric Huang It is to fix a bug of gpu_recovery on multiple GPUs, When one gpu is reset, the application running on other gpu hangs, because kfd post reset doesn't restore the running process. And it also fixes a bug in the function kfd_process_evict_queues, when one gpu hangs, process running on other gpus can't be evicted. Signed-off-by: Eric Huang <jinhuieric.huang@amd.com> --- drivers/gpu/drm/amd/amdkfd/kfd_device.c | 2 +- drivers/gpu/drm/amd/amdkfd/kfd_process.c | 24 +----------------------- 2 files changed, 2 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c index 24b5e0aa1eac..daf1c19bd799 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c @@ -984,7 +984,7 @@ int kgd2kfd_post_reset(struct kfd_dev *kfd) if (!kfd->init_complete) return 0; - ret = kfd_resume(kfd); + ret = kgd2kfd_resume(kfd, false, true); if (ret) return ret; atomic_dec(&kfd_locked); diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c index 38a9dee40785..9272a12c1db8 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c @@ -1879,36 +1879,14 @@ int kfd_process_evict_queues(struct kfd_process *p) { int r = 0; int i; - unsigned int n_evicted = 0; for (i = 0; i < p->n_pdds; i++) { struct kfd_process_device *pdd = p->pdds[i]; r = pdd->dev->dqm->ops.evict_process_queues(pdd->dev->dqm, &pdd->qpd); - if (r) { + if (r) pr_err("Failed to evict process queues\n"); - goto fail; - } - n_evicted++; - } - - return r; - -fail: - /* To keep state consistent, roll back partial eviction by - * restoring queues - */ - for (i = 0; i < p->n_pdds; i++) { - struct kfd_process_device *pdd = p->pdds[i]; - - if (n_evicted == 0) - break; - if (pdd->dev->dqm->ops.restore_process_queues(pdd->dev->dqm, - &pdd->qpd)) - pr_err("Failed to restore queues\n"); - - n_evicted--; } return r; -- 2.25.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/amdkfd: Restore all process on post reset 2021-07-28 17:31 [PATCH] drm/amdkfd: Restore all process on post reset Eric Huang @ 2021-07-30 21:26 ` Felix Kuehling 2021-08-03 15:02 ` Eric Huang 0 siblings, 1 reply; 4+ messages in thread From: Felix Kuehling @ 2021-07-30 21:26 UTC (permalink / raw) To: Eric Huang, amd-gfx Am 2021-07-28 um 1:31 p.m. schrieb Eric Huang: > It is to fix a bug of gpu_recovery on multiple GPUs, > When one gpu is reset, the application running on other > gpu hangs, because kfd post reset doesn't restore the > running process. This will resume all processes, even those that were affected by the GPU reset. The assumption we made here is, that KFD processes can use all GPUs. So when one GPU is reset, all processes are affected. If we want to refine that, we'll need some more complex logic to identify the processes affected by a GPU reset and keep only those processes suspended. This could be based on the GPUs that a process has created queues on, or allocated memory on. What we don't want, is processes continuing with bad data or inconsistent state after a GPU reset. > And it also fixes a bug in the function > kfd_process_evict_queues, when one gpu hangs, process > running on other gpus can't be evicted. This should be a separate patch. The code you're removing was there as an attempt to make kfd_process_evict_queues transactional. That means, it either succeeds completely or it fails completely. I wanted to avoid putting the system into an unknown state where some queues are suspended and others are not and the caller has no idea how to proceed. So I roll back a partial queue eviction if something failed. Your changing this to "try to evict as much as possible". Then a failure does not mean "eviction failed" but "eviction completed but something hung". Then the GPU reset can take care of the hanging part. I think that's a reasonable strategy. But we need to be sure that there are no other error conditions (other than hangs) that could cause a queue eviction to fail. There were some recent changes in pqm_destroy_queue that check the return value of dqm->ops.destroy_queue, which indirectly comes from execute_queues (sam as in the eviction case). -ETIME means a hang. Other errors are possible. Those other errors may still need the roll-back. Otherwise we'd leave stuff running on a non-hanging GPU after we promised that we evicted everything. See one more comment inline. > > Signed-off-by: Eric Huang <jinhuieric.huang@amd.com> > --- > drivers/gpu/drm/amd/amdkfd/kfd_device.c | 2 +- > drivers/gpu/drm/amd/amdkfd/kfd_process.c | 24 +----------------------- > 2 files changed, 2 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c > index 24b5e0aa1eac..daf1c19bd799 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c > @@ -984,7 +984,7 @@ int kgd2kfd_post_reset(struct kfd_dev *kfd) > if (!kfd->init_complete) > return 0; > > - ret = kfd_resume(kfd); > + ret = kgd2kfd_resume(kfd, false, true); Which branch is this for? On amd-staging-drm-next kgd2kfd_resume only has two parameters. Regards, Felix > if (ret) > return ret; > atomic_dec(&kfd_locked); > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c > index 38a9dee40785..9272a12c1db8 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c > @@ -1879,36 +1879,14 @@ int kfd_process_evict_queues(struct kfd_process *p) > { > int r = 0; > int i; > - unsigned int n_evicted = 0; > > for (i = 0; i < p->n_pdds; i++) { > struct kfd_process_device *pdd = p->pdds[i]; > > r = pdd->dev->dqm->ops.evict_process_queues(pdd->dev->dqm, > &pdd->qpd); > - if (r) { > + if (r) > pr_err("Failed to evict process queues\n"); > - goto fail; > - } > - n_evicted++; > - } > - > - return r; > - > -fail: > - /* To keep state consistent, roll back partial eviction by > - * restoring queues > - */ > - for (i = 0; i < p->n_pdds; i++) { > - struct kfd_process_device *pdd = p->pdds[i]; > - > - if (n_evicted == 0) > - break; > - if (pdd->dev->dqm->ops.restore_process_queues(pdd->dev->dqm, > - &pdd->qpd)) > - pr_err("Failed to restore queues\n"); > - > - n_evicted--; > } > > return r; ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/amdkfd: Restore all process on post reset 2021-07-30 21:26 ` Felix Kuehling @ 2021-08-03 15:02 ` Eric Huang 2021-08-03 15:38 ` Felix Kuehling 0 siblings, 1 reply; 4+ messages in thread From: Eric Huang @ 2021-08-03 15:02 UTC (permalink / raw) To: Felix Kuehling, amd-gfx On 2021-07-30 5:26 p.m., Felix Kuehling wrote: > Am 2021-07-28 um 1:31 p.m. schrieb Eric Huang: >> It is to fix a bug of gpu_recovery on multiple GPUs, >> When one gpu is reset, the application running on other >> gpu hangs, because kfd post reset doesn't restore the >> running process. > This will resume all processes, even those that were affected by the GPU > reset. > > The assumption we made here is, that KFD processes can use all GPUs. So > when one GPU is reset, all processes are affected. If we want to refine > that, we'll need some more complex logic to identify the processes > affected by a GPU reset and keep only those processes suspended. This > could be based on the GPUs that a process has created queues on, or > allocated memory on. > > What we don't want, is processes continuing with bad data or > inconsistent state after a GPU reset. > Current code doesn't take care of this assumption. When a GPU hangs, evicting queues will fail on it and roll back to restore all processes queues on other GPUs, and continue to running with unclear state and data after a GPU reset. The original thought about this patch is to call kfd_suspend_all_processes and kfd_restore_all_processes in pair on pre_reset and post_reset. And It keeps the consistent behavior for both amdgpu_gpu_recover and hang_hws. >> And it also fixes a bug in the function >> kfd_process_evict_queues, when one gpu hangs, process >> running on other gpus can't be evicted. > This should be a separate patch. The code you're removing was there as > an attempt to make kfd_process_evict_queues transactional. That means, > it either succeeds completely or it fails completely. I wanted to avoid > putting the system into an unknown state where some queues are suspended > and others are not and the caller has no idea how to proceed. So I roll > back a partial queue eviction if something failed. Can we let the caller to decide if roll-back is needed? because no all the callers need to roll back, e.g. kfd_suspend_all_processes and kgd2kfd_quiesce_mm. > > Your changing this to "try to evict as much as possible". Then a failure > does not mean "eviction failed" but "eviction completed but something > hung". Then the GPU reset can take care of the hanging part. I think > that's a reasonable strategy. But we need to be sure that there are no > other error conditions (other than hangs) that could cause a queue > eviction to fail. > > There were some recent changes in pqm_destroy_queue that check the > return value of dqm->ops.destroy_queue, which indirectly comes from > execute_queues (sam as in the eviction case). -ETIME means a hang. Other > errors are possible. Those other errors may still need the roll-back. > Otherwise we'd leave stuff running on a non-hanging GPU after we > promised that we evicted everything. I think it depends on case scenario. GPU reset doesn't need to know the return state. Memory eviction may need. Does Memory notifier invalidate range need? > > See one more comment inline. > > >> Signed-off-by: Eric Huang <jinhuieric.huang@amd.com> >> --- >> drivers/gpu/drm/amd/amdkfd/kfd_device.c | 2 +- >> drivers/gpu/drm/amd/amdkfd/kfd_process.c | 24 +----------------------- >> 2 files changed, 2 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c >> index 24b5e0aa1eac..daf1c19bd799 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c >> @@ -984,7 +984,7 @@ int kgd2kfd_post_reset(struct kfd_dev *kfd) >> if (!kfd->init_complete) >> return 0; >> >> - ret = kfd_resume(kfd); >> + ret = kgd2kfd_resume(kfd, false, true); > Which branch is this for? On amd-staging-drm-next kgd2kfd_resume only > has two parameters. Sorry, it is based on dkms staging 5.11. I didn't notice there is difference between two branches. Regards, Eric > > Regards, > Felix > > >> if (ret) >> return ret; >> atomic_dec(&kfd_locked); >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> index 38a9dee40785..9272a12c1db8 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >> @@ -1879,36 +1879,14 @@ int kfd_process_evict_queues(struct kfd_process *p) >> { >> int r = 0; >> int i; >> - unsigned int n_evicted = 0; >> >> for (i = 0; i < p->n_pdds; i++) { >> struct kfd_process_device *pdd = p->pdds[i]; >> >> r = pdd->dev->dqm->ops.evict_process_queues(pdd->dev->dqm, >> &pdd->qpd); >> - if (r) { >> + if (r) >> pr_err("Failed to evict process queues\n"); >> - goto fail; >> - } >> - n_evicted++; >> - } >> - >> - return r; >> - >> -fail: >> - /* To keep state consistent, roll back partial eviction by >> - * restoring queues >> - */ >> - for (i = 0; i < p->n_pdds; i++) { >> - struct kfd_process_device *pdd = p->pdds[i]; >> - >> - if (n_evicted == 0) >> - break; >> - if (pdd->dev->dqm->ops.restore_process_queues(pdd->dev->dqm, >> - &pdd->qpd)) >> - pr_err("Failed to restore queues\n"); >> - >> - n_evicted--; >> } >> >> return r; ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/amdkfd: Restore all process on post reset 2021-08-03 15:02 ` Eric Huang @ 2021-08-03 15:38 ` Felix Kuehling 0 siblings, 0 replies; 4+ messages in thread From: Felix Kuehling @ 2021-08-03 15:38 UTC (permalink / raw) To: Eric Huang, amd-gfx Am 2021-08-03 um 11:02 a.m. schrieb Eric Huang: > > > On 2021-07-30 5:26 p.m., Felix Kuehling wrote: >> Am 2021-07-28 um 1:31 p.m. schrieb Eric Huang: >>> It is to fix a bug of gpu_recovery on multiple GPUs, >>> When one gpu is reset, the application running on other >>> gpu hangs, because kfd post reset doesn't restore the >>> running process. >> This will resume all processes, even those that were affected by the GPU >> reset. >> >> The assumption we made here is, that KFD processes can use all GPUs. So >> when one GPU is reset, all processes are affected. If we want to refine >> that, we'll need some more complex logic to identify the processes >> affected by a GPU reset and keep only those processes suspended. This >> could be based on the GPUs that a process has created queues on, or >> allocated memory on. >> >> What we don't want, is processes continuing with bad data or >> inconsistent state after a GPU reset. >> > Current code doesn't take care of this assumption. When a GPU hangs, > evicting queues will fail on it and roll back to restore all processes > queues on other GPUs, and continue to running with unclear state and > data after a GPU reset. That's a bug. > > The original thought about this patch is to call > kfd_suspend_all_processes and kfd_restore_all_processes in pair on > pre_reset and post_reset. And It keeps the consistent behavior for > both amdgpu_gpu_recover and hang_hws. This is true for suspend/resume, but not for GPU reset. After a GPU reset we want queues of affected processes to stay evicted. >>> And it also fixes a bug in the function >>> kfd_process_evict_queues, when one gpu hangs, process >>> running on other gpus can't be evicted. >> This should be a separate patch. The code you're removing was there as >> an attempt to make kfd_process_evict_queues transactional. That means, >> it either succeeds completely or it fails completely. I wanted to avoid >> putting the system into an unknown state where some queues are suspended >> and others are not and the caller has no idea how to proceed. So I roll >> back a partial queue eviction if something failed. > Can we let the caller to decide if roll-back is needed? because no all > the callers need to roll back, e.g. kfd_suspend_all_processes and > kgd2kfd_quiesce_mm. I don't think so. If suspend partially succeeds, how can the caller do a partial roll back? The caller doesn't have the information about what succeeded and what failed. But you have a good point. It may depend on the caller whether a rollback is needed or not. In the case of GPU reset, we don't care so much about the HW state, but we do care about the queue eviction state. That must be updated in order to prevent queues to resume after the reset, no matter what else fails. This could be done by adding a "bool force" flag to kfd_suspend_all_processes and kfd_process_evict_queues. If the flag is set we don't roll back and we keep going to suspend everything even after partial failures. I think kfd_suspend_all_processes should return an error if force=false and some error occurred so that kgd2kfd_suspend can return a failure. In the case of suspend/resume that can stop the suspend process. >> >> Your changing this to "try to evict as much as possible". Then a failure >> does not mean "eviction failed" but "eviction completed but something >> hung". Then the GPU reset can take care of the hanging part. I think >> that's a reasonable strategy. But we need to be sure that there are no >> other error conditions (other than hangs) that could cause a queue >> eviction to fail. >> >> There were some recent changes in pqm_destroy_queue that check the >> return value of dqm->ops.destroy_queue, which indirectly comes from >> execute_queues (sam as in the eviction case). -ETIME means a hang. Other >> errors are possible. Those other errors may still need the roll-back. >> Otherwise we'd leave stuff running on a non-hanging GPU after we >> promised that we evicted everything. > I think it depends on case scenario. GPU reset doesn't need to know > the return state. Memory eviction may need. Does Memory notifier > invalidate range need? You're right in the sense, that for a GPU reset we don't care about the HW state because we're about to reset it anyway. But we do care about the queue eviction state of the queues (q->properties.is_evicted). After the reset completes we don't want the queues of affected processes to be in evicted state so they are not resumed on the hardware. In the MMU notifier case (kgd2kfd_quiesce_mm) we do care about the HW state. When the MMU notifier returns to the caller, it promises that the HW will no longer access the memory. If the GPU is hanging, that's probably OK as far as the MMU notifier cares. So this is another case where we don't want to roll back. Other error cases (not GPU hangs) would be problematic. Regards, Felix >> >> See one more comment inline. >> >> >>> Signed-off-by: Eric Huang <jinhuieric.huang@amd.com> >>> --- >>> drivers/gpu/drm/amd/amdkfd/kfd_device.c | 2 +- >>> drivers/gpu/drm/amd/amdkfd/kfd_process.c | 24 >>> +----------------------- >>> 2 files changed, 2 insertions(+), 24 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c >>> b/drivers/gpu/drm/amd/amdkfd/kfd_device.c >>> index 24b5e0aa1eac..daf1c19bd799 100644 >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c >>> @@ -984,7 +984,7 @@ int kgd2kfd_post_reset(struct kfd_dev *kfd) >>> if (!kfd->init_complete) >>> return 0; >>> - ret = kfd_resume(kfd); >>> + ret = kgd2kfd_resume(kfd, false, true); >> Which branch is this for? On amd-staging-drm-next kgd2kfd_resume only >> has two parameters. > Sorry, it is based on dkms staging 5.11. I didn't notice there is > difference between two branches. > > Regards, > Eric >> >> Regards, >> Felix >> >> >>> if (ret) >>> return ret; >>> atomic_dec(&kfd_locked); >>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c >>> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >>> index 38a9dee40785..9272a12c1db8 100644 >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c >>> @@ -1879,36 +1879,14 @@ int kfd_process_evict_queues(struct >>> kfd_process *p) >>> { >>> int r = 0; >>> int i; >>> - unsigned int n_evicted = 0; >>> for (i = 0; i < p->n_pdds; i++) { >>> struct kfd_process_device *pdd = p->pdds[i]; >>> r = pdd->dev->dqm->ops.evict_process_queues(pdd->dev->dqm, >>> &pdd->qpd); >>> - if (r) { >>> + if (r) >>> pr_err("Failed to evict process queues\n"); >>> - goto fail; >>> - } >>> - n_evicted++; >>> - } >>> - >>> - return r; >>> - >>> -fail: >>> - /* To keep state consistent, roll back partial eviction by >>> - * restoring queues >>> - */ >>> - for (i = 0; i < p->n_pdds; i++) { >>> - struct kfd_process_device *pdd = p->pdds[i]; >>> - >>> - if (n_evicted == 0) >>> - break; >>> - if (pdd->dev->dqm->ops.restore_process_queues(pdd->dev->dqm, >>> - &pdd->qpd)) >>> - pr_err("Failed to restore queues\n"); >>> - >>> - n_evicted--; >>> } >>> return r; > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-08-03 15:39 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-07-28 17:31 [PATCH] drm/amdkfd: Restore all process on post reset Eric Huang 2021-07-30 21:26 ` Felix Kuehling 2021-08-03 15:02 ` Eric Huang 2021-08-03 15:38 ` Felix Kuehling
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox