[dpdk-dev] [PATCH v3 1/2] power: add fifo per core for JSON interface

[Hajkowski, MarcinX]

> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Lukasz Gosiewski
> Sent: Wednesday, April 24, 2019 10:22 AM
> To: Hunt, David <david.hunt at intel.com>
> Cc: dev at dpdk.org; Krakowiak, LukaszX <lukaszx.krakowiak at intel.com>;
> Gosiewski, LukaszX <lukaszx.gosiewski at intel.com>
> Subject: [dpdk-dev] [PATCH v3 1/2] power: add fifo per core for JSON
> interface
> 
> From: Lukasz Krakowiak <lukaszx.krakowiak at intel.com>
> 
> This patch implement a separate FIFO for each cpu core.
> For proper handling JSON interface, removed fields from cmds:
> core_list, resource_id, name.
> 
> Signed-off-by: Lukasz Krakowiak <lukaszx.krakowiak at intel.com>
> Signed-off-by: Lukasz Gosiewski <lukaszx.gosiewski at intel.com>

I belive there should be information provided that patch depends on http://patchwork.dpdk.org/patch/52186/. 

> 
> ---
> v2:
> * updated handling vm_name (use proper buff size)
> * rebase to master changes
> 
> v3:
> * improvement to coding style
> ---
>  examples/vm_power_manager/channel_manager.c | 85 +++++++++++++-----
> -  examples/vm_power_manager/channel_manager.h |  7 +-
> examples/vm_power_manager/channel_monitor.c | 91 +++++++++++++++---
> ---
>  examples/vm_power_manager/main.c            |  2 +-
>  4 files changed, 129 insertions(+), 56 deletions(-)
> 
> diff --git a/examples/vm_power_manager/channel_manager.c
> b/examples/vm_power_manager/channel_manager.c
> index 05c0eea44..d19276839 100644
> --- a/examples/vm_power_manager/channel_manager.c
> +++ b/examples/vm_power_manager/channel_manager.c
> @@ -346,10 +346,22 @@ setup_channel_info(struct virtual_machine_info
> **vm_info_dptr,
>  	return 0;
>  }
> 
> -static void
> -fifo_path(char *dst, unsigned int len)
> +static int
> +fifo_path(char *dst, unsigned int len, unsigned int id)
>  {
> -	snprintf(dst, len, "%sfifo", CHANNEL_MGR_SOCKET_PATH);
> +	int cnt;
> +
> +	cnt = snprintf(dst, len, "%s%s%d", CHANNEL_MGR_SOCKET_PATH,
> +			CHANNEL_MGR_FIFO_PATTERN_NAME, id);
> +
> +	if ((cnt < 0) || (cnt > (int)len - 1)) {
> +		RTE_LOG(ERR, CHANNEL_MANAGER, "Could not create
> proper "
> +			"string for fifo path\n");
> +
> +		return -1;
> +	}
> +
> +	return 0;
>  }
> 
>  static int
> @@ -535,40 +547,59 @@ add_channels(const char *vm_name, unsigned
> *channel_list,  }
> 
>  int
> -add_host_channel(void)
> +add_host_channels(void)
>  {
>  	struct channel_info *chan_info;
>  	char socket_path[PATH_MAX];
>  	int num_channels_enabled = 0;
>  	int ret;
> +	struct core_info *ci;
> 
> -	fifo_path(socket_path, sizeof(socket_path));
> -
> -	ret = mkfifo(socket_path, 0660);
> -	if ((errno != EEXIST) && (ret < 0)) {
> -		RTE_LOG(ERR, CHANNEL_MANAGER, "Cannot create fifo '%s'
> error: "
> -				"%s\n", socket_path, strerror(errno));
> +	ci = get_core_info();
> +	if (ci == NULL) {
> +		RTE_LOG(ERR, CHANNEL_MANAGER, "Cannot allocate
> memory for
> +core_info\n");
>  		return 0;
>  	}
> 
> -	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), 0);
> -	if (chan_info == NULL) {
> -		RTE_LOG(ERR, CHANNEL_MANAGER, "Error allocating
> memory for "
> -				"channel '%s'\n", socket_path);
> -		return 0;
> -	}
> -	rte_strlcpy(chan_info->channel_path, socket_path,
> UNIX_PATH_MAX);
> +	do {
> +		if (ci->cd[num_channels_enabled].global_enabled_cpus == 0)
> +			continue;
> 
> -	if (setup_host_channel_info(&chan_info, 0) < 0) {
> -		rte_free(chan_info);
> -		return 0;
> -	}
> -	num_channels_enabled++;
> +		ret = fifo_path(socket_path, sizeof(socket_path),
> +
> 	num_channels_enabled);
> +		if (ret < 0)
> +			return 0;
> +
> +		ret = mkfifo(socket_path, 0660);
> +		RTE_LOG(DEBUG, CHANNEL_MANAGER, "TRY CREATE fifo
> '%s'\n",
> +			socket_path);
> +		if ((errno != EEXIST) && (ret < 0)) {
> +			RTE_LOG(ERR, CHANNEL_MANAGER, "Cannot create
> fifo '%s' error: "
> +					"%s\n", socket_path,
> strerror(errno));
> +			return 0;
> +		}
> +
> +		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), 0);
> +		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),
> +			"%s", socket_path);
> +
> +		if (setup_host_channel_info(&chan_info,
> +			num_channels_enabled) < 0) {
> +			rte_free(chan_info);
> +			return 0;
> +		}
> +	} while (++num_channels_enabled <= ci->core_count);
> 
>  	return num_channels_enabled;
>  }
> diff --git a/examples/vm_power_manager/channel_manager.h
> b/examples/vm_power_manager/channel_manager.h
> index c3cdce492..7fddaf6a6 100644
> --- a/examples/vm_power_manager/channel_manager.h
> +++ b/examples/vm_power_manager/channel_manager.h
> @@ -28,6 +28,9 @@ extern "C" {
>  /* File socket directory */
>  #define CHANNEL_MGR_SOCKET_PATH     "/tmp/powermonitor/"
> 
> +/* FIFO file name template */
> +#define CHANNEL_MGR_FIFO_PATTERN_NAME   "fifo"
> +
>  #ifndef UNIX_PATH_MAX
>  struct sockaddr_un _sockaddr_un;
>  #define UNIX_PATH_MAX sizeof(_sockaddr_un.sun_path) @@ -212,13
> +215,13 @@ int add_channels(const char *vm_name, unsigned
> *channel_list,
>  		unsigned num_channels);
> 
>  /**
> - * Set up a fifo by which host applications can send command an policies
> + * Set up fifos by which host applications can send command an policies
>   * through a fifo to the vm_power_manager
>   *
>   * @return
>   *  - 0 for success
>   */
> -int add_host_channel(void);
> +int add_host_channels(void);
> 
>  /**
>   * Remove a channel definition from the channel manager. This must only be
> diff --git a/examples/vm_power_manager/channel_monitor.c
> b/examples/vm_power_manager/channel_monitor.c
> index 74df0fe20..41ee8671d 100644
> --- a/examples/vm_power_manager/channel_monitor.c
> +++ b/examples/vm_power_manager/channel_monitor.c
> @@ -29,6 +29,7 @@
>  #include <rte_cycles.h>
>  #include <rte_ethdev.h>
>  #include <rte_pmd_i40e.h>
> +#include <rte_string_fns.h>
> 
>  #include <libvirt/libvirt.h>
>  #include "channel_monitor.h"
> @@ -51,7 +52,7 @@ static volatile unsigned run_loop = 1;  static int
> global_event_fd;  static unsigned int policy_is_set;  static struct epoll_event
> *global_events_list; -static struct policy policies[MAX_CLIENTS];
> +static struct policy policies[POWER_MGR_MAX_CPUS];

POWER_MGR_MAX_CPUS was deleted. RTE_MAX_LCORE should be used instead I think.

> 
>  #ifdef USE_JANSSON
> 
> @@ -130,13 +131,45 @@ set_policy_mac(struct channel_packet *pkt, int idx,
> char *mac)
>  	return 0;
>  }
> 
> +static char*
> +get_resource_name_from_chn_path(const char *channel_path) {
> +	char *substr = NULL;
> +
> +	substr = strstr(channel_path,
> CHANNEL_MGR_FIFO_PATTERN_NAME);
> +
> +	return substr;
> +}
> 
>  static int
> -parse_json_to_pkt(json_t *element, struct channel_packet *pkt)
> +get_resource_id_from_vmname(const char *vm_name) {
> +	int result = -1;
> +	int off = 0;
> +
> +	if (vm_name == NULL)
> +		return -1;
> +
> +	while (vm_name[off] != '\0') {
> +		if (isdigit(vm_name[off]))
> +			break;
> +		off++;
> +	}
> +	result = atoi(&vm_name[off]);
> +	if ((result == 0) && (vm_name[off] != '0'))
> +		return -1;
> +
> +	return result;
> +}
> +
> +static int
> +parse_json_to_pkt(json_t *element, struct channel_packet *pkt,
> +					const char *vm_name)
>  {
>  	const char *key;
>  	json_t *value;
>  	int ret;
> +	int resource_id;
> 
>  	memset(pkt, 0, sizeof(struct channel_packet));
> 
> @@ -147,19 +180,23 @@ parse_json_to_pkt(json_t *element, struct
> channel_packet *pkt)
>  	pkt->command = PKT_POLICY;
>  	pkt->core_type = CORE_TYPE_PHYSICAL;
> 
> +	if (vm_name == NULL) {
> +		RTE_LOG(ERR, CHANNEL_MONITOR,
> +			"vm_name is NULL, request rejected !\n");
> +		return -1;
> +	}
> +
>  	json_object_foreach(element, key, value) {
>  		if (!strcmp(key, "policy")) {
>  			/* Recurse in to get the contents of profile */
> -			ret = parse_json_to_pkt(value, pkt);
> +			ret = parse_json_to_pkt(value, pkt, vm_name);
>  			if (ret)
>  				return ret;
>  		} else if (!strcmp(key, "instruction")) {
>  			/* Recurse in to get the contents of instruction */
> -			ret = parse_json_to_pkt(value, pkt);
> +			ret = parse_json_to_pkt(value, pkt, vm_name);
>  			if (ret)
>  				return ret;
> -		} else if (!strcmp(key, "name")) {
> -			strcpy(pkt->vm_name, json_string_value(value));
>  		} else if (!strcmp(key, "command")) {
>  			char command[32];
>  			strlcpy(command, json_string_value(value), 32); @@
> -222,16 +259,6 @@ parse_json_to_pkt(json_t *element, struct
> channel_packet *pkt)
>  						json_array_get(value, i));
>  				pkt->timer_policy.quiet_hours[i] = hour;
>  			}
> -		} else if (!strcmp(key, "core_list")) {
> -			unsigned int i;
> -			size_t size = json_array_size(value);
> -
> -			for (i = 0; i < size; i++) {
> -				int core = (int)json_integer_value(
> -						json_array_get(value, i));
> -				pkt->vcpu_to_control[i] = core;
> -			}
> -			pkt->num_vcpu = size;
>  		} else if (!strcmp(key, "mac_list")) {
>  			unsigned int i;
>  			size_t size = json_array_size(value); @@ -270,13
> +297,21 @@ parse_json_to_pkt(json_t *element, struct channel_packet
> *pkt)
>  					"Invalid command received in
> JSON\n");
>  				return -1;
>  			}
> -		} else if (!strcmp(key, "resource_id")) {
> -			pkt->resource_id =
> (uint32_t)json_integer_value(value);
>  		} else {
>  			RTE_LOG(ERR, CHANNEL_MONITOR,
>  				"Unknown key received in JSON string: %s\n",
>  				key);
>  		}
> +
> +		resource_id = get_resource_id_from_vmname(vm_name);
> +		if (resource_id < 0) {
> +			RTE_LOG(ERR, CHANNEL_MONITOR,
> +				"Could not get resource_id from
> vm_name:%s\n",
> +				vm_name);
> +			return -1;
> +		}
> +		rte_strlcpy(pkt->vm_name, vm_name, VM_MAX_NAME_SZ);
> +		pkt->resource_id = resource_id;
>  	}
>  	return 0;
>  }
> @@ -426,13 +461,13 @@ update_policy(struct channel_packet *pkt)  {
> 
>  	unsigned int updated = 0;
> -	int i;
> +	unsigned int i;
> 
> 
>  	RTE_LOG(INFO, CHANNEL_MONITOR,
>  			"Applying policy for %s\n", pkt->vm_name);
> 
> -	for (i = 0; i < MAX_CLIENTS; i++) {
> +	for (i = 0; i < RTE_DIM(policies); i++) {
>  		if (strcmp(policies[i].pkt.vm_name, pkt->vm_name) == 0) {
>  			/* Copy the contents of *pkt into the policy.pkt */
>  			policies[i].pkt = *pkt;
> @@ -447,7 +482,7 @@ update_policy(struct channel_packet *pkt)
>  		}
>  	}
>  	if (!updated) {
> -		for (i = 0; i < MAX_CLIENTS; i++) {
> +		for (i = 0; i < RTE_DIM(policies); i++) {
>  			if (policies[i].enabled == 0) {
>  				policies[i].pkt = *pkt;
>  				get_pcpu_to_control(&policies[i]);
> @@ -465,13 +500,13 @@ update_policy(struct channel_packet *pkt)  static
> int  remove_policy(struct channel_packet *pkt __rte_unused)  {
> -	int i;
> +	unsigned int i;
> 
>  	/*
>  	 * Disabling the policy is simply a case of setting
>  	 * enabled to 0
>  	 */
> -	for (i = 0; i < MAX_CLIENTS; i++) {
> +	for (i = 0; i < RTE_DIM(policies); i++) {
>  		if (strcmp(policies[i].pkt.vm_name, pkt->vm_name) == 0) {
>  			policies[i].enabled = 0;
>  			return 0;
> @@ -792,6 +827,8 @@ read_json_packet(struct channel_info *chan_info)
>  	int n_bytes, ret;
>  	json_t *root;
>  	json_error_t error;
> +	const char *resource_name;
> +
> 
>  	/* read opening brace to closing brace */
>  	do {
> @@ -828,13 +865,15 @@ read_json_packet(struct channel_info *chan_info)
>  		root = json_loads(json_data, 0, &error);
> 
>  		if (root) {
> +			resource_name =
> get_resource_name_from_chn_path(
> +				chan_info->channel_path);
>  			/*
>  			 * Because our data is now in the json
>  			 * object, we can overwrite the pkt
>  			 * with a channel_packet struct, using
>  			 * parse_json_to_pkt()
>  			 */
> -			ret = parse_json_to_pkt(root, &pkt);
> +			ret = parse_json_to_pkt(root, &pkt, resource_name);
>  			json_decref(root);
>  			if (ret) {
>  				RTE_LOG(ERR, CHANNEL_MONITOR,
> @@ -891,9 +930,9 @@ run_channel_monitor(void)
>  		}
>  		rte_delay_us(time_period_ms*1000);
>  		if (policy_is_set) {
> -			int j;
> +			unsigned int j;
> 
> -			for (j = 0; j < MAX_CLIENTS; j++) {
> +			for (j = 0; j < RTE_DIM(policies); j++) {
>  				if (policies[j].enabled == 1)
>  					apply_policy(&policies[j]);
>  			}
> diff --git a/examples/vm_power_manager/main.c
> b/examples/vm_power_manager/main.c
> index 893bf4cdd..9f24cf69b 100644
> --- a/examples/vm_power_manager/main.c
> +++ b/examples/vm_power_manager/main.c
> @@ -421,7 +421,7 @@ main(int argc, char **argv)
>  		return -1;
>  	}
> 
> -	add_host_channel();
> +	add_host_channels();
> 
>  	printf("Running core monitor on lcore id %d\n", lcore_id);
>  	rte_eal_remote_launch(run_core_monitor, NULL, lcore_id);
> --
> 2.17.1

Tested-by: Marcin Hajkowski <marcinx.hajkowski at intel.com>