[v1,1/4] examples/power: change 64-bit masks to arrays
Checks
Commit Message
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@intel.com>
---
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(-)
Comments
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@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>
> + 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?
> 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).
> }
> @@ -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.
> 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.
> }
> 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?
> }
> }
> }
> @@ -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.
> + 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.
> + 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 =
>
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@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.
@@ -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) {
+ 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);
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);
}
@@ -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]) {
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++;
+ }
}
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;
}
}
}
@@ -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) {
+ 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) {
+ 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));
@@ -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.
@@ -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 =