Re: [PATCH 2/2] tools/ocaml: Fix stubs the introduction of domain_create.cpupool_id

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

+ Adding toolstack maintainer

> On 18 May 2022, at 12:34, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> 
> On 18/05/2022 11:30, Luca Fancellu wrote:
>>> On 18 May 2022, at 11:12, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:
>>> 
>>> On 18/05/2022 10:51, Edwin Torok wrote:
>>>>> diff --git a/tools/ocaml/libs/xc/xenctrl.ml 
>>>>> b/tools/ocaml/libs/xc/xenctrl.ml
>>>>> index 7503031d8f61..8eab6f60eb14 100644
>>>>> --- a/tools/ocaml/libs/xc/xenctrl.ml
>>>>> +++ b/tools/ocaml/libs/xc/xenctrl.ml
>>>>> @@ -85,6 +85,7 @@ type domctl_create_config =
>>>>>   max_grant_frames: int;
>>>>>   max_maptrack_frames: int;
>>>>>   max_grant_version: int;
>>>>> + cpupool_id: int32;
>>>> What are the valid values for a CPU pool id, in particular what value 
>>>> should be passed here to get back the behaviour prior to these changes in 
>>>> Xen?
>>>> (i.e. would it be cpu pool id 0 or -1 if cpu pools aren't otherwise 
>>>> explicitly configured on the system)
>>> cpupools are a non-optional construct in Xen.
>>> 
>>> By default, one cpupool exists, with the id 0, using the default
>>> scheduler covering all pCPUs, and dom0 is constructed in this cpupool.
>>> 
>>> Passing 0 here is the backwards compatible option.
>>> 
>>> And on that note, Luca, you ought to patch xl/libxl to apply the pool=
>>> setting directly during domain create, rather than depending on cpupool
>>> 0 existing and moving the domain later.
>> Is it an enhancement or a bug fix?
> 
> This isn't a binary option.
> 
> Your series added an optimisation to DOMCTL_createdomain, then didn't
> adjust libxl to use the optimisation (which would have reduced the
> number of hypercalls to create the domain, and reduce the number of
> dynamic memory allocations in the hypervisor.  Marginal, certainly, but
> clearly a nice-to-have).
> 
> Therefore, you created technical debt, which is option 3.
> 
> By default, as the contributor, you are expected to address the
> technical debt, because it is an important difference between hacking a
> feature up for yourself, and integrating the feature nicely for everyone.
> 
> You can of course negotiate with the tools maintainer to see if they
> care, and right now that's a bit difficult.  It's quite possible that
> noone other than me cares, and I'm not libxl maintainer.
> 
> Either you need to pay off the technical debt, or someone else will have
> to.  Someone else is going to have to start with digging into source
> history, which means it's more expensive than you doing it now.
> 
> At an absolute minimum, you need to be aware of where/when you are
> creating technical debt.

Ok, we've just created a task to handle this work so that we can track it, we 
will
handle it in the future.

Cheers,
Luca

> 
>> From what I know, please correct me if I’m wrong, cpupool0
>> Is always present, so there won’t be problem on assuming its existence
> 
> From what I can see, your series has reduced the magic involved with
> cpupool0, which is good.
> 
> But the fact that it still has magic properties is still technical debt
> that someone is going to have to pay off eventually.
> 
> ~Andrew