On 07/09/18 11:15, Jan Beulich wrote: >>>> On 06.09.18 at 21:25, <andrew.coop...@citrix.com> 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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel