From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 624E01B0F7 for ; Mon, 10 Dec 2018 13:18:44 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Dec 2018 04:18:43 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,338,1539673200"; d="scan'208";a="99504729" Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.93]) ([10.237.220.93]) by orsmga006.jf.intel.com with ESMTP; 10 Dec 2018 04:18:42 -0800 To: David Hunt , dev@dpdk.org Cc: lei.a.yao@intel.com References: <20181122170220.55482-1-david.hunt@intel.com> <20181122170220.55482-2-david.hunt@intel.com> From: "Burakov, Anatoly" Message-ID: <470299e9-409a-fa76-e63e-ad60ed7a1c95@intel.com> Date: Mon, 10 Dec 2018 12:18:40 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.3.2 MIME-Version: 1.0 In-Reply-To: <20181122170220.55482-2-david.hunt@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v1 1/4] examples/power: change 64-bit masks to arrays X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Dec 2018 12:18:45 -0000 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 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; > + 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 = > -- Thanks, Anatoly