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

["Jürgen Groß" <jgross@suse.com>]

On 06.04.20 12:37, Julien Grall wrote:
> 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?

PV and HVM with PV enhancements.

> 
>>
>>>
>>>> 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.

Okay, what about moving the default setting of b_info->event_channels
into libxl__arch_domain_build_info_setdefault() then?

> 
>>
>>>
>>> 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@suse.com>
>>>> ---
>>>> 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.

Okay.


Juergen