From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 0E67F1B4FA for ; Thu, 13 Dec 2018 13:03:34 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Dec 2018 04:03:34 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,348,1539673200"; d="scan'208";a="283276282" Received: from dhunt5-mobl2.ger.corp.intel.com (HELO [10.237.221.66]) ([10.237.221.66]) by orsmga005.jf.intel.com with ESMTP; 13 Dec 2018 04:03:32 -0800 From: "Hunt, David" To: "Burakov, Anatoly" , dev@dpdk.org Cc: lei.a.yao@intel.com References: <20181122170220.55482-1-david.hunt@intel.com> <20181122170220.55482-2-david.hunt@intel.com> <470299e9-409a-fa76-e63e-ad60ed7a1c95@intel.com> Message-ID: <56a7b099-8050-6afe-2e97-af6285b05541@intel.com> Date: Thu, 13 Dec 2018 12:03:31 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.3.3 MIME-Version: 1.0 In-Reply-To: <470299e9-409a-fa76-e63e-ad60ed7a1c95@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US 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: Thu, 13 Dec 2018 12:03:35 -0000 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 > > 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; > > 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.