Re: [Xen-devel] [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/09/18 11:15, Jan Beulich wrote:
>>>> On 06.09.18 at 21:25, <andrew.cooper3@xxxxxxxxxx> wrote:
>> While v0 must be the first allocated vcpu for for_each_vcpu() to work, it
>> isn't a requirement for the threading the vcpu into the linked list, so 
>> update
>> the threading code to be more generic, and add a comment explaining why we
>> need to search for prev_id.
> I'm afraid I can't bring this in line with the code change:
>
>> @@ -178,15 +190,27 @@ struct vcpu *vcpu_create(
>>      if ( arch_vcpu_create(v) != 0 )
>>          goto fail_sched;
>>  
>> +    /* Insert the vcpu into the domain's vcpu list. */
>>      d->vcpu[vcpu_id] = v;
>>      if ( vcpu_id != 0 )
> There still is this conditional, and you don't add any else to it. Hence
> afaics if vCPU 0 was created after vCPU 1, vCPU 0's next_in_list
> would not be made point to vCPU 1. That's not what I'd call "more
> generic".

You are completely correct.  I don't know what I was thinking when
saying that this is more generic.

I suspect it also means that, depending on hardware CPU numbering,
for_each_vcpu(idle) may not work, but perhaps that is not an issue as
the idle domains pointer is actually private to scheduler_init().  It is
available from either current->domain, or idle_vcpu[$X]->domain, but I
think its reasonable to expect that we never use for_each_vcpu() over
the idle domain.

> But the question is what use the next_in_list field is in the first place,
> when the entries there are sorted by ID anyway: Why can't we
> simply use v->domain->vcpu[] instead? In the common case
> v->domain->vcpu[v->vcpu_id+1] is not going to be NULL anyway,
> and I don't think for_each_vcpu() would become that much more
> complicated that way.

It would, because every iteration you'd need a check of v->vcpu_id
against d->max_vcpus to avoid walking off the end of d->vcpu[]

>
>>      {
>>          int prev_id = v->vcpu_id - 1;
>> +
>> +        /*
>> +         * Look for the previously allocated vcpu, and splice into the
>> +         * next_in_list single linked list.
> I'm also not very happy about the use of "previously" here: This (to
> me as a non-native speaker) in no way means "the one with the next
> lowest ID".

Given the simplifying assumption of not the idle domain, the setting up
of the single linked list can be made much more concise, as we know the
IDs are packed and allocated in ascending order.

~Andrew

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