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

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

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 =
> 


-- 
Thanks,
Anatoly