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.h...@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.cindex 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 intchannel_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.hindex 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.cindex 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); elsecmdline_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.
