* [PATCH 0/2] x86/irq: fix calculation of maximum pIRQs for dom0
@ 2024-11-20 11:35 Roger Pau Monne
2024-11-20 11:35 ` [PATCH 1/2] x86/irq: fix calculation of max PV dom0 pIRQs Roger Pau Monne
2024-11-20 11:35 ` [PATCH 2/2] x86/pvh: also print hardware domain pIRQ limit for PVH Roger Pau Monne
0 siblings, 2 replies; 15+ messages in thread
From: Roger Pau Monne @ 2024-11-20 11:35 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper
Hello,
First patch is a fix for the calculation of the max number of pIRQs
allowed to dom0, second patch fixes the print of dom0 pIRQ limits so
it's also printed for a PVH dom0.
Thanks, Roger.
Roger Pau Monne (2):
x86/irq: fix calculation of max PV dom0 pIRQs
x86/pvh: also print hardware domain pIRQ limit for PVH
xen/arch/x86/io_apic.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
--
2.46.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] x86/irq: fix calculation of max PV dom0 pIRQs
2024-11-20 11:35 [PATCH 0/2] x86/irq: fix calculation of maximum pIRQs for dom0 Roger Pau Monne
@ 2024-11-20 11:35 ` Roger Pau Monne
2024-11-20 12:06 ` Andrew Cooper
2024-11-21 10:49 ` Jan Beulich
2024-11-20 11:35 ` [PATCH 2/2] x86/pvh: also print hardware domain pIRQ limit for PVH Roger Pau Monne
1 sibling, 2 replies; 15+ messages in thread
From: Roger Pau Monne @ 2024-11-20 11:35 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper
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(-)
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);
--
2.46.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] x86/pvh: also print hardware domain pIRQ limit for PVH
2024-11-20 11:35 [PATCH 0/2] x86/irq: fix calculation of maximum pIRQs for dom0 Roger Pau Monne
2024-11-20 11:35 ` [PATCH 1/2] x86/irq: fix calculation of max PV dom0 pIRQs Roger Pau Monne
@ 2024-11-20 11:35 ` Roger Pau Monne
2024-11-20 11:45 ` Andrew Cooper
2024-11-21 10:54 ` Jan Beulich
1 sibling, 2 replies; 15+ messages in thread
From: Roger Pau Monne @ 2024-11-20 11:35 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper
Do not return early in the PVH/HVM case, so that the number of pIRQs is also
printed.
Fixes: 17f6d398f765 ('cmdline: document and enforce "extra_guest_irqs" upper bounds')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/io_apic.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index bd5ad61c85e4..d9db2efc4f58 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2754,11 +2754,13 @@ unsigned int __hwdom_init arch_hwdom_irqs(const struct domain *d)
/* PVH (generally: HVM) can't use PHYSDEVOP_pirq_eoi_gmfn_v{1,2}. */
if ( is_hvm_domain(d) )
- return nr_irqs;
-
- if ( !d->domain_id )
- n = min(n, dom0_max_vcpus());
- n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, max_irqs);
+ n = nr_irqs;
+ else
+ {
+ if ( !d->domain_id )
+ n = min(n, dom0_max_vcpus());
+ n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, max_irqs);
+ }
printk("%pd has maximum %u PIRQs\n", d, n);
--
2.46.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] x86/pvh: also print hardware domain pIRQ limit for PVH
2024-11-20 11:35 ` [PATCH 2/2] x86/pvh: also print hardware domain pIRQ limit for PVH Roger Pau Monne
@ 2024-11-20 11:45 ` Andrew Cooper
2024-11-21 10:54 ` Jan Beulich
1 sibling, 0 replies; 15+ messages in thread
From: Andrew Cooper @ 2024-11-20 11:45 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel; +Cc: Jan Beulich
On 20/11/2024 11:35 am, Roger Pau Monne wrote:
> Do not return early in the PVH/HVM case, so that the number of pIRQs is also
> printed.
>
> Fixes: 17f6d398f765 ('cmdline: document and enforce "extra_guest_irqs" upper bounds')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] x86/irq: fix calculation of max PV dom0 pIRQs
2024-11-20 11:35 ` [PATCH 1/2] x86/irq: fix calculation of max PV dom0 pIRQs Roger Pau Monne
@ 2024-11-20 12:06 ` Andrew Cooper
2024-11-21 10:49 ` Jan Beulich
1 sibling, 0 replies; 15+ messages in thread
From: Andrew Cooper @ 2024-11-20 12:06 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel; +Cc: Jan Beulich
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] x86/irq: fix calculation of max PV dom0 pIRQs
2024-11-20 11:35 ` [PATCH 1/2] x86/irq: fix calculation of max PV dom0 pIRQs Roger Pau Monne
2024-11-20 12:06 ` Andrew Cooper
@ 2024-11-21 10:49 ` Jan Beulich
2024-11-21 11:04 ` Roger Pau Monné
1 sibling, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2024-11-21 10:49 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] x86/pvh: also print hardware domain pIRQ limit for PVH
2024-11-20 11:35 ` [PATCH 2/2] x86/pvh: also print hardware domain pIRQ limit for PVH Roger Pau Monne
2024-11-20 11:45 ` Andrew Cooper
@ 2024-11-21 10:54 ` Jan Beulich
2024-11-21 11:08 ` Roger Pau Monné
1 sibling, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2024-11-21 10:54 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel
On 20.11.2024 12:35, Roger Pau Monne wrote:
> Do not return early in the PVH/HVM case, so that the number of pIRQs is also
> printed.
What you're printing ...
> Fixes: 17f6d398f765 ('cmdline: document and enforce "extra_guest_irqs" upper bounds')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> xen/arch/x86/io_apic.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index bd5ad61c85e4..d9db2efc4f58 100644
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -2754,11 +2754,13 @@ unsigned int __hwdom_init arch_hwdom_irqs(const struct domain *d)
>
> /* PVH (generally: HVM) can't use PHYSDEVOP_pirq_eoi_gmfn_v{1,2}. */
> if ( is_hvm_domain(d) )
> - return nr_irqs;
> -
> - if ( !d->domain_id )
> - n = min(n, dom0_max_vcpus());
> - n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, max_irqs);
> + n = nr_irqs;
... is rather the number of IRQs we picked for the system. That may happen to
end up being the upper bound for PVH Dom0, yet not logging this at all was
because of the limited use pIRQ-s have there. Granted at the time I was still
under the impression they have no use there at all, so this isn't really an
objection to the change. I would have been nice though if the description had
mentioned why significance pIRQ-s actually have in PVH Dom0.
Jan
> + else
> + {
> + if ( !d->domain_id )
> + n = min(n, dom0_max_vcpus());
> + n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, max_irqs);
> + }
>
> printk("%pd has maximum %u PIRQs\n", d, n);
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] x86/irq: fix calculation of max PV dom0 pIRQs
2024-11-21 10:49 ` Jan Beulich
@ 2024-11-21 11:04 ` Roger Pau Monné
2024-11-21 11:39 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Roger Pau Monné @ 2024-11-21 11:04 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] x86/pvh: also print hardware domain pIRQ limit for PVH
2024-11-21 10:54 ` Jan Beulich
@ 2024-11-21 11:08 ` Roger Pau Monné
2024-11-21 11:14 ` Andrew Cooper
0 siblings, 1 reply; 15+ messages in thread
From: Roger Pau Monné @ 2024-11-21 11:08 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
On Thu, Nov 21, 2024 at 11:54:49AM +0100, Jan Beulich wrote:
> On 20.11.2024 12:35, Roger Pau Monne wrote:
> > Do not return early in the PVH/HVM case, so that the number of pIRQs is also
> > printed.
>
> What you're printing ...
>
> > Fixes: 17f6d398f765 ('cmdline: document and enforce "extra_guest_irqs" upper bounds')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > xen/arch/x86/io_apic.c | 12 +++++++-----
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> > index bd5ad61c85e4..d9db2efc4f58 100644
> > --- a/xen/arch/x86/io_apic.c
> > +++ b/xen/arch/x86/io_apic.c
> > @@ -2754,11 +2754,13 @@ unsigned int __hwdom_init arch_hwdom_irqs(const struct domain *d)
> >
> > /* PVH (generally: HVM) can't use PHYSDEVOP_pirq_eoi_gmfn_v{1,2}. */
> > if ( is_hvm_domain(d) )
> > - return nr_irqs;
> > -
> > - if ( !d->domain_id )
> > - n = min(n, dom0_max_vcpus());
> > - n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, max_irqs);
> > + n = nr_irqs;
>
> ... is rather the number of IRQs we picked for the system. That may happen to
> end up being the upper bound for PVH Dom0, yet not logging this at all was
> because of the limited use pIRQ-s have there. Granted at the time I was still
> under the impression they have no use there at all, so this isn't really an
> objection to the change. I would have been nice though if the description had
> mentioned why significance pIRQ-s actually have in PVH Dom0.
Sure, what about adding to the commit message:
"While PVH dom0 doesn't have access to the hypercalls to manage pIRQs
itself, neither the knowledge to do so, pIRQs are still used by Xen to
map and bind interrupts to a PVH dom0 behind its back. Hence the
pIRQ limit is still relevant for a PVH dom0."
Thanks, Roger.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] x86/pvh: also print hardware domain pIRQ limit for PVH
2024-11-21 11:08 ` Roger Pau Monné
@ 2024-11-21 11:14 ` Andrew Cooper
2024-11-21 11:24 ` Roger Pau Monné
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2024-11-21 11:14 UTC (permalink / raw)
To: Roger Pau Monné, Jan Beulich; +Cc: xen-devel
On 21/11/2024 11:08 am, Roger Pau Monné wrote:
> On Thu, Nov 21, 2024 at 11:54:49AM +0100, Jan Beulich wrote:
>> On 20.11.2024 12:35, Roger Pau Monne wrote:
>>> Do not return early in the PVH/HVM case, so that the number of pIRQs is also
>>> printed.
>> What you're printing ...
>>
>>> Fixes: 17f6d398f765 ('cmdline: document and enforce "extra_guest_irqs" upper bounds')
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> xen/arch/x86/io_apic.c | 12 +++++++-----
>>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
>>> index bd5ad61c85e4..d9db2efc4f58 100644
>>> --- a/xen/arch/x86/io_apic.c
>>> +++ b/xen/arch/x86/io_apic.c
>>> @@ -2754,11 +2754,13 @@ unsigned int __hwdom_init arch_hwdom_irqs(const struct domain *d)
>>>
>>> /* PVH (generally: HVM) can't use PHYSDEVOP_pirq_eoi_gmfn_v{1,2}. */
>>> if ( is_hvm_domain(d) )
>>> - return nr_irqs;
>>> -
>>> - if ( !d->domain_id )
>>> - n = min(n, dom0_max_vcpus());
>>> - n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, max_irqs);
>>> + n = nr_irqs;
>> ... is rather the number of IRQs we picked for the system. That may happen to
>> end up being the upper bound for PVH Dom0, yet not logging this at all was
>> because of the limited use pIRQ-s have there. Granted at the time I was still
>> under the impression they have no use there at all, so this isn't really an
>> objection to the change. I would have been nice though if the description had
>> mentioned why significance pIRQ-s actually have in PVH Dom0.
> Sure, what about adding to the commit message:
>
> "While PVH dom0 doesn't have access to the hypercalls to manage pIRQs
> itself, neither the knowledge to do so, pIRQs are still used by Xen to
> map and bind interrupts to a PVH dom0 behind its back. Hence the
> pIRQ limit is still relevant for a PVH dom0."
Minor grammar point. You want "nor" rather than "neither" in this
context, because it's introducing the second of two negative things.
~Andrew
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] x86/pvh: also print hardware domain pIRQ limit for PVH
2024-11-21 11:14 ` Andrew Cooper
@ 2024-11-21 11:24 ` Roger Pau Monné
2024-11-21 11:27 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Roger Pau Monné @ 2024-11-21 11:24 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Jan Beulich, xen-devel
On Thu, Nov 21, 2024 at 11:14:23AM +0000, Andrew Cooper wrote:
> On 21/11/2024 11:08 am, Roger Pau Monné wrote:
> > On Thu, Nov 21, 2024 at 11:54:49AM +0100, Jan Beulich wrote:
> >> On 20.11.2024 12:35, Roger Pau Monne wrote:
> >>> Do not return early in the PVH/HVM case, so that the number of pIRQs is also
> >>> printed.
> >> What you're printing ...
> >>
> >>> Fixes: 17f6d398f765 ('cmdline: document and enforce "extra_guest_irqs" upper bounds')
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>> ---
> >>> xen/arch/x86/io_apic.c | 12 +++++++-----
> >>> 1 file changed, 7 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> >>> index bd5ad61c85e4..d9db2efc4f58 100644
> >>> --- a/xen/arch/x86/io_apic.c
> >>> +++ b/xen/arch/x86/io_apic.c
> >>> @@ -2754,11 +2754,13 @@ unsigned int __hwdom_init arch_hwdom_irqs(const struct domain *d)
> >>>
> >>> /* PVH (generally: HVM) can't use PHYSDEVOP_pirq_eoi_gmfn_v{1,2}. */
> >>> if ( is_hvm_domain(d) )
> >>> - return nr_irqs;
> >>> -
> >>> - if ( !d->domain_id )
> >>> - n = min(n, dom0_max_vcpus());
> >>> - n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, max_irqs);
> >>> + n = nr_irqs;
> >> ... is rather the number of IRQs we picked for the system. That may happen to
> >> end up being the upper bound for PVH Dom0, yet not logging this at all was
> >> because of the limited use pIRQ-s have there. Granted at the time I was still
> >> under the impression they have no use there at all, so this isn't really an
> >> objection to the change. I would have been nice though if the description had
> >> mentioned why significance pIRQ-s actually have in PVH Dom0.
> > Sure, what about adding to the commit message:
> >
> > "While PVH dom0 doesn't have access to the hypercalls to manage pIRQs
> > itself, neither the knowledge to do so, pIRQs are still used by Xen to
> > map and bind interrupts to a PVH dom0 behind its back. Hence the
> > pIRQ limit is still relevant for a PVH dom0."
>
> Minor grammar point. You want "nor" rather than "neither" in this
> context, because it's introducing the second of two negative things.
Thanks! Could one of you adjust at commit if Jan agrees with adding
the paragraph?
Regards, Roger.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] x86/pvh: also print hardware domain pIRQ limit for PVH
2024-11-21 11:24 ` Roger Pau Monné
@ 2024-11-21 11:27 ` Jan Beulich
0 siblings, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2024-11-21 11:27 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper
On 21.11.2024 12:24, Roger Pau Monné wrote:
> On Thu, Nov 21, 2024 at 11:14:23AM +0000, Andrew Cooper wrote:
>> On 21/11/2024 11:08 am, Roger Pau Monné wrote:
>>> On Thu, Nov 21, 2024 at 11:54:49AM +0100, Jan Beulich wrote:
>>>> On 20.11.2024 12:35, Roger Pau Monne wrote:
>>>>> Do not return early in the PVH/HVM case, so that the number of pIRQs is also
>>>>> printed.
>>>> What you're printing ...
>>>>
>>>>> Fixes: 17f6d398f765 ('cmdline: document and enforce "extra_guest_irqs" upper bounds')
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>> ---
>>>>> xen/arch/x86/io_apic.c | 12 +++++++-----
>>>>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
>>>>> index bd5ad61c85e4..d9db2efc4f58 100644
>>>>> --- a/xen/arch/x86/io_apic.c
>>>>> +++ b/xen/arch/x86/io_apic.c
>>>>> @@ -2754,11 +2754,13 @@ unsigned int __hwdom_init arch_hwdom_irqs(const struct domain *d)
>>>>>
>>>>> /* PVH (generally: HVM) can't use PHYSDEVOP_pirq_eoi_gmfn_v{1,2}. */
>>>>> if ( is_hvm_domain(d) )
>>>>> - return nr_irqs;
>>>>> -
>>>>> - if ( !d->domain_id )
>>>>> - n = min(n, dom0_max_vcpus());
>>>>> - n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, max_irqs);
>>>>> + n = nr_irqs;
>>>> ... is rather the number of IRQs we picked for the system. That may happen to
>>>> end up being the upper bound for PVH Dom0, yet not logging this at all was
>>>> because of the limited use pIRQ-s have there. Granted at the time I was still
>>>> under the impression they have no use there at all, so this isn't really an
>>>> objection to the change. I would have been nice though if the description had
>>>> mentioned why significance pIRQ-s actually have in PVH Dom0.
>>> Sure, what about adding to the commit message:
>>>
>>> "While PVH dom0 doesn't have access to the hypercalls to manage pIRQs
>>> itself, neither the knowledge to do so, pIRQs are still used by Xen to
>>> map and bind interrupts to a PVH dom0 behind its back. Hence the
>>> pIRQ limit is still relevant for a PVH dom0."
>>
>> Minor grammar point. You want "nor" rather than "neither" in this
>> context, because it's introducing the second of two negative things.
>
> Thanks! Could one of you adjust at commit if Jan agrees with adding
> the paragraph?
Sounds good, and certainly not a problem to add while committing.
Jan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] x86/irq: fix calculation of max PV dom0 pIRQs
2024-11-21 11:04 ` Roger Pau Monné
@ 2024-11-21 11:39 ` Jan Beulich
2024-11-22 8:01 ` Roger Pau Monné
0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2024-11-21 11:39 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: Andrew Cooper, xen-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] x86/irq: fix calculation of max PV dom0 pIRQs
2024-11-21 11:39 ` Jan Beulich
@ 2024-11-22 8:01 ` Roger Pau Monné
2024-11-22 9:27 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Roger Pau Monné @ 2024-11-22 8:01 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
On Thu, Nov 21, 2024 at 12:39:06PM +0100, Jan Beulich wrote:
> 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.
I don't have a strong opinion regarding the fixes tag, but would like
to get this sorted as soon as possible. If you prefer just drop the
fixes tag. I think this wants backporting to all supported releases,
because it's an issue XenServer has hit on real servers when dom0 is
sized to use 16 vCPUs at most.
Thanks, Roger.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] x86/irq: fix calculation of max PV dom0 pIRQs
2024-11-22 8:01 ` Roger Pau Monné
@ 2024-11-22 9:27 ` Jan Beulich
0 siblings, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2024-11-22 9:27 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: Andrew Cooper, xen-devel
On 22.11.2024 09:01, Roger Pau Monné wrote:
> On Thu, Nov 21, 2024 at 12:39:06PM +0100, Jan Beulich wrote:
>> 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.
>
> I don't have a strong opinion regarding the fixes tag, but would like
> to get this sorted as soon as possible. If you prefer just drop the
> fixes tag. I think this wants backporting to all supported releases,
> because it's an issue XenServer has hit on real servers when dom0 is
> sized to use 16 vCPUs at most.
In which case yes, I guess we want to keep the Fixes:.
Jan
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-11-22 9:28 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-20 11:35 [PATCH 0/2] x86/irq: fix calculation of maximum pIRQs for dom0 Roger Pau Monne
2024-11-20 11:35 ` [PATCH 1/2] x86/irq: fix calculation of max PV dom0 pIRQs Roger Pau Monne
2024-11-20 12:06 ` Andrew Cooper
2024-11-21 10:49 ` Jan Beulich
2024-11-21 11:04 ` Roger Pau Monné
2024-11-21 11:39 ` Jan Beulich
2024-11-22 8:01 ` Roger Pau Monné
2024-11-22 9:27 ` Jan Beulich
2024-11-20 11:35 ` [PATCH 2/2] x86/pvh: also print hardware domain pIRQ limit for PVH Roger Pau Monne
2024-11-20 11:45 ` Andrew Cooper
2024-11-21 10:54 ` Jan Beulich
2024-11-21 11:08 ` Roger Pau Monné
2024-11-21 11:14 ` Andrew Cooper
2024-11-21 11:24 ` Roger Pau Monné
2024-11-21 11:27 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.