iommu/arm-smmu-v3: Shrink command/event/PRI queues in kdump kernel
Subsystems (2)
PATCH v3 1/1 iommu/arm-smmu-v3: Shrink command/event/PRI queues in kdump kernel
All SMMU queues are sized from the maxima the hardware advertises in IDR1, which can be several megabytes each, and are allocated at probe. The kdump kernel already disables the event and PRI queues (arm_smmu_device_reset() drops CR0_EVTQEN/CR0_PRIQEN) but still allocates them at full size. On systems with many SMMUv3 instances that cost is paid per instance and adds up to tens of megabytes of coherent DMA in the capture kernel.
A kdump capture kernel runs from a small crashkernel reservation and only has to drive the few devices used to save the dump, so deep queues serve no purpose. The queues are not on the DMA data path, so dump throughput is unaffected; a shallower command queue only bounds how many commands may be in flight before a sync, which does not matter for the capture kernel's small device count and modest I/O.
Clamp every queue to a single page when is_kdump_kernel() is true. Doing it in arm_smmu_init_one_queue() covers the command, event and PRI queues in one place. The command queue still holds at least one batch plus a sync (256 entries on a 4K-page kernel, well above CMDQ_BATCH_ENTRIES), so command batching keeps working.
---
v3:
- Reword the commit message: all SMMU queues are sized from IDR1, and the event/PRI queues, though disabled in kdump, are still allocated (Pranjal Shrivastava).
- Collect Reviewed-by from Pranjal, Jason and Nicolin; Tested-by from Nicolin.
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +14 -0
@@ -4414,6 +4414,20 @@ int arm_smmu_init_one_queue(struct arm_smmu_device *smmu, { size_t qsz; + /* + * A kdump capture kernel runs from a small crashkernel reservation and + * only has to drive the few devices used to save the dump, so there is + * no point sizing the queues for the (multi-megabyte) maxima the + * hardware advertises. Clamp each queue to a single page. ent_sz_shift + * is the log2 of the entry size in bytes (dwords * 8). + */ + if (is_kdump_kernel()) { + u32 ent_sz_shift = ilog2(dwords) + 3; + + q->llq.max_n_shift = min(q->llq.max_n_shift, + PAGE_SHIFT - ent_sz_shift); + } + do { qsz = ((1 << q->llq.max_n_shift) * dwords) << 3; q->base = dmam_alloc_coherent(smmu->dev, qsz, &q->base_dma,
Signature · 1 line
2.54.0
Quoted 20 lines · Kiryl Shutsemau (Meta)
All SMMU queues are sized from the maxima the hardware advertises in IDR1, which can be several megabytes each, and are allocated at probe. The kdump kernel already disables the event and PRI queues (
arm_smmu_device_reset()drops CR0_EVTQEN/CR0_PRIQEN) but still allocates them at full size. On systems with many SMMUv3 instances that cost is paid per instance and adds up to tens of megabytes of coherent DMA in the capture kernel.A kdump capture kernel runs from a small crashkernel reservation and only has to drive the few devices used to save the dump, so deep queues serve no purpose. The queues are not on the DMA data path, so dump throughput is unaffected; a shallower command queue only bounds how many commands may be in flight before a sync, which does not matter for the capture kernel's small device count and modest I/O.
Clamp every queue to a single page when
is_kdump_kernel()is true. Doing it inarm_smmu_init_one_queue()covers the command, event and PRI queues in one place. The command queue still holds at least one batch plus a sync (256 entries on a 4K-page kernel, well aboveCMDQ_BATCH_ENTRIES), so command batching keeps working.
Quoted 7 lines
Suggested-by Kyle McMartin <[email protected]>Signed-off-by Kiryl Shutsemau (Meta) <[email protected]>Reviewed-by Breno Leitao <[email protected]>Reviewed-by Pranjal Shrivastava <[email protected]>Reviewed-by Jason Gunthorpe <[email protected]>Reviewed-by Nicolin Chen <[email protected]>Tested-by Nicolin Chen <[email protected]>
Gentle ping?
Signature · 1 line
Kiryl Shutsemau / Kirill A. Shutemov
Quoted 29 lines · Kiryl Shutsemau
Quoted 20 lines · Kiryl Shutsemau (Meta)
All SMMU queues are sized from the maxima the hardware advertises in IDR1, which can be several megabytes each, and are allocated at probe. The kdump kernel already disables the event and PRI queues (
arm_smmu_device_reset()drops CR0_EVTQEN/CR0_PRIQEN) but still allocates them at full size. On systems with many SMMUv3 instances that cost is paid per instance and adds up to tens of megabytes of coherent DMA in the capture kernel.A kdump capture kernel runs from a small crashkernel reservation and only has to drive the few devices used to save the dump, so deep queues serve no purpose. The queues are not on the DMA data path, so dump throughput is unaffected; a shallower command queue only bounds how many commands may be in flight before a sync, which does not matter for the capture kernel's small device count and modest I/O.
Clamp every queue to a single page when
is_kdump_kernel()is true. Doing it inarm_smmu_init_one_queue()covers the command, event and PRI queues in one place. The command queue still holds at least one batch plus a sync (256 entries on a 4K-page kernel, well aboveCMDQ_BATCH_ENTRIES), so command batching keeps working.Quoted 7 lines
Suggested-by Kyle McMartin <[email protected]>Signed-off-by Kiryl Shutsemau (Meta) <[email protected]>Reviewed-by Breno Leitao <[email protected]>Reviewed-by Pranjal Shrivastava <[email protected]>Reviewed-by Jason Gunthorpe <[email protected]>Reviewed-by Nicolin Chen <[email protected]>Tested-by Nicolin Chen <[email protected]>
Quoted 1 line
Gentle ping?
Hey folks,
Can we get the patch applied?
Signature · 1 line
Kiryl Shutsemau / Kirill A. Shutemov
Will Deacon
maintainerQuoted 14 lines · Kiryl Shutsemau (Meta)
All SMMU queues are sized from the maxima the hardware advertises in IDR1, which can be several megabytes each, and are allocated at probe. The kdump kernel already disables the event and PRI queues (
arm_smmu_device_reset()drops CR0_EVTQEN/CR0_PRIQEN) but still allocates them at full size. On systems with many SMMUv3 instances that cost is paid per instance and adds up to tens of megabytes of coherent DMA in the capture kernel.A kdump capture kernel runs from a small crashkernel reservation and only has to drive the few devices used to save the dump, so deep queues serve no purpose. The queues are not on the DMA data path, so dump throughput is unaffected; a shallower command queue only bounds how many commands may be in flight before a sync, which does not matter for the capture kernel's small device count and modest I/O.
Quoted 5 lines
Clamp every queue to a single page when
is_kdump_kernel()is true. Doing it inarm_smmu_init_one_queue()covers the command, event and PRI queues in one place. The command queue still holds at least one batch plus a sync (256 entries on a 4K-page kernel, well aboveCMDQ_BATCH_ENTRIES), so command batching keeps working.
Wouldn't we be better of not allocating unused queues in the first place? That's what this patch does:
so help reviewing that series would be much appreciated!
If you want to reduce the cmdq size, I'd prefer a cmdline option rather than special-casing kdump (as I've had other folks ask about configuring the cmdq size for other reasons).
Will
Quoted 21 lines · Will Deacon
Quoted 14 lines · Kiryl Shutsemau (Meta)
All SMMU queues are sized from the maxima the hardware advertises in IDR1, which can be several megabytes each, and are allocated at probe. The kdump kernel already disables the event and PRI queues (
arm_smmu_device_reset()drops CR0_EVTQEN/CR0_PRIQEN) but still allocates them at full size. On systems with many SMMUv3 instances that cost is paid per instance and adds up to tens of megabytes of coherent DMA in the capture kernel.A kdump capture kernel runs from a small crashkernel reservation and only has to drive the few devices used to save the dump, so deep queues serve no purpose. The queues are not on the DMA data path, so dump throughput is unaffected; a shallower command queue only bounds how many commands may be in flight before a sync, which does not matter for the capture kernel's small device count and modest I/O.
Quoted 5 lines
Clamp every queue to a single page when
is_kdump_kernel()is true. Doing it inarm_smmu_init_one_queue()covers the command, event and PRI queues in one place. The command queue still holds at least one batch plus a sync (256 entries on a 4K-page kernel, well aboveCMDQ_BATCH_ENTRIES), so command batching keeps working.
Quoted 4 lines
Wouldn't we be better of not allocating unused queues in the first place? That's what this patch does:
Maybe I'm reading it wrong, but I couldn't find the allocation change in there. That patch seems to only touch arm_smmu_device_reset() -- skipping the EVTQ/PRIQ base/prod/cons writes and the CR0_EVTQEN/CR0_PRIQEN enables instead of the current enable-then-mask-out.
I went through the rest of the series too and didn't see arm_smmu_init_queues() or arm_smmu_init_one_queue() touched anywhere, so as far as I can tell the kdump kernel still allocates full-size evtq/priq, and the cmdq is not affected either.
Is the allocation side handled somewhere else that I've missed?
Quoted 5 lines
so help reviewing that series would be much appreciated!
If you want to reduce the cmdq size, I'd prefer a cmdline option rather than special-casing kdump (as I've had other folks ask about configuring the cmdq size for other reasons).
Fair enough for the cmdq. I'm less sure what to do about evtq/priq: they get disabled in the kdump kernel anyway, so I don't know what a size knob would mean for them, other than every kdump config having to pass it to get back the memory.
Would skipping their allocation when is_kdump_kernel() be acceptable on its own, or would you rather see it done differently?
Signature · 1 line
Kiryl Shutsemau / Kirill A. Shutemov
Will Deacon
maintainerQuoted 32 lines · Kiryl Shutsemau
Quoted 21 lines · Will Deacon
Quoted 14 lines · Kiryl Shutsemau (Meta)
All SMMU queues are sized from the maxima the hardware advertises in IDR1, which can be several megabytes each, and are allocated at probe. The kdump kernel already disables the event and PRI queues (
arm_smmu_device_reset()drops CR0_EVTQEN/CR0_PRIQEN) but still allocates them at full size. On systems with many SMMUv3 instances that cost is paid per instance and adds up to tens of megabytes of coherent DMA in the capture kernel.A kdump capture kernel runs from a small crashkernel reservation and only has to drive the few devices used to save the dump, so deep queues serve no purpose. The queues are not on the DMA data path, so dump throughput is unaffected; a shallower command queue only bounds how many commands may be in flight before a sync, which does not matter for the capture kernel's small device count and modest I/O.
Quoted 5 lines
Clamp every queue to a single page when
is_kdump_kernel()is true. Doing it inarm_smmu_init_one_queue()covers the command, event and PRI queues in one place. The command queue still holds at least one batch plus a sync (256 entries on a 4K-page kernel, well aboveCMDQ_BATCH_ENTRIES), so command batching keeps working.Quoted 4 lines
Wouldn't we be better of not allocating unused queues in the first place? That's what this patch does:
Maybe I'm reading it wrong, but I couldn't find the allocation change in there. That patch seems to only touch
arm_smmu_device_reset()-- skipping the EVTQ/PRIQ base/prod/cons writes and the CR0_EVTQEN/CR0_PRIQEN enables instead of the current enable-then-mask-out.
Quoted 6 lines
I went through the rest of the series too and didn't see
arm_smmu_init_queues()orarm_smmu_init_one_queue()touched anywhere, so as far as I can tell the kdump kernel still allocates full-size evtq/priq, and the cmdq is not affected either.Is the allocation side handled somewhere else that I've missed?
Sorry, yes, it would need extending to avoid allocating the queues now that they're not used at all. I was hoping you and Nicolin could work together on that.
Quoted 11 lines
Quoted 3 lines
If you want to reduce the cmdq size, I'd prefer a cmdline option rather than special-casing kdump (as I've had other folks ask about configuring the cmdq size for other reasons).
Fair enough for the cmdq. I'm less sure what to do about evtq/priq: they get disabled in the kdump kernel anyway, so I don't know what a size knob would mean for them, other than every kdump config having to pass it to get back the memory.
Would skipping their allocation when
is_kdump_kernel()be acceptable on its own, or would you rather see it done differently?
With Nicolin's patch, we can just avoid allocating the priq and the evtq entirely in the kdump case. We can then add a cmdline option to control the maximum size of the cmdq, which is useful regardless of kdump.
Will
Quoted 6 lines · Will Deacon
Quoted 2 lines
Would skipping their allocation when
is_kdump_kernel()be acceptable on its own, or would you rather see it done differently?With Nicolin's patch, we can just avoid allocating the priq and the evtq entirely in the kdump case. We can then add a cmdline option to control the maximum size of the cmdq, which is useful regardless of kdump.
I would still prefer kdump do this re-sizing automatically, even if we do add a commandline. It makes it easire to deploy than having to know secret command lines :\
So functionally I still think this patch is fine, though yes it points out more things in Nicolin's work too.
Jason
Robin Murphy
reviewerQuoted 11 lines · Jason Gunthorpe
Quoted 6 lines · Will Deacon
Quoted 2 lines
Would skipping their allocation when
is_kdump_kernel()be acceptable on its own, or would you rather see it done differently?With Nicolin's patch, we can just avoid allocating the priq and the evtq entirely in the kdump case. We can then add a cmdline option to control the maximum size of the cmdq, which is useful regardless of kdump.
I would still prefer kdump do this re-sizing automatically, even if we do add a commandline. It makes it easire to deploy than having to know secret command lines :\
Right, but if we implement a general command-line option/module parameter/whatever with a variable that needs a default value for when it's not overridden, then it's even more trivial to initialise *that* default value based on further conditions as desired.
Plus implementing the more dynamic mechanism to begin with is arguably nicer than hard-coding a kdump special case in what is effectively the wrong place, since semantically what we really want to override/generalise are the max_n_shift initialisations in arm_smmu_device_hw_probe() itself.
Thanks,
Robin.
Quoted 4 lines
So functionally I still think this patch is fine, though yes it points out more things in Nicolin's work too.
Jason
Quoted 8 lines · Jason Gunthorpe
Quoted 6 lines · Will Deacon
Quoted 2 lines
Would skipping their allocation when
is_kdump_kernel()be acceptable on its own, or would you rather see it done differently?With Nicolin's patch, we can just avoid allocating the priq and the evtq entirely in the kdump case. We can then add a cmdline option to control the maximum size of the cmdq, which is useful regardless of kdump.
Quoted 6 lines
I would still prefer kdump do this re-sizing automatically, even if we do add a commandline. It makes it easire to deploy than having to know secret command lines :\
So functionally I still think this patch is fine, though yes it points out more things in Nicolin's work too.
I will send a v10 of mine, to skip the EVTQ/PRIQ allocations.
Thanks
Nicolin
Quoted 9 lines · Nicolin Chen
Quoted 8 lines · Jason Gunthorpe
Quoted 6 lines · Will Deacon
Quoted 2 lines
Would skipping their allocation when
is_kdump_kernel()be acceptable on its own, or would you rather see it done differently?With Nicolin's patch, we can just avoid allocating the priq and the evtq entirely in the kdump case. We can then add a cmdline option to control the maximum size of the cmdq, which is useful regardless of kdump.
Quoted 8 lines
Quoted 6 lines
I would still prefer kdump do this re-sizing automatically, even if we do add a commandline. It makes it easire to deploy than having to know secret command lines :\
So functionally I still think this patch is fine, though yes it points out more things in Nicolin's work too.
I will send a v10 of mine, to skip the EVTQ/PRIQ allocations.
I think Robin's point is to structure the v10 so you introduce a bunch of tunables, including something like 0 size to disable a feature.
Then have kdump default those tunables to the right thing
Jason
Quoted 17 lines · Jason Gunthorpe
Quoted 16 lines · Nicolin Chen
Quoted 14 lines · Jason Gunthorpe
Quoted 6 lines · Will Deacon
Quoted 2 lines
Would skipping their allocation when
is_kdump_kernel()be acceptable on its own, or would you rather see it done differently?With Nicolin's patch, we can just avoid allocating the priq and the evtq entirely in the kdump case. We can then add a cmdline option to control the maximum size of the cmdq, which is useful regardless of kdump.
I would still prefer kdump do this re-sizing automatically, even if we do add a commandline. It makes it easire to deploy than having to know secret command lines :\
So functionally I still think this patch is fine, though yes it points out more things in Nicolin's work too.
Quoted 6 lines
Quoted 1 line
I will send a v10 of mine, to skip the EVTQ/PRIQ allocations.
I think Robin's point is to structure the v10 so you introduce a bunch of tunables, including something like 0 size to disable a feature.
Then have kdump default those tunables to the right thing
But in kdump, EVTQ/PRIQ are disabled, so my series should have skipped their allocations regardless of the tunables.
So, I think the tunables can be done in a separate series. And its use case might not be confined to kdump?
Nicolin
Quoted 23 lines · Nicolin Chen
Quoted 22 lines · Jason Gunthorpe
Quoted 9 lines · Nicolin Chen
Quoted 8 lines · Jason Gunthorpe
Quoted 6 lines · Will Deacon
Quoted 2 lines
Would skipping their allocation when
is_kdump_kernel()be acceptable on its own, or would you rather see it done differently?With Nicolin's patch, we can just avoid allocating the priq and the evtq entirely in the kdump case. We can then add a cmdline option to control the maximum size of the cmdq, which is useful regardless of kdump.
Quoted 8 lines
Quoted 6 lines
I would still prefer kdump do this re-sizing automatically, even if we do add a commandline. It makes it easire to deploy than having to know secret command lines :\
So functionally I still think this patch is fine, though yes it points out more things in Nicolin's work too.
I will send a v10 of mine, to skip the EVTQ/PRIQ allocations.
I think Robin's point is to structure the v10 so you introduce a bunch of tunables, including something like 0 size to disable a feature.
Quoted 7 lines
Quoted 1 line
Then have kdump default those tunables to the right thing
But in kdump, EVTQ/PRIQ are disabled, so my series should have skipped their allocations regardless of the tunables.
So, I think the tunables can be done in a separate series. And its use case might not be confined to kdump?
I think the ask is to do the tunables first and change your series to default the tunables instead of doing direct kdump overrides.
IDK that anyone wants commandline tunables, but it doesn't seem like a big task
Jason
Quoted 26 lines · Jason Gunthorpe
Quoted 25 lines · Nicolin Chen
Quoted 23 lines · Jason Gunthorpe
Quoted 9 lines · Nicolin Chen
Quoted 8 lines · Jason Gunthorpe
Quoted 6 lines · Will Deacon
Quoted 2 lines
Would skipping their allocation when
is_kdump_kernel()be acceptable on its own, or would you rather see it done differently?With Nicolin's patch, we can just avoid allocating the priq and the evtq entirely in the kdump case. We can then add a cmdline option to control the maximum size of the cmdq, which is useful regardless of kdump.
Quoted 8 lines
Quoted 6 lines
I would still prefer kdump do this re-sizing automatically, even if we do add a commandline. It makes it easire to deploy than having to know secret command lines :\
So functionally I still think this patch is fine, though yes it points out more things in Nicolin's work too.
I will send a v10 of mine, to skip the EVTQ/PRIQ allocations.
I think Robin's point is to structure the v10 so you introduce a bunch of tunables, including something like 0 size to disable a feature.
Then have kdump default those tunables to the right thing
Quoted 8 lines
Quoted 5 lines
But in kdump, EVTQ/PRIQ are disabled, so my series should have skipped their allocations regardless of the tunables.
So, I think the tunables can be done in a separate series. And its use case might not be confined to kdump?
I think the ask is to do the tunables first and change your series to default the tunables instead of doing direct kdump overrides.
You mean the driver can override the tunables to 0 in kdump mode, right?
Well, we are skipping a lot of things in kdump mode already; it's a very small change to skip EVTQ/PRIQ even without the tunables.
If Will prefers this merging sequence, I wouldn't mind changing my series to overriding the tunables.
Thanks
Nicolin
Will Deacon
maintainerQuoted 36 lines · Nicolin Chen
Quoted 26 lines · Jason Gunthorpe
Quoted 25 lines · Nicolin Chen
Quoted 23 lines · Jason Gunthorpe
Quoted 9 lines · Nicolin Chen
Quoted 8 lines · Jason Gunthorpe
Quoted 6 lines · Will Deacon
Quoted 2 lines
Would skipping their allocation when
is_kdump_kernel()be acceptable on its own, or would you rather see it done differently?With Nicolin's patch, we can just avoid allocating the priq and the evtq entirely in the kdump case. We can then add a cmdline option to control the maximum size of the cmdq, which is useful regardless of kdump.
Quoted 8 lines
Quoted 6 lines
I would still prefer kdump do this re-sizing automatically, even if we do add a commandline. It makes it easire to deploy than having to know secret command lines :\
So functionally I still think this patch is fine, though yes it points out more things in Nicolin's work too.
I will send a v10 of mine, to skip the EVTQ/PRIQ allocations.
I think Robin's point is to structure the v10 so you introduce a bunch of tunables, including something like 0 size to disable a feature.
Then have kdump default those tunables to the right thing
Quoted 8 lines
Quoted 5 lines
But in kdump, EVTQ/PRIQ are disabled, so my series should have skipped their allocations regardless of the tunables.
So, I think the tunables can be done in a separate series. And its use case might not be confined to kdump?
I think the ask is to do the tunables first and change your series to default the tunables instead of doing direct kdump overrides.
Quoted 8 lines
You mean the driver can override the tunables to 0 in kdump mode, right?
Well, we are skipping a lot of things in kdump mode already; it's a very small change to skip EVTQ/PRIQ even without the tunables.
If Will prefers this merging sequence, I wouldn't mind changing my series to overriding the tunables.
I was thinking that the tunable would only apply to the cmdq. We can extend it later if necessary, but since we're not going to enable the evtq/priq in kdump mode at all, I don't think we need tunables for those atm.
I probably wouldn't make a size of 0 disable the queue, either. Have a minimum size of a page or something like that.
Will
Quoted 2 lines · Will Deacon
I probably wouldn't make a size of 0 disable the queue, either. Have a minimum size of a page or something like that.
If there is an argument someone is pressed for memory on cmdq they probably also want to disable the fault queue (and fault support entirely) and shrink the event queue too.
The kdump case was an obvious one, but I imagine an embedded system may want similar tuning.
The non-disable-able ones should min to page size..
Jason