[libvirt] [PATCH 04/34] qemu: don't iterate vcpus using priv->nvcpupids in qemuProcessSetSchedParams

[John Ferlan]

On 01/15/2016 01:50 PM, John Ferlan wrote:
> 
> 
> On 01/14/2016 11:26 AM, Peter Krempa wrote:
>> This should be the last offender.
>> ---
>>  src/qemu/qemu_process.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 845d5e1..dec4572 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -2350,10 +2350,14 @@ qemuProcessSetSchedParams(int id,
>>  static int
>>  qemuProcessSetSchedulers(virDomainObjPtr vm)
>>  {
>> -    qemuDomainObjPrivatePtr priv = vm->privateData;
>>      size_t i = 0;
>>
>> -    for (i = 0; i < priv->nvcpupids; i++) {
>> +    for (i = 0; i < virDomainDefGetVcpusMax(vm->def); i++) {
>> +        virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(vm->def, i);
>> +
>> +        if (!vcpu->online)
>> +            continue;
>> +
>>          if (qemuProcessSetSchedParams(i, qemuDomainGetVcpuPid(vm, i),
>>                                        vm->def->cputune.nvcpusched,
>>                                        vm->def->cputune.vcpusched) < 0)
>>
> 
> Is the mapping of 'i' the same for the qemuProcessSetSchedParams?  That
> is, 'i' could be incremented for an 'offline' vcpu; whereas, prior to
> this patch it wouldn't be.
> 
> Although qemuDomainGetVcpuPid and qemuProcessSetSchedParams do check if
> (vcpu >= priv->nvcpupids, e.g. vcpu == i), does it necessarily mean the
> 3rd element of 8 elements in the vcpupids maps the same as the 3rd
> element in def->vcpus?
> 

So after thinking about this one some more and of course seeing most of
the rest of the code - I revisited this one...

I'm perhaps thinking ahead w/r/t how adding "a" vcpu could/would work as
opposed to the current model of using setvcpus to change the current
count as long as it doesn't go over max.  There isn't a "chance" that
the def->vcpus could have vcpuids 0, 1, and 4 'online' while vcpuid 3 is
'offline'.

IOW: the def->vcpus array is would have the active/online vcpus first
followed by the offline second. This would thus match the cardinality of
the priv->vcpupids, so I think we're OK here.

Although it would still be nice to have an OnlineVcpuMap available.

In any case, ACK for the change, but it's definitely something that'll
need to be thought about eventually. At least w/r/t the number of places
where qemuDomainGetVcpuPid and virDomainDefGetVcpu assume the same order.

John