| Message ID | 20241120113555.38146-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/irq: fix calculation of maximum pIRQs for dom0 | expand |
On 20/11/2024 11:35 am, Roger Pau Monne wrote: > The current calculation of PV dom0 pIRQs uses: > > n = min(fls(num_present_cpus()), dom0_max_vcpus()); > > The usage of fls() is wrong, as num_present_cpus() already returns the number > of present CPUs, not the bitmap mask of CPUs. > > Fix by removing the usage of fls(). > > Fixes: 7e73a6e7f12a ('have architectures specify the number of PIRQs a hardware domain gets') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Yeah, that fls() fails the dimensional analysis sniff test. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Is there any hint as to what the reasoning was? ~Andrew
On 20.11.2024 12:35, Roger Pau Monne wrote: > The current calculation of PV dom0 pIRQs uses: > > n = min(fls(num_present_cpus()), dom0_max_vcpus()); > > The usage of fls() is wrong, as num_present_cpus() already returns the number > of present CPUs, not the bitmap mask of CPUs. Hmm. Perhaps that use of fls() should have been accompanied by a comment, but I think it might have been put there intentionally, to avoid linear growth. Which isn't to say that I mind the adjustment, especially now that we don't use any clustered modes anymore for I/O interrupts. I'm merely questioning the Fixes: tag, and with that whether / how far to backport. Jan
On Thu, Nov 21, 2024 at 11:49:44AM +0100, Jan Beulich wrote: > On 20.11.2024 12:35, Roger Pau Monne wrote: > > The current calculation of PV dom0 pIRQs uses: > > > > n = min(fls(num_present_cpus()), dom0_max_vcpus()); > > > > The usage of fls() is wrong, as num_present_cpus() already returns the number > > of present CPUs, not the bitmap mask of CPUs. > > Hmm. Perhaps that use of fls() should have been accompanied by a comment, but > I think it might have been put there intentionally, to avoid linear growth. > Which isn't to say that I mind the adjustment, especially now that we don't > use any clustered modes anymore for I/O interrupts. I'm merely questioning > the Fixes: tag, and with that whether / how far to backport. Hm, sorry I've assumed the fls() was a typo. It seems wrong to cap dom0 vCPUs with the fls of the present CPUs number. For consistency, if the intention was to use fls to limit growth, I would have expected to also be applied to the dom0 number of vCPUs. And a comment would have been nice indeed :). In any case this is hurting XenServer now: we got reports of pIRQ exhaustion on some systems. Thanks, Roger.
On 21.11.2024 12:04, Roger Pau Monné wrote: > On Thu, Nov 21, 2024 at 11:49:44AM +0100, Jan Beulich wrote: >> On 20.11.2024 12:35, Roger Pau Monne wrote: >>> The current calculation of PV dom0 pIRQs uses: >>> >>> n = min(fls(num_present_cpus()), dom0_max_vcpus()); >>> >>> The usage of fls() is wrong, as num_present_cpus() already returns the number >>> of present CPUs, not the bitmap mask of CPUs. >> >> Hmm. Perhaps that use of fls() should have been accompanied by a comment, but >> I think it might have been put there intentionally, to avoid linear growth. >> Which isn't to say that I mind the adjustment, especially now that we don't >> use any clustered modes anymore for I/O interrupts. I'm merely questioning >> the Fixes: tag, and with that whether / how far to backport. > > Hm, sorry I've assumed the fls() was a typo. It seems wrong to cap > dom0 vCPUs with the fls of the present CPUs number. For consistency, > if the intention was to use fls to limit growth, I would have expected > to also be applied to the dom0 number of vCPUs. FTR: My vague recollection (it has been nearly 10 years) is that I first had it there, too. Until I realized that it hardly ever would have any effect, because of the min(). And for Dom0-s with extremely few vCPU-s it seemed reasonable to not apply that cap there. Jan
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c index d44d2c9a4173..bd5ad61c85e4 100644 --- a/xen/arch/x86/io_apic.c +++ b/xen/arch/x86/io_apic.c @@ -2744,7 +2744,7 @@ void __init ioapic_init(void) unsigned int __hwdom_init arch_hwdom_irqs(const struct domain *d) { - unsigned int n = fls(num_present_cpus()); + unsigned int n = num_present_cpus(); /* Bounding by the domain pirq EOI bitmap capacity. */ const unsigned int max_irqs = min_t(unsigned int, nr_irqs, PAGE_SIZE * BITS_PER_BYTE);
The current calculation of PV dom0 pIRQs uses: n = min(fls(num_present_cpus()), dom0_max_vcpus()); The usage of fls() is wrong, as num_present_cpus() already returns the number of present CPUs, not the bitmap mask of CPUs. Fix by removing the usage of fls(). Fixes: 7e73a6e7f12a ('have architectures specify the number of PIRQs a hardware domain gets') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/io_apic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)