Re: [PATCH v2] tools/libxl: make default of max event channels dependant on vcpus

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 06/04/2020 11:17, Jürgen Groß wrote:
> On 06.04.20 11:24, Julien Grall wrote:
> > Hi Jurgen,
> >
> > On 06/04/2020 09:27, Juergen Gross wrote:
> > > Since Xen 4.4 the maximum number of event channels for a guest is
> > > defaulting to 1023. For large guests with lots of vcpus this is not
> > > enough, as e.g. the Linux kernel uses 7 event channels per vcpu,
> > > limiting the guest to about 140 vcpus.
> > Large guests on which arch? Which type of guests?
> I'm pretty sure this applies to x86 only. I'm not aware of event
> channels being used on ARM for IPIs.
How about the guest types?

> > > Instead of requiring to specify the allowed number of event channels
> > > via the "event_channels" domain config option, make the default
> > > depend on the maximum number of vcpus of the guest. This will require
> > > to use the "event_channels" domain config option in fewer cases as
> > > before.
> > >
> > > As different guests will have differing needs the calculation of the
> > > maximum number of event channels can be a rough estimate only,
> > > currently based on the Linux kernel requirements.
> > I am not overly happy to extend the default numbers of event channels 
> > for everyone based on Linux behavior on a given setup. Yes you have 
> > more guests that would be able to run, but at the expense of allowing 
> > a guest to use more xen memory.
> The resulting number would be larger than today only for guests with
> more than 96 vcpus. So I don't think the additional amount of memory
> is really that problematic.
This is not a very forward looking argument. For Arm, we limit to 128 
vCPUs at the moment but it would be possible to support many more (I 
think our vGIC implementation support up to 4096 vCPUs).
So even if your change impacts a small subset, each architectures should 
be able to make the decision on the limit and not imposed by x86 Linux PV.
> > For instance, I don't think this limit increase is necessary on Arm.
> >
> > > Nevertheless it is
> > > much better than the static upper limit of today as more guests will
> > > boot just fine with the new approach.
> > >
> > > In order not to regress current configs use 1023 as the minimum
> > > default setting.
> > >
> > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > ---
> > > V2:
> > > - use max() instead of min()
> > > - clarify commit message a little bit
> > > ---
> > >   tools/libxl/libxl_create.c | 2 +-
> > The documentation should be updated.
> Oh, indeed.
>
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > > index e7cb2dbc2b..c025b21894 100644
> > > --- a/tools/libxl/libxl_create.c
> > > +++ b/tools/libxl/libxl_create.c
> > > @@ -226,7 +226,7 @@ int libxl__domain_build_info_setdefault(libxl__gc 
> > > *gc,
> > >               b_info->iomem[i].gfn = b_info->iomem[i].start;
> > >       if (!b_info->event_channels)
> > > -        b_info->event_channels = 1023;
> > > +        b_info->event_channels = max(1023, b_info->max_vcpus * 8 + 
> > > 255);
> > What is the 255 for?
> Just some headroom for e.g. pv devices.
That should really be explained in the commit message and a comment.

Cheers,

--
Julien Grall