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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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.

>      {
>          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".

Jan

> +         * All domains other than IDLE have tightly packed vcpu_id's.  IDLE
> +         * vcpu_id's are derived from hardware CPU id's and can be sparse.
> +         */
>          while ( (prev_id >= 0) && (d->vcpu[prev_id] == NULL) )
>              prev_id--;
> -        BUG_ON(prev_id < 0);
> -        v->next_in_list = d->vcpu[prev_id]->next_in_list;
> -        d->vcpu[prev_id]->next_in_list = v;
> +
> +        if ( prev_id >= 0 )
> +        {
> +            v->next_in_list = d->vcpu[prev_id]->next_in_list;
> +            d->vcpu[prev_id]->next_in_list = v;
> +        }
>      }
>  
>      /* Must be called after making new vcpu visible to for_each_vcpu(). */
> -- 
> 2.1.4





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