Repository navigation
Conversation
454b978 to
a2dca4c
Compare
|
Will approve after I get a chance to look at CI logs that the patch applies cleanly on both OS. |
|
Is that CI failure real? I can't see how it could possibly be. |
|
A |
Well the original commit fixed random failures that occurred with workloads that did context switches. But it was predicated on a kernel change being present to work. It wasn't a 100% failure rate, but it was enough to notice. Could you trigger that CI job again and see what happens? |
|
I triggered the test job again and it timed out again. |
jayhawk-commits
left a comment
jayhawk-commits
left a comment
There was a problem hiding this comment.
Changing review status due to test failure
a2dca4c to
183c0b6
Compare
| Subject: [PATCH] Revert "hsakmt: bump vgpr count for gfx1151 (#1807)" | ||
|
|
||
| This reverts commit 770f30bc4c72d763742e39932e2c0583813d531f. | ||
| This reverts part of commit 770f30bc4c72d763742e39932e2c0583813d531f. | ||
| --- |
There was a problem hiding this comment.
If we proceed with this PR or not, this patch needs more information in the commit message.
Why did we revert the commit? What issues did it cause? Why are we carrying the revert via a patch instead of landing the revert in rocm-systems?
There was a problem hiding this comment.
For this ask, the information provided in #2338's description can be leveraged, plus more specifics on the kernel patch not released yet.
There was a problem hiding this comment.
I'll update the description. But I want to be very clear - the kernel patch is released. It's upstream in the 6.18 kernel and already in stable and LTS kernels. The decision to carry this patch is because the downstream Ubuntu kernel that is used for CI doesn't have it.
There was a problem hiding this comment.
The story here isn't adding up for me, so we're going to need to dig in deeper (cc @stellaraccident). From that PR:
In order to not disrupt the next ROCm legacy release and kernel driver release, revert above using a patch that will live on TheRock until the corresponding KFD patch is released and usable on test runners.
We don't evaluate commits or patches based on what kernel drivers are installed on test runners used in a particular repository. We evaluate them based on what our support policy requires, and CI/CD runners are one way that we test that our support policies are satisfied. I don't have a link to those policies on hand (we should absolutely add that to the open source documentation), but any user-space dependency on kernel patches must be carefully rolled out as user-space software (ROCm) is distributed separately from kernel drivers.
In the context of this patch that reverts that commit, what I'm wondering is:
- What does that commit do and why does reverting help in TheRock? The PR description was just "GFX1151 has 1.5x VGPR memory compared to the rest of GFX11."
- If reverting helps us in TheRock, why are we not reverting in rocm-systems?
- Is a fix possible that will make the changes compatible with all supported kernels/drivers (e.g. N, N-1, N-2)? Is that fix in progress?
There was a problem hiding this comment.
Yes Stella met with me and several other people and that's how we landed on this policy. Our plan is that newer kernels the kernel exports source of truth (using cwsr and ctl stack size in sysfs) and rocr-runtime will use that to calculate VGPR size.
Is a fix possible that will make the changes compatible with all supported kernels/drivers (e.g. N, N-1, N-2)? Is that fix in progress?
Older kernels with non-legacy release will have wrong VGPR value. There will be a stability problem but it works for workloads that don't need a context switch. We brainstormed creative ideas, but couldn't come up with one for older kernels because the kernel enforces the size.
If reverting helps us in TheRock, why are we not reverting in rocm-systems?
Good point. I think it went into rocm-systems so that it could backport to 7.2. Based upon the policy discussion I could see an argument to revert there too. I'm not sure how we accomplish that though given this patch is in the TheRock already. It feels like a chicken and egg problem - no?
There was a problem hiding this comment.
The revert of the VGPR change can happen in rocm-systems, with that same PR removing the patch from TheRock in the workflow yml files. This can happen first. Was it decided that this change will not make it to 7.2?
There was a problem hiding this comment.
Here's an attempt at reverting it in rocm-systems. I'm worried how TheRock handles this though, I expect the build probably fails because of that chicken-egg problem.
If that's the case I think we should merge this PR to TheROck, then that one to rocm-systems and then a 3rd to drop this patch file all together in TheRock.
There was a problem hiding this comment.
Thanks for the context. This is sounding good to me. I'd like to get enough of this sort of discussion in commit messages and PR descriptions too, so others watching the development activities can understand what is going on as well.
Based upon the policy discussion I could see an argument to revert there too. I'm not sure how we accomplish that though given this patch is in the TheRock already. It feels like a chicken and egg problem - no?
The revert of the VGPR change can happen in rocm-systems, with that same PR removing the patch from TheRock in the workflow yml files.
Yep. I'm adding some docs for how the patches are managed in #2517. See these sections:
There was a problem hiding this comment.
The change is in 7.2 - very specifically because the documentation ensures the proper kernel is loaded.
183c0b6 to
56b3058
Compare
Although the value is correct; there is no source of truth between kernel and userspace. This leads to problems if the kernel has strict restrictions (such as kernel 6.17 or earlier). The restrictions were lifted in 6.17.9 and and 6.18, but there is no guarantee userspace is using this. So short term this value will be wrong. But on newer kernels the kernel will communicate the right size and rocr-runtime will be adjusted to use that. Link: ROCm/TheRock#2505 Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
Although the value is correct; there is no source of truth between kernel and userspace. This leads to problems if the kernel has strict restrictions (such as kernel 6.17 or earlier). The restrictions were lifted in 6.17.9 and and 6.18, but there is no guarantee userspace is using this. So short term this value will be wrong. But on newer kernels the kernel will communicate the right size and rocr-runtime will be adjusted to use that. Link: ROCm/TheRock#2505 Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
Although the value is correct; there is no source of truth between kernel and userspace. This leads to problems if the kernel has strict restrictions (such as kernel 6.17 or earlier). The restrictions were lifted in 6.17.9 and and 6.18, but there is no guarantee userspace is using this. So short term this value will be wrong. But on newer kernels the kernel will communicate the right size and rocr-runtime will be adjusted to use that. Link: ROCm/TheRock#2505 Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org> [rocm-systems] ROCm/rocm-systems#2268 (commit 0c4d08f)
|
OK so with ROCm/rocm-systems#2268 merged, I think that once TheRock gets the next rocm-systems submodule bump we can drop the patch I was touching here. |
The patch was reverted in rocm-systems and is no longer necessary in TheRock. Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
56b3058 to
cdafc59
Compare
superm1
changed the title
ScottTodd
left a comment
ScottTodd
left a comment
There was a problem hiding this comment.
We get test failures just dropping the patch on its own: https://github.com/ROCm/TheRock/actions/runs/20187543773/job/57962204079?pr=2505
We'll drop the patch when we bump rocm-systems, like on #2533.
OK, I'll just close and we can track that happening in #2533. |
Although the value is correct; there is no source of truth between kernel and userspace. This leads to problems if the kernel has strict restrictions (such as kernel 6.17 or earlier). The restrictions were lifted in 6.17.9 and and 6.18, but there is no guarantee userspace is using this. So short term this value will be wrong. But on newer kernels the kernel will communicate the right size and rocr-runtime will be adjusted to use that. Link: ROCm/TheRock#2505 Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
We need the definition for other code. Just revert the VGPR size bump.