[PATCHv1] xen: increase default number of PIRQs for hardware domains

[PATCHv1] xen: increase default number of PIRQs for hardware domains

[David Vrabel]

The default limit for the number of PIRQs for hardware domains (dom0)
is not sufficient for some (x86) systems.

Since the pirq structures are individually and dynamically allocated,
the limit for hardware domains may be increased to the number of
possible IRQs.

The extra_guest_irqs command line option now only allows changes to
the domU value.  Any argument for dom0 is ignored.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 docs/misc/xen-command-line.markdown |   11 ++++-------
 xen/common/domain.c                 |    7 +------
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 0866df2..d352031 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -594,15 +594,12 @@ except for debugging purposes.
 Force or disable use of EFI runtime services.
 
 ### extra\_guest\_irqs
-> `= [<domU number>][,<dom0 number>]`
+> `= [<number>]`
 
-> Default: `32,256`
+> Default: `32`
 
-Change the number of PIRQs available for guests.  The optional first number is
-common for all domUs, while the optional second number (preceded by a comma)
-is for dom0.  Changing the setting for domU has no impact on dom0 and vice
-versa.  For example to change dom0 without changing domU, use
-`extra_guest_irqs=,512`
+Change the number of PIRQs available for guests. This limit does not
+apply to hardware domains (dom0).
 
 ### flask\_enabled
 > `= <integer>`
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 4a62c1d..a88d829 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -231,14 +231,11 @@ static int late_hwdom_init(struct domain *d)
 #endif
 }
 
-static unsigned int __read_mostly extra_dom0_irqs = 256;
 static unsigned int __read_mostly extra_domU_irqs = 32;
 static void __init parse_extra_guest_irqs(const char *s)
 {
     if ( isdigit(*s) )
         extra_domU_irqs = simple_strtoul(s, &s, 0);
-    if ( *s == ',' && isdigit(*++s) )
-        extra_dom0_irqs = simple_strtoul(s, &s, 0);
 }
 custom_param("extra_guest_irqs", parse_extra_guest_irqs);
 
@@ -324,10 +321,8 @@ struct domain *domain_create(
         atomic_inc(&d->pause_count);
 
         if ( !is_hardware_domain(d) )
-            d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
+            d->nr_pirqs = min(nr_static_irqs + extra_domU_irqs, nr_irqs);
         else
-            d->nr_pirqs = nr_static_irqs + extra_dom0_irqs;
-        if ( d->nr_pirqs > nr_irqs )
             d->nr_pirqs = nr_irqs;
 
         radix_tree_init(&d->pirq_tree);
-- 
1.7.10.4

Re: [PATCHv1] xen: increase default number of PIRQs for hardware domains

[Andrew Cooper]

On 03/12/14 16:04, David Vrabel wrote:
> The default limit for the number of PIRQs for hardware domains (dom0)
> is not sufficient for some (x86) systems.
>
> Since the pirq structures are individually and dynamically allocated,
> the limit for hardware domains may be increased to the number of
> possible IRQs.
>
> The extra_guest_irqs command line option now only allows changes to
> the domU value.  Any argument for dom0 is ignored.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  docs/misc/xen-command-line.markdown |   11 ++++-------
>  xen/common/domain.c                 |    7 +------
>  2 files changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 0866df2..d352031 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -594,15 +594,12 @@ except for debugging purposes.
>  Force or disable use of EFI runtime services.
>  
>  ### extra\_guest\_irqs
> -> `= [<domU number>][,<dom0 number>]`
> +> `= [<number>]`
>  
> -> Default: `32,256`
> +> Default: `32`
>  
> -Change the number of PIRQs available for guests.  The optional first number is
> -common for all domUs, while the optional second number (preceded by a comma)
> -is for dom0.  Changing the setting for domU has no impact on dom0 and vice
> -versa.  For example to change dom0 without changing domU, use
> -`extra_guest_irqs=,512`
> +Change the number of PIRQs available for guests. This limit does not
> +apply to hardware domains (dom0).
>  
>  ### flask\_enabled
>  > `= <integer>`
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 4a62c1d..a88d829 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -231,14 +231,11 @@ static int late_hwdom_init(struct domain *d)
>  #endif
>  }
>  
> -static unsigned int __read_mostly extra_dom0_irqs = 256;
>  static unsigned int __read_mostly extra_domU_irqs = 32;
>  static void __init parse_extra_guest_irqs(const char *s)
>  {
>      if ( isdigit(*s) )
>          extra_domU_irqs = simple_strtoul(s, &s, 0);
> -    if ( *s == ',' && isdigit(*++s) )
> -        extra_dom0_irqs = simple_strtoul(s, &s, 0);
>  }
>  custom_param("extra_guest_irqs", parse_extra_guest_irqs);
>  
> @@ -324,10 +321,8 @@ struct domain *domain_create(
>          atomic_inc(&d->pause_count);
>  
>          if ( !is_hardware_domain(d) )
> -            d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
> +            d->nr_pirqs = min(nr_static_irqs + extra_domU_irqs, nr_irqs);
>          else
> -            d->nr_pirqs = nr_static_irqs + extra_dom0_irqs;
> -        if ( d->nr_pirqs > nr_irqs )
>              d->nr_pirqs = nr_irqs;
>  
>          radix_tree_init(&d->pirq_tree);

Re: [PATCHv1] xen: increase default number of PIRQs for hardware domains

[Konrad Rzeszutek Wilk]

On Wed, Dec 03, 2014 at 04:04:20PM +0000, David Vrabel wrote:
> The default limit for the number of PIRQs for hardware domains (dom0)
> is not sufficient for some (x86) systems.
> 
> Since the pirq structures are individually and dynamically allocated,
> the limit for hardware domains may be increased to the number of
> possible IRQs.

Why not also expand the number for the guest?
> 
> The extra_guest_irqs command line option now only allows changes to
> the domU value.  Any argument for dom0 is ignored.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  docs/misc/xen-command-line.markdown |   11 ++++-------
>  xen/common/domain.c                 |    7 +------
>  2 files changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 0866df2..d352031 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -594,15 +594,12 @@ except for debugging purposes.
>  Force or disable use of EFI runtime services.
>  
>  ### extra\_guest\_irqs
> -> `= [<domU number>][,<dom0 number>]`
> +> `= [<number>]`
>  
> -> Default: `32,256`
> +> Default: `32`
>  
> -Change the number of PIRQs available for guests.  The optional first number is
> -common for all domUs, while the optional second number (preceded by a comma)
> -is for dom0.  Changing the setting for domU has no impact on dom0 and vice
> -versa.  For example to change dom0 without changing domU, use
> -`extra_guest_irqs=,512`
> +Change the number of PIRQs available for guests. This limit does not
> +apply to hardware domains (dom0).
>  
>  ### flask\_enabled
>  > `= <integer>`
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 4a62c1d..a88d829 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -231,14 +231,11 @@ static int late_hwdom_init(struct domain *d)
>  #endif
>  }
>  
> -static unsigned int __read_mostly extra_dom0_irqs = 256;
>  static unsigned int __read_mostly extra_domU_irqs = 32;
>  static void __init parse_extra_guest_irqs(const char *s)
>  {
>      if ( isdigit(*s) )
>          extra_domU_irqs = simple_strtoul(s, &s, 0);
> -    if ( *s == ',' && isdigit(*++s) )
> -        extra_dom0_irqs = simple_strtoul(s, &s, 0);
>  }
>  custom_param("extra_guest_irqs", parse_extra_guest_irqs);
>  
> @@ -324,10 +321,8 @@ struct domain *domain_create(
>          atomic_inc(&d->pause_count);
>  
>          if ( !is_hardware_domain(d) )
> -            d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
> +            d->nr_pirqs = min(nr_static_irqs + extra_domU_irqs, nr_irqs);
>          else
> -            d->nr_pirqs = nr_static_irqs + extra_dom0_irqs;
> -        if ( d->nr_pirqs > nr_irqs )
>              d->nr_pirqs = nr_irqs;
>  
>          radix_tree_init(&d->pirq_tree);
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCHv1] xen: increase default number of PIRQs for hardware domains

[David Vrabel]

On 03/12/14 20:38, Konrad Rzeszutek Wilk wrote:
> On Wed, Dec 03, 2014 at 04:04:20PM +0000, David Vrabel wrote:
>> The default limit for the number of PIRQs for hardware domains (dom0)
>> is not sufficient for some (x86) systems.
>>
>> Since the pirq structures are individually and dynamically allocated,
>> the limit for hardware domains may be increased to the number of
>> possible IRQs.
> 
> Why not also expand the number for the guest?

Because the default doesn't need to be increased for domUs at this time
and I did not want to audit the code to make sure a guest can't (for
example) repeatedly map PIRQs, using up all pirqs/irqs in Xen.

David

Re: [PATCHv1] xen: increase default number of PIRQs for hardware domains

[Jan Beulich]

>>> On 03.12.14 at 17:04, <david.vrabel@citrix.com> wrote:
> The default limit for the number of PIRQs for hardware domains (dom0)
> is not sufficient for some (x86) systems.
> 
> Since the pirq structures are individually and dynamically allocated,
> the limit for hardware domains may be increased to the number of
> possible IRQs.

I nevertheless disagree to moving the bound up to the Xen internal
limit unconditionally: What use does it have to allow hwdom to use
thousands of MSIs? If a system got that many, the main purpose of
running Xen on it I would expect to be to hand various of the
respective devices to guests. Hence no need for hwdom to have
that many by default, even if this doesn't result in any extra
resource consumption.

That said, I can see the current default of 256 being too low though.
Quite likely in the absence of a user specified value the default
ought to be derived from nr_irqs - nr_static_irqs rather than being
any fixed number. Considering the default used for nr_irqs, I'd think
along the lines of sqrt(num_present_cpus()) * NR_DYNAMIC_VECTORS
or dom0->max_vcpus * NR_DYNAMIC_VECTORS (or the minimum of
the two) for x86.

Jan

> The extra_guest_irqs command line option now only allows changes to
> the domU value.  Any argument for dom0 is ignored.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  docs/misc/xen-command-line.markdown |   11 ++++-------
>  xen/common/domain.c                 |    7 +------
>  2 files changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown 
> b/docs/misc/xen-command-line.markdown
> index 0866df2..d352031 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -594,15 +594,12 @@ except for debugging purposes.
>  Force or disable use of EFI runtime services.
>  
>  ### extra\_guest\_irqs
> -> `= [<domU number>][,<dom0 number>]`
> +> `= [<number>]`
>  
> -> Default: `32,256`
> +> Default: `32`
>  
> -Change the number of PIRQs available for guests.  The optional first number 
> is
> -common for all domUs, while the optional second number (preceded by a comma)
> -is for dom0.  Changing the setting for domU has no impact on dom0 and vice
> -versa.  For example to change dom0 without changing domU, use
> -`extra_guest_irqs=,512`
> +Change the number of PIRQs available for guests. This limit does not
> +apply to hardware domains (dom0).
>  
>  ### flask\_enabled
>  > `= <integer>`
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 4a62c1d..a88d829 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -231,14 +231,11 @@ static int late_hwdom_init(struct domain *d)
>  #endif
>  }
>  
> -static unsigned int __read_mostly extra_dom0_irqs = 256;
>  static unsigned int __read_mostly extra_domU_irqs = 32;
>  static void __init parse_extra_guest_irqs(const char *s)
>  {
>      if ( isdigit(*s) )
>          extra_domU_irqs = simple_strtoul(s, &s, 0);
> -    if ( *s == ',' && isdigit(*++s) )
> -        extra_dom0_irqs = simple_strtoul(s, &s, 0);
>  }
>  custom_param("extra_guest_irqs", parse_extra_guest_irqs);
>  
> @@ -324,10 +321,8 @@ struct domain *domain_create(
>          atomic_inc(&d->pause_count);
>  
>          if ( !is_hardware_domain(d) )
> -            d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
> +            d->nr_pirqs = min(nr_static_irqs + extra_domU_irqs, nr_irqs);
>          else
> -            d->nr_pirqs = nr_static_irqs + extra_dom0_irqs;
> -        if ( d->nr_pirqs > nr_irqs )
>              d->nr_pirqs = nr_irqs;
>  
>          radix_tree_init(&d->pirq_tree);
> -- 
> 1.7.10.4

Re: [PATCHv1] xen: increase default number of PIRQs for hardware domains

[David Vrabel]

On 05/12/14 09:44, Jan Beulich wrote:
>>>> On 03.12.14 at 17:04, <david.vrabel@citrix.com> wrote:
>> The default limit for the number of PIRQs for hardware domains (dom0)
>> is not sufficient for some (x86) systems.
>>
>> Since the pirq structures are individually and dynamically allocated,
>> the limit for hardware domains may be increased to the number of
>> possible IRQs.
> 
> I nevertheless disagree to moving the bound up to the Xen internal
> limit unconditionally: What use does it have to allow hwdom to use
> thousands of MSIs? If a system got that many, the main purpose of
> running Xen on it I would expect to be to hand various of the
> respective devices to guests. Hence no need for hwdom to have
> that many by default, even if this doesn't result in any extra
> resource consumption.
> 
> That said, I can see the current default of 256 being too low though.
> Quite likely in the absence of a user specified value the default
> ought to be derived from nr_irqs - nr_static_irqs rather than being
> any fixed number. Considering the default used for nr_irqs, I'd think
> along the lines of sqrt(num_present_cpus()) * NR_DYNAMIC_VECTORS
> or dom0->max_vcpus * NR_DYNAMIC_VECTORS (or the minimum of
> the two) for x86.

The reason for a non obvious default like this would need a comment and
I can't write one because I can't see a reason for it.  Perhaps if you
write a suitable comment for your preferred default I can respin this patch?

David

Re: [PATCHv1] xen: increase default number of PIRQs for hardware domains

[Andrew Cooper]

On 05/12/14 09:44, Jan Beulich wrote:
>>>> On 03.12.14 at 17:04, <david.vrabel@citrix.com> wrote:
>> The default limit for the number of PIRQs for hardware domains (dom0)
>> is not sufficient for some (x86) systems.
>>
>> Since the pirq structures are individually and dynamically allocated,
>> the limit for hardware domains may be increased to the number of
>> possible IRQs.
> I nevertheless disagree to moving the bound up to the Xen internal
> limit unconditionally: What use does it have to allow hwdom to use
> thousands of MSIs?

Because systems that big exist.  We have one.  In particular, it needs
somewhere between 288 and 512 pirqs to scan the bus and bring up the
physical functions alone.

> If a system got that many, the main purpose of
> running Xen on it I would expect to be to hand various of the
> respective devices to guests. Hence no need for hwdom to have
> that many by default, even if this doesn't result in any extra
> resource consumption.
>
> That said, I can see the current default of 256 being too low though.
> Quite likely in the absence of a user specified value the default
> ought to be derived from nr_irqs - nr_static_irqs rather than being
> any fixed number. Considering the default used for nr_irqs, I'd think
> along the lines of sqrt(num_present_cpus()) * NR_DYNAMIC_VECTORS
> or dom0->max_vcpus * NR_DYNAMIC_VECTORS (or the minimum of
> the two) for x86.

The hardware domain is trusted ultimately.  It can, amongst other
things, rewrite the bootloader command line and replace xen.gz.  It can
be trusted not to maliciously waste Xen resource.

Having an arbitrary restriction on the the hardware domains means only
that, in the case the arbitrary limit is hit, system devices fail to
function properly.  This is far more noticeable if the limit is hit
during probe.  The admin can edit the bootloader and increase the limit,
but only if the root disk was a driver lucky enough to get its
interrupt, or the default network card got its interrupts.

The limit serves no security or resource purpose, but has the chance of
crippling the boot of the system, and making recovery hard or
impossible.  On this justification alone, the limit should be removed.

~Andrew

Re: [PATCHv1] xen: increase default number of PIRQs for hardware domains

[Jan Beulich]

>>> On 05.12.14 at 13:02, <andrew.cooper3@citrix.com> wrote:
> On 05/12/14 09:44, Jan Beulich wrote:
>>>>> On 03.12.14 at 17:04, <david.vrabel@citrix.com> wrote:
>>> The default limit for the number of PIRQs for hardware domains (dom0)
>>> is not sufficient for some (x86) systems.
>>>
>>> Since the pirq structures are individually and dynamically allocated,
>>> the limit for hardware domains may be increased to the number of
>>> possible IRQs.
>> I nevertheless disagree to moving the bound up to the Xen internal
>> limit unconditionally: What use does it have to allow hwdom to use
>> thousands of MSIs?
> 
> Because systems that big exist.  We have one.  In particular, it needs
> somewhere between 288 and 512 pirqs to scan the bus and bring up the
> physical functions alone.

This are hundreds, not thousands. I also heavily doubt that a system
needs any IRQs at all to scan the bus.

>> If a system got that many, the main purpose of
>> running Xen on it I would expect to be to hand various of the
>> respective devices to guests. Hence no need for hwdom to have
>> that many by default, even if this doesn't result in any extra
>> resource consumption.
>>
>> That said, I can see the current default of 256 being too low though.
>> Quite likely in the absence of a user specified value the default
>> ought to be derived from nr_irqs - nr_static_irqs rather than being
>> any fixed number. Considering the default used for nr_irqs, I'd think
>> along the lines of sqrt(num_present_cpus()) * NR_DYNAMIC_VECTORS
>> or dom0->max_vcpus * NR_DYNAMIC_VECTORS (or the minimum of
>> the two) for x86.
> 
> The hardware domain is trusted ultimately.  It can, amongst other
> things, rewrite the bootloader command line and replace xen.gz.  It can
> be trusted not to maliciously waste Xen resource.
> 
> Having an arbitrary restriction on the the hardware domains means only
> that, in the case the arbitrary limit is hit, system devices fail to
> function properly.  This is far more noticeable if the limit is hit
> during probe.  The admin can edit the bootloader and increase the limit,
> but only if the root disk was a driver lucky enough to get its
> interrupt, or the default network card got its interrupts.

There's no need to have disk access in order to add a boot option
- any reasonable boot loader ought to allow editing the command
lines.

> The limit serves no security or resource purpose, but has the chance of
> crippling the boot of the system, and making recovery hard or
> impossible.  On this justification alone, the limit should be removed.

But David's patch doesn't remove the limit, it just moves it as high as
is currently deemed reasonable. That may change, even if we can't
foresee it right now. I'm fine with proposing an alternative patch as
requested by David, but I'm not going to ack this one. If another
maintainer wants to commit it nevertheless, my disagreement here
isn't meant to be a veto...

Jan