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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/08/18 14:11, Jan Beulich wrote:
>>>> On 13.08.18 at 12:01, <andrew.cooper3@xxxxxxxxxx> wrote:
>> @@ -423,6 +436,11 @@ struct domain *domain_create(domid_t domid,
>>  
>>      sched_destroy_domain(d);
>>  
>> +    if ( d->max_vcpus )
>> +    {
>> +        d->max_vcpus = 0;
>> +        XFREE(d->vcpu);
>> +    }
>>      if ( init_status & INIT_arch )
>>          arch_domain_destroy(d);
> I'm not sure it is a good idea to free the vcpus this early, in particular
> before arch_domain_destroy().

Actually, this positioning is deliberate, so as not to change the
current behaviour of arch_domain_destroy().

Before this patch, d-vcpu[] was guaranteed to be NULL in the
arch_domain_destroy() call, and I don't currently trust it to work
properly if changed.  All of this cleanup logic needs further improvements.

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

~Andrew

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