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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew

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