Re: [Xen-devel] [PATCH v2 12/12] xen/domain: Allocate d->vcpu[] in domain_create()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.08.18 at 14:29, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 29/08/18 13:10, Jan Beulich wrote:
>>>>> On 29.08.18 at 12:36, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 15/08/18 16:18, Jan Beulich wrote:
>>>>>>> --- a/xen/common/domctl.c
>>>>>>> +++ b/xen/common/domctl.c
>>>>>>> @@ -554,16 +554,9 @@ long 
>>>>>>> do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
>>> u_domctl)
>>>>>>>  
>>>>>>>          ret = -EINVAL;
>>>>>>>          if ( (d == current->domain) || /* no domain_pause() */
>>>>>>> -             (max > domain_max_vcpus(d)) )
>>>>>>> +             (max != d->max_vcpus) )   /* max_vcpus set up in 
>>>>>>> createdomain 
>>> */
>>>>>>>              break;
>>>>>>>  
>>>>>>> -        /* Until Xenoprof can dynamically grow its vcpu-s array... */
>>>>>>> -        if ( d->xenoprof )
>>>>>>> -        {
>>>>>>> -            ret = -EAGAIN;
>>>>>>> -            break;
>>>>>>> -        }
>>>>>>> -
>>>>>>>          /* Needed, for example, to ensure writable p.t. state is 
>>>>>>> synced. */
>>>>>>>          domain_pause(d);
>>>>>>>  
>>>>>>> @@ -581,38 +574,8 @@ long 
>>>>>>> do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
>>> u_domctl)
>>>>>>>              }
>>>>>>>          }
>>>>>>>  
>>>>>>> -        /* We cannot reduce maximum VCPUs. */
>>>>>>> -        ret = -EINVAL;
>>>>>>> -        if ( (max < d->max_vcpus) && (d->vcpu[max] != NULL) 
>>>>>>> )xc_domain_max_vcpus
>>>>>>> -            goto maxvcpu_out;
>>>>>>> -
>>>>>>> -        /*
>>>>>>> -         * For now don't allow increasing the vcpu count from a 
>>>>>>> non-zero
>>>>>>> -         * value: This code and all readers of d->vcpu would otherwise 
>>>>>>> need
>>>>>>> -         * to be converted to use RCU, but at present there's no tools 
>>>>>>> side
>>>>>>> -         * code path that would issue such a request.
>>>>>>> -         */
>>>>>>> -        ret = -EBUSY;
>>>>>>> -        if ( (d->max_vcpus > 0) && (max > d->max_vcpus) )
>>>>>>> -            goto maxvcpu_out;
>>>>>>> -
>>>>>>>          ret = -ENOMEM;
>>>>>>>          online = cpupool_domain_cpumask(d);
>>>>>>> -        if ( max > d->max_vcpus )
>>>>>>> -        {
>>>>>>> -            struct vcpu **vcpus;
>>>>>>> -
>>>>>>> -            BUG_ON(d->vcpu != NULL);
>>>>>>> -            BUG_ON(d->max_vcpus != 0);
>>>>>>> -
>>>>>>> -            if ( (vcpus = xzalloc_array(struct vcpu *, max)) == NULL )
>>>>>>> -                goto maxvcpu_out;
>>>>>>> -
>>>>>>> -            /* Install vcpu array /then/ update max_vcpus. */
>>>>>>> -            d->vcpu = vcpus;
>>>>>>> -            smp_wmb();
>>>>>>> -            d->max_vcpus = max;
>>>>>>> -        }
>>>>>>>  
>>>>>>>          for ( i = 0; i < max; i++ )
>>>>>>>          {
>>>>>> With all of this dropped, I think the domctl should be renamed. By
>>>>>> dropping its "max" input at the same time, there would then also
>>>>>> no longer be a need to check that the value matches what was
>>>>>> stored during domain creation.
>>>>> I'm still looking to eventually delete the hypercall, but we need to be
>>>>> able to clean up all domain/vcpu allocations without calling
>>>>> complete_domain_destroy, or rearrange the entry logic so
>>>>> complete_domain_destroy() can be reused for a domain which isn't
>>>>> currently in the domlist.
>>>>>
>>>>> Unfortunately, I think this is going to be fairly complicated, I think.
>>>> Especially when we expect this to take some time, I think it would
>>>> be quite helpful for the domctl to actually say what it does until
>>>> then, rather than retaining its current (then misleading) name.
>>> Renaming the domctl means renaming xc_domain_max_vcpus(), and the
>>> python/ocaml stubs, the latter of which does have external users.
>> This is an option, but the libxc and higher layer functions could as well
>> be left alone, perhaps with a comment added to the function you name
>> explaining why its name doesn't match the domctl it uses.
> 
> And what good will that do?  You'll now have inconsistent naming, which
> is worse.
> 
> Its either all or nothing, and there are several good reasons to not
> change everything.  I definitely don't think renaming the infrastructure
> is a constructive use of my time, or anyone elses for that matter.
> 
> I'm open to the idea of leaving a comment by the implementation of
> XEN_DOMCTL_max_vcpus: explaining its change in behaviour, but I think
> that the extent of what is reasonable to do here.

Well, I'll leave it to the other REST maintainers then.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel