[dpdk-dev] [PATCH v1 1/4] examples/power: change 64-bit masks to arrays

Hunt, David david.hunt at intel.com
Thu Dec 13 13:03:31 CET 2018


Hi Anatoly,

On 10/12/2018 12:18 PM, Burakov, Anatoly wrote:
> On 22-Nov-18 5:02 PM, David Hunt wrote:
>> vm_power_manager currently makes use of uint64_t masks to keep track of
>> cores in use, limiting use of the app to only being able to manage the
>> first 64 cores in a multi-core system. Many modern systems have core
>> counts greater than 64, so this limitation needs to be removed.
>>
>> This patch converts the relevant 64-bit masks to character arrays.
>>
>> Signed-off-by: David Hunt <david.hunt at intel.com>
>
> Hi Dave,
>
> Overall looks OK, but some comments below.
>
>> ---
>>   examples/vm_power_manager/channel_manager.c | 111 +++++++++++---------
>>   examples/vm_power_manager/channel_manager.h |   2 +-
>>   examples/vm_power_manager/vm_power_cli.c    |   6 +-
>>   3 files changed, 68 insertions(+), 51 deletions(-)
>>
>> diff --git a/examples/vm_power_manager/channel_manager.c 
>> b/examples/vm_power_manager/channel_manager.c
>> index 4fac099df..5af4996db 100644
>> --- a/examples/vm_power_manager/channel_manager.c
>> +++ b/examples/vm_power_manager/channel_manager.c
>> @@ -29,14 +29,11 @@
>>   #include "channel_manager.h"
>>   #include "channel_commands.h"
>>   #include "channel_monitor.h"
>> +#include "power_manager.h"
>>       #define RTE_LOGTYPE_CHANNEL_MANAGER RTE_LOGTYPE_USER1
>>   -#define ITERATIVE_BITMASK_CHECK_64(mask_u64b, i) \
>> -        for (i = 0; mask_u64b; mask_u64b &= ~(1ULL << i++)) \
>> -        if ((mask_u64b >> i) & 1) \
>> -
>>   /* Global pointer to libvirt connection */
>>   static virConnectPtr global_vir_conn_ptr;
>>   @@ -54,7 +51,7 @@ struct virtual_machine_info {
>>       char name[CHANNEL_MGR_MAX_NAME_LEN];
>>       rte_atomic64_t pcpu_mask[CHANNEL_CMDS_MAX_CPUS];
>>       struct channel_info *channels[CHANNEL_CMDS_MAX_VM_CHANNELS];
>> -    uint64_t channel_mask;
>> +    char channel_mask[POWER_MGR_MAX_CPUS];
>>       uint8_t num_channels;
>>       enum vm_status status;
>>       virDomainPtr domainPtr;
>> @@ -135,12 +132,14 @@ update_pcpus_mask(struct virtual_machine_info 
>> *vm_info)
>>   }
>>     int
>> -set_pcpus_mask(char *vm_name, unsigned vcpu, uint64_t core_mask)
>> +set_pcpus_mask(char *vm_name, unsigned int vcpu, char *core_mask)
>>   {
>>       unsigned i = 0;
>>       int flags = VIR_DOMAIN_AFFECT_LIVE|VIR_DOMAIN_AFFECT_CONFIG;
>>       struct virtual_machine_info *vm_info;
>> -    uint64_t mask = core_mask;
>> +    char mask[POWER_MGR_MAX_CPUS];
>> +
>> +    memcpy(mask, core_mask, POWER_MGR_MAX_CPUS);
>>         if (vcpu >= CHANNEL_CMDS_MAX_CPUS) {
>>           RTE_LOG(ERR, CHANNEL_MANAGER, "vCPU(%u) exceeds max 
>> allowable(%d)\n",
>> @@ -156,8 +155,8 @@ set_pcpus_mask(char *vm_name, unsigned vcpu, 
>> uint64_t core_mask)
>>         if (!virDomainIsActive(vm_info->domainPtr)) {
>>           RTE_LOG(ERR, CHANNEL_MANAGER, "Unable to set vCPU(%u) to 
>> pCPU "
>> -                "mask(0x%"PRIx64") for VM '%s', VM is not active\n",
>> -                vcpu, core_mask, vm_info->name);
>> +                " for VM '%s', VM is not active\n",
>> +                vcpu, vm_info->name);
>>           return -1;
>>       }
>>   @@ -167,22 +166,25 @@ set_pcpus_mask(char *vm_name, unsigned vcpu, 
>> uint64_t core_mask)
>>           return -1;
>>       }
>>       memset(global_cpumaps, 0 , CHANNEL_CMDS_MAX_CPUS * global_maplen);
>> -    ITERATIVE_BITMASK_CHECK_64(mask, i) {
>> -        VIR_USE_CPU(global_cpumaps, i);
>> -        if (i >= global_n_host_cpus) {
>> -            RTE_LOG(ERR, CHANNEL_MANAGER, "CPU(%u) exceeds the 
>> available "
>> -                    "number of CPUs(%u)\n", i, global_n_host_cpus);
>> -            return -1;
>> +    for (i = 0; i < POWER_MGR_MAX_CPUS; i++) {
>> +        if (mask[i] == 1) {
>
> Here and in other places - early exit would've avoided unneeded extra 
> indents, e.g.
>
> if (mask[i] != 1)
>     continue;
> <do stuff>
>

Sure, will do.


>> +            VIR_USE_CPU(global_cpumaps, i);
>> +            if (i >= global_n_host_cpus) {
>> +                RTE_LOG(ERR, CHANNEL_MANAGER, "CPU(%u) exceeds the 
>> available "
>> +                        "number of CPUs(%u)\n",
>> +                        i, global_n_host_cpus);
>> +                return -1;
>> +            }
>>           }
>>       }
>>       if (virDomainPinVcpuFlags(vm_info->domainPtr, vcpu, 
>> global_cpumaps,
>>               global_maplen, flags) < 0) {
>>           RTE_LOG(ERR, CHANNEL_MANAGER, "Unable to set vCPU(%u) to 
>> pCPU "
>> -                "mask(0x%"PRIx64") for VM '%s'\n", vcpu, core_mask,
>> +                " for VM '%s'\n", vcpu,
>>                   vm_info->name);
>>           return -1;
>>       }
>> -    rte_atomic64_set(&vm_info->pcpu_mask[vcpu], core_mask);
>> +    memcpy(&vm_info->pcpu_mask[vcpu], core_mask, POWER_MGR_MAX_CPUS);
>
> I'm probably missing something here, but at face value, replacing an 
> atomic set operation with a simple memcpy is not equivalent. Is there 
> any locking that's missing from here? Or was it that atomics were not 
> necessary in the first place?


Yes. The rest of the updates to the config space are protected by a 
spinlock, this case used an atomic because it was a uint64, so now that 
it's an array I've added a spinlock lock/unlock around the memcpy.


>
>>       return 0;
>>     }
>> @@ -190,7 +192,11 @@ set_pcpus_mask(char *vm_name, unsigned vcpu, 
>> uint64_t core_mask)
>>   int
>>   set_pcpu(char *vm_name, unsigned vcpu, unsigned core_num)
>>   {
>> -    uint64_t mask = 1ULL << core_num;
>> +    char mask[POWER_MGR_MAX_CPUS];
>> +
>> +    memset(mask, 0, POWER_MGR_MAX_CPUS);
>> +
>> +    mask[core_num] = 1;
>>         return set_pcpus_mask(vm_name, vcpu, mask);
>
> This was a thin wrapper to get around dealing with masks. Now that 
> you're dealing with array indexes, this function seems redundant (and 
> creates an extra mask/memset in the process).
>

Both functions are called from other files. So still needed, 
unfortunately. It may be possible to remove later in the series, if 
those calls are removed.


>>   }
>> @@ -211,7 +217,7 @@ static inline int
>>   channel_exists(struct virtual_machine_info *vm_info, unsigned 
>> channel_num)
>>   {
>>       rte_spinlock_lock(&(vm_info->config_spinlock));
>> -    if (vm_info->channel_mask & (1ULL << channel_num)) {
>> +    if (vm_info->channel_mask[channel_num]) {
>
> You seem to be using (val) and (val == 1) interchangeably. This worked 
> for masks, but will not work for char arrays. Comparisons for values 
> should be explicit in this and other similar cases.


Fixed.


>
>> rte_spinlock_unlock(&(vm_info->config_spinlock));
>>           return 1;
>>       }
>> @@ -343,7 +349,7 @@ setup_channel_info(struct virtual_machine_info 
>> **vm_info_dptr,
>>       }
>>       rte_spinlock_lock(&(vm_info->config_spinlock));
>>       vm_info->num_channels++;
>> -    vm_info->channel_mask |= 1ULL << channel_num;
>> +    vm_info->channel_mask[channel_num] = 1;
>>       vm_info->channels[channel_num] = chan_info;
>>       chan_info->status = CHANNEL_MGR_CHANNEL_CONNECTED;
>>       rte_spinlock_unlock(&(vm_info->config_spinlock));
>> @@ -590,7 +596,7 @@ remove_channel(struct channel_info **chan_info_dptr)
>>       vm_info = (struct virtual_machine_info *)chan_info->priv_info;
>>         rte_spinlock_lock(&(vm_info->config_spinlock));
>> -    vm_info->channel_mask &= ~(1ULL << chan_info->channel_num);
>> +    vm_info->channel_mask[chan_info->channel_num] = 0;
>>       vm_info->num_channels--;
>>       rte_spinlock_unlock(&(vm_info->config_spinlock));
>>   @@ -603,7 +609,7 @@ set_channel_status_all(const char *vm_name, 
>> enum channel_status status)
>>   {
>>       struct virtual_machine_info *vm_info;
>>       unsigned i;
>> -    uint64_t mask;
>> +    char mask[POWER_MGR_MAX_CPUS];
>>       int num_channels_changed = 0;
>>         if (!(status == CHANNEL_MGR_CHANNEL_CONNECTED ||
>> @@ -619,10 +625,12 @@ set_channel_status_all(const char *vm_name, 
>> enum channel_status status)
>>       }
>>         rte_spinlock_lock(&(vm_info->config_spinlock));
>> -    mask = vm_info->channel_mask;
>> -    ITERATIVE_BITMASK_CHECK_64(mask, i) {
>> -        vm_info->channels[i]->status = status;
>> -        num_channels_changed++;
>> +    memcpy(mask, (char *)vm_info->channel_mask, POWER_MGR_MAX_CPUS);
>> +    for (i = 0; i < POWER_MGR_MAX_CPUS; i++) {
>> +        if (mask[i] == 1) {
>> +            vm_info->channels[i]->status = status;
>> +            num_channels_changed++;
>> +        }
>
> Same as above re: indent - perhaps, early exit (well, early 
> continue...) would make things easier to read.


Done.


>
>>       }
>>       rte_spinlock_unlock(&(vm_info->config_spinlock));
>>       return num_channels_changed;
>> @@ -665,7 +673,7 @@ get_all_vm(int *num_vm, int *num_vcpu)
>>         virNodeInfo node_info;
>>       virDomainPtr *domptr;
>> -    uint64_t mask;
>> +    char mask[POWER_MGR_MAX_CPUS];
>>       int i, ii, numVcpus[MAX_VCPUS], cpu, n_vcpus;
>>       unsigned int jj;
>>       const char *vm_name;
>> @@ -714,15 +722,16 @@ get_all_vm(int *num_vm, int *num_vcpu)
>>             /* Save pcpu in use by libvirt VMs */
>>           for (ii = 0; ii < n_vcpus; ii++) {
>> -            mask = 0;
>> +            memset(mask, 0, POWER_MGR_MAX_CPUS);
>>               for (jj = 0; jj < global_n_host_cpus; jj++) {
>>                   if (VIR_CPU_USABLE(global_cpumaps,
>>                           global_maplen, ii, jj) > 0) {
>> -                    mask |= 1ULL << jj;
>> +                    mask[jj] = 1;
>>                   }
>>               }
>> -            ITERATIVE_BITMASK_CHECK_64(mask, cpu) {
>> -                lvm_info[i].pcpus[ii] = cpu;
>> +            for (cpu = 0; cpu < POWER_MGR_MAX_CPUS; cpu++) {
>> +                if (mask[cpu])
>> +                    lvm_info[i].pcpus[ii] = cpu;
>
> Same, should be mask[cpu] == 1.
>
> Also, are two loops necessary?
>

I'll consolidate.


>>               }
>>           }
>>       }
>> @@ -733,7 +742,7 @@ get_info_vm(const char *vm_name, struct vm_info 
>> *info)
>>   {
>>       struct virtual_machine_info *vm_info;
>>       unsigned i, channel_num = 0;
>> -    uint64_t mask;
>> +    char mask[POWER_MGR_MAX_CPUS];
>>         vm_info = find_domain_by_name(vm_name);
>>       if (vm_info == NULL) {
>> @@ -746,14 +755,19 @@ get_info_vm(const char *vm_name, struct vm_info 
>> *info)
>>         rte_spinlock_lock(&(vm_info->config_spinlock));
>>   -    mask = vm_info->channel_mask;
>> -    ITERATIVE_BITMASK_CHECK_64(mask, i) {
>> -        info->channels[channel_num].channel_num = i;
>> -        memcpy(info->channels[channel_num].channel_path,
>> -                vm_info->channels[i]->channel_path, UNIX_PATH_MAX);
>> -        info->channels[channel_num].status = 
>> vm_info->channels[i]->status;
>> -        info->channels[channel_num].fd = vm_info->channels[i]->fd;
>> -        channel_num++;
>> +    memcpy(mask, (char *)vm_info->channel_mask, POWER_MGR_MAX_CPUS);
>> +    for (i = 0; i < POWER_MGR_MAX_CPUS; i++) {
>> +        if (mask[i] == 1) {
>
> Same - early exit would make things more readable.
>

Done


>> + info->channels[channel_num].channel_num = i;
>> +            memcpy(info->channels[channel_num].channel_path,
>> +                    vm_info->channels[i]->channel_path,
>> +                    UNIX_PATH_MAX);
>> +            info->channels[channel_num].status =
>> +                    vm_info->channels[i]->status;
>> +            info->channels[channel_num].fd =
>> +                    vm_info->channels[i]->fd;
>> +            channel_num++;
>> +        }
>>       }
>>         info->num_channels = channel_num;
>> @@ -822,7 +836,7 @@ add_vm(const char *vm_name)
>>       }
>>       strncpy(new_domain->name, vm_name, sizeof(new_domain->name));
>>       new_domain->name[sizeof(new_domain->name) - 1] = '\0';
>> -    new_domain->channel_mask = 0;
>> +    memset(new_domain->channel_mask, 0, POWER_MGR_MAX_CPUS);
>>       new_domain->num_channels = 0;
>>         if (!virDomainIsActive(dom_ptr))
>> @@ -940,18 +954,21 @@ void
>>   channel_manager_exit(void)
>>   {
>>       unsigned i;
>> -    uint64_t mask;
>> +    char mask[POWER_MGR_MAX_CPUS];
>>       struct virtual_machine_info *vm_info;
>>         LIST_FOREACH(vm_info, &vm_list_head, vms_info) {
>> rte_spinlock_lock(&(vm_info->config_spinlock));
>>   -        mask = vm_info->channel_mask;
>> -        ITERATIVE_BITMASK_CHECK_64(mask, i) {
>> - remove_channel_from_monitor(vm_info->channels[i]);
>> -            close(vm_info->channels[i]->fd);
>> -            rte_free(vm_info->channels[i]);
>> +        memcpy(mask, (char *)vm_info->channel_mask, 
>> POWER_MGR_MAX_CPUS);
>> +        for (i = 0; i < POWER_MGR_MAX_CPUS; i++) {
>> +            if (mask[i] == 1) {
>
> Same - early exit.
>

Done


>> + remove_channel_from_monitor(
>> +                        vm_info->channels[i]);
>> +                close(vm_info->channels[i]->fd);
>> +                rte_free(vm_info->channels[i]);
>> +            }
>>           }
>> rte_spinlock_unlock(&(vm_info->config_spinlock));
>>   diff --git a/examples/vm_power_manager/channel_manager.h 
>> b/examples/vm_power_manager/channel_manager.h
>> index d948b304c..c2520ab6f 100644
>> --- a/examples/vm_power_manager/channel_manager.h
>> +++ b/examples/vm_power_manager/channel_manager.h
>> @@ -149,7 +149,7 @@ uint64_t get_pcpus_mask(struct channel_info 
>> *chan_info, unsigned vcpu);
>>    *  - 0 on success.
>>    *  - Negative on error.
>>    */
>> -int set_pcpus_mask(char *vm_name, unsigned vcpu, uint64_t core_mask);
>> +int set_pcpus_mask(char *vm_name, unsigned int vcpu, char *core_mask);
>>     /**
>>    * Set the Physical CPU for the specified vCPU.
>> diff --git a/examples/vm_power_manager/vm_power_cli.c 
>> b/examples/vm_power_manager/vm_power_cli.c
>> index d588d38aa..101e67c9c 100644
>> --- a/examples/vm_power_manager/vm_power_cli.c
>> +++ b/examples/vm_power_manager/vm_power_cli.c
>> @@ -128,7 +128,7 @@ struct cmd_set_pcpu_mask_result {
>>       cmdline_fixed_string_t set_pcpu_mask;
>>       cmdline_fixed_string_t vm_name;
>>       uint8_t vcpu;
>> -    uint64_t core_mask;
>> +    char core_mask[POWER_MGR_MAX_CPUS];
>>   };
>>     static void
>> @@ -139,10 +139,10 @@ cmd_set_pcpu_mask_parsed(void *parsed_result, 
>> struct cmdline *cl,
>>         if (set_pcpus_mask(res->vm_name, res->vcpu, res->core_mask) 
>> == 0)
>>           cmdline_printf(cl, "Pinned vCPU(%"PRId8") to pCPU core "
>> -                "mask(0x%"PRIx64")\n", res->vcpu, res->core_mask);
>> +                "\n", res->vcpu);
>>       else
>>           cmdline_printf(cl, "Unable to pin vCPU(%"PRId8") to pCPU 
>> core "
>> -                "mask(0x%"PRIx64")\n", res->vcpu, res->core_mask);
>> +                "\n", res->vcpu);
>>   }
>>     cmdline_parse_token_string_t cmd_set_pcpu_mask =
>>
>
>

Updated patch coming soon.

Thanks,
Dave.



More information about the dev mailing list