Re: [dpdk-dev] [PATCH v3 4/8] examples/power: add host channel to power manager

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

On 14-Sep-18 2:54 PM, David Hunt wrote:
> This patch adds a fifo channel to the vm_power_manager app through which
> we can send commands and polices. Intended for sending JSON strings.
> The fifo is at /tmp/powermonitor/fifo.0
> 
> Signed-off-by: David Hunt <david.hunt@intel.com>
> ---

A bunch of nitpick comments below :)

>   examples/vm_power_manager/channel_manager.c | 108 +++++++++++++++
>   examples/vm_power_manager/channel_manager.h |  17 ++-
>   examples/vm_power_manager/channel_monitor.c | 146 +++++++++++++++-----
>   examples/vm_power_manager/main.c            |   2 +
>   4 files changed, 238 insertions(+), 35 deletions(-)
> 

<snip>

> +				"Error(%s) setting non-blocking "
> +				"socket for '%s'\n",
> +				strerror(errno), info->channel_path);
> +		return -1;
> +	}
> +	return 0;
> +}
> +

As far as i can tell, vm power manager is a proper DPDK application, 
meaning there can technically be several of these running independently 
under different prefixes. Hardcoded paths are OK, but you probably need 
to place a write-lock on a file to prevent another VM power manager from 
(accidentally) taking over the FIFO? Init would probably fail earlier, 
but you never know :)

>   static int
>   setup_channel_info(struct virtual_machine_info **vm_info_dptr,
>   		struct channel_info **chan_info_dptr, unsigned channel_num)
> @@ -294,6 +327,7 @@ setup_channel_info(struct virtual_machine_info **vm_info_dptr,
>   	chan_info->channel_num = channel_num;
>   	chan_info->priv_info = (void *)vm_info;
>   	chan_info->status = CHANNEL_MGR_CHANNEL_DISCONNECTED;
> +	chan_info->type = CHANNEL_TYPE_BINARY;
>   	if (open_non_blocking_channel(chan_info) < 0) {
>   		RTE_LOG(ERR, CHANNEL_MANAGER, "Could not open channel: "
>   				"'%s' for VM '%s'\n",
> @@ -316,6 +350,35 @@ setup_channel_info(struct virtual_machine_info **vm_info_dptr,
>   	return 0;
>   }
>   
> +static int
> +setup_host_channel_info(struct channel_info **chan_info_dptr,
> +		unsigned int channel_num)
> +{
> +	struct channel_info *chan_info = *chan_info_dptr;
> +
> +	chan_info->channel_num = channel_num;
> +	chan_info->priv_info = (void *)0;

NULL?

> +	chan_info->status = CHANNEL_MGR_CHANNEL_DISCONNECTED;
> +	chan_info->type = CHANNEL_TYPE_JSON;
> +	sprintf(chan_info->channel_path, "%sfifo.0", CHANNEL_MGR_SOCKET_PATH);

Here, 0 is part of the format string...

> +
> +	if (open_host_channel(chan_info) < 0) {
> +		RTE_LOG(ERR, CHANNEL_MANAGER, "Could not open host channel: "
> +				"'%s'\n",
> +				chan_info->channel_path);
> +		return -1;
> +	}

<snip>

> +int
> +add_host_channel(void)
> +{
> +	struct channel_info *chan_info;
> +	char socket_path[PATH_MAX];
> +	int num_channels_enabled = 0;
> +	int ret;
> +
> +	snprintf(socket_path, sizeof(socket_path), "%sfifo.%u",
> +			CHANNEL_MGR_SOCKET_PATH, 0);

...while here, it's an argument. What's the significance of 0 in this 
context? Also, maybe better to put it in a function, so as to only have 
one place to fix if anything changes, instead of two?

> +
> +	errno = 0;
> +	ret = mkfifo(socket_path, 0666);

0666 seems like overly permissive to me?

> +	if ((errno != EEXIST) && (ret < 0)) {
> +		printf(" %d %d, %d\n", ret, EEXIST, errno);

This looks like a leftover debug printf?

Also, maybe if (ret < 0 && errno != EEXIST)? I don't think there's a 
need to set errno beforehand here.

> +		RTE_LOG(ERR, CHANNEL_MANAGER, "Cannot create fifo '%s' error: "
> +				"%s\n", socket_path, strerror(errno));
> +		return 0;
> +	}
> +
> +	errno = 0;

...and here too - if access() call failed, does it not always set errno 
value?

> +	if (access(socket_path, F_OK) < 0) {
> +		RTE_LOG(ERR, CHANNEL_MANAGER, "Channel path '%s' error: "
> +				"%s\n", socket_path, strerror(errno));
> +		return 0;
> +	}
> +	chan_info = rte_malloc(NULL, sizeof(*chan_info),
> +			RTE_CACHE_LINE_SIZE);

0 alignment is equivalent to RTE_CACHE_LINE_SIZE, so no need to specify 
it explicitly.

> +	if (chan_info == NULL) {
> +		RTE_LOG(ERR, CHANNEL_MANAGER, "Error allocating memory for "
> +				"channel '%s'\n", socket_path);
> +		return 0;
> +	}
> +	snprintf(chan_info->channel_path,
> +			sizeof(chan_info->channel_path), "%sfifo.%u",
> +			CHANNEL_MGR_SOCKET_PATH, 0);

Creating FIFO path again. Definitely needs a function :)

> +	if (setup_host_channel_info(&chan_info, 0) < 0) {
> +		rte_free(chan_info);
> +		return 0;
> +	}
> +	num_channels_enabled++;
> +
> +	return num_channels_enabled;
> +}
> +
>   int
>   remove_channel(struct channel_info **chan_info_dptr)
>   {
> diff --git a/examples/vm_power_manager/channel_manager.h b/examples/vm_power_manager/channel_manager.h
> index 872ec6140..c157cc22b 100644
> --- a/examples/vm_power_manager/channel_manager.h
> +++ b/examples/vm_power_manager/channel_manager.h
> @@ -37,7 +37,7 @@ struct sockaddr_un _sockaddr_un;
>   #define UNIX_PATH_MAX sizeof(_sockaddr_un.sun_path)
>   #endif
>   
> -#define MAX_VMS 4
> +#define MAX_VMS 64

This change probably needs to be called out in commit message and 
explained. Or broken into a separate commit? Also, i think technically 
"MAX_VMS" is a bad name now that you're supporting containers as well as 
VM's. MAX_CLIENTS maybe?

>   #define MAX_VCPUS 20
>   
>   
> @@ -54,6 +54,11 @@ enum channel_status { CHANNEL_MGR_CHANNEL_DISCONNECTED = 0,
>   	CHANNEL_MGR_CHANNEL_DISABLED,
>   	CHANNEL_MGR_CHANNEL_PROCESSING};
>   
> +/* Communication Channel Type */
> +enum channel_type { CHANNEL_TYPE_BINARY = 0,

Should probably start values on a new line?

> +	CHANNEL_TYPE_INI,
> +	CHANNEL_TYPE_JSON};
> +
>   /* VM libvirt(qemu/KVM) connection status */
>   enum vm_status { CHANNEL_MGR_VM_INACTIVE = 0, CHANNEL_MGR_VM_ACTIVE};
>   
> @@ -66,6 +71,7 @@ struct channel_info {
>   	volatile uint32_t status;    /**< Connection status(enum channel_status) */

<snip>

> -					pol->core_share[count].pcpu = pcpu;
> -					printf("Monitoring pcpu %d\n", pcpu);
> -				}
> +	RTE_LOG(INFO, CHANNEL_MONITOR,
> +			"Looking for pcpu for %s\n", pol->pkt.vm_name);
> +
> +	/*
> +	 * So now that we're handling virtual and physical cores, we need to
> +	 * differenciate between them when adding them to the branch monitor.
> +	 * Virtual cores need to be converted to physical cores.
> +	 */
> +
> +
> +
> +

Needs moar newlines :)

> +	if (pol->pkt.core_type == CORE_TYPE_VIRTUAL) {
> +		/*
> +		 * If the cores in the policy are virtual, we need to map them
> +		 * to physical core. We look up the vm info and use that for
> +		 * the mapping.
> +		 */
> +		get_info_vm(pol->pkt.vm_name, &info);

<snip>

> @@ -362,10 +425,12 @@ process_request(struct channel_packet *pkt, struct channel_info *chan_info)
>   	if (pkt->command == CPU_POWER) {
>   		core_mask = get_pcpus_mask(chan_info, pkt->resource_id);
>   		if (core_mask == 0) {
> -			RTE_LOG(ERR, CHANNEL_MONITOR, "Error get physical CPU mask for "
> -				"channel '%s' using vCPU(%u)\n", chan_info->channel_path,
> -				(unsigned)pkt->unit);
> -			return -1;
> +			/*
> +			 * Core mask will be 0 in the case where
> +			 * hypervisor is not available so we're working in
> +			 * the host, so use the core as the mask.
> +			 */
> +			core_mask = 1 << pkt->resource_id;

1ULL?


-- 
Thanks,
Anatoly