Re: [PATCH v2 3/4] examples/power: allow vms to use lcores over 63

["Hunt, David" <david.hunt@intel.com>]

Hi Anatoly,

On 14/12/2018 12:08 PM, Burakov, Anatoly wrote:
> On 14-Dec-18 11:49 AM, David Hunt wrote:
>> Extending the functionality to allow vms to power manage cores beyond 
>> 63.
>>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
>> ---
>>   examples/vm_power_manager/channel_manager.c | 74 +++++++++------------
>>   examples/vm_power_manager/channel_manager.h | 30 ++-------
>>   examples/vm_power_manager/channel_monitor.c | 56 ++++++----------
>>   examples/vm_power_manager/vm_power_cli.c    |  4 +-
>>   4 files changed, 57 insertions(+), 107 deletions(-)
>>
>> diff --git a/examples/vm_power_manager/channel_manager.c 
>> b/examples/vm_power_manager/channel_manager.c
>> index 71f4a0ccf..8756b53b8 100644
>> --- a/examples/vm_power_manager/channel_manager.c
>> +++ b/examples/vm_power_manager/channel_manager.c
>> @@ -49,7 +49,7 @@ static bool global_hypervisor_available;
>>    */
>>   struct virtual_machine_info {
>>       char name[CHANNEL_MGR_MAX_NAME_LEN];
>> -    rte_atomic64_t pcpu_mask[CHANNEL_CMDS_MAX_CPUS];
>> +    uint16_t pcpu_map[CHANNEL_CMDS_MAX_CPUS];
>>       struct channel_info *channels[CHANNEL_CMDS_MAX_VM_CHANNELS];
>>       char channel_mask[POWER_MGR_MAX_CPUS];
>>       uint8_t num_channels;
>> @@ -79,7 +79,6 @@ update_pcpus_mask(struct virtual_machine_info 
>> *vm_info)
>>       virVcpuInfoPtr cpuinfo;
>>       unsigned i, j;
>>       int n_vcpus;
>> -    uint64_t mask;
>>         memset(global_cpumaps, 0, CHANNEL_CMDS_MAX_CPUS*global_maplen);
>>   @@ -120,26 +119,23 @@ update_pcpus_mask(struct virtual_machine_info 
>> *vm_info)
>>           vm_info->info.nrVirtCpu = n_vcpus;
>>       }
>>       for (i = 0; i < vm_info->info.nrVirtCpu; i++) {
>> -        mask = 0;
>>           for (j = 0; j < global_n_host_cpus; j++) {
>> -            if (VIR_CPU_USABLE(global_cpumaps, global_maplen, i, j) 
>> > 0) {
>> -                mask |= 1ULL << j;
>> -            }
>> +            if (VIR_CPU_USABLE(global_cpumaps,
>> +                    global_maplen, i, j) <= 0)
>> +                continue;
>> + rte_spinlock_lock(&(vm_info->config_spinlock));
>> +            vm_info->pcpu_map[i] = j;
>> + rte_spinlock_unlock(&(vm_info->config_spinlock));
>
> This is not an equivalent replacement, because something can happen 
> inbetween the unlock and subsequent lock. There's no need to 
> lock-unlock it on every iteration anyway - just lock it before the for 
> (i = 0...) and unlock it right before return.
>

Will fix in next revision.


>>           }
>> -        rte_atomic64_set(&vm_info->pcpu_mask[i], mask);
>>       }
>>       return 0;
>>   }
>>     int
>> -set_pcpus_mask(char *vm_name, unsigned int vcpu, char *core_mask)
>> +set_pcpu(char *vm_name, unsigned int vcpu, unsigned int pcpu)
>>   {
>
> <snip>
>
>> --- a/examples/vm_power_manager/channel_manager.h
>> +++ b/examples/vm_power_manager/channel_manager.h
>> @@ -40,7 +40,6 @@ struct sockaddr_un _sockaddr_un;
>>   #define MAX_CLIENTS 64
>>   #define MAX_VCPUS 20
>>   -
>>   struct libvirt_vm_info {
>
> Unintended whitespace change?
>

Yes, will address.


>>       const char *vm_name;
>>       unsigned int pcpus[MAX_VCPUS];
>> @@ -82,7 +81,7 @@ struct channel_info {
>>   struct vm_info {
>>       char name[CHANNEL_MGR_MAX_NAME_LEN];          /**< VM name */
>>       enum vm_status status;                        /**< libvirt 
>> status */
>

Thanks,
Dave.