Re: [dpdk-dev] [PATCH v6] power: add fifo per core for JSON interface

["Hunt, David" <david.hunt@intel.com>]

Hi Thomas,

    Fyi, I am unable mark v4 as superseded in patchwork 
(http://patches.dpdk.org/project/dpdk/list/?series=4997), although I've 
done that with v5, v6 is the latest version.

Rgds,
Dave.


On 09/07/2019 16:21, David Hunt wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
>
> This patch implements a separate FIFO for each cpu core to improve the
> previous functionality where anyone with access to the FIFO could affect
> any core on the system. By using appropriate permissions, fifo interfaces
> can be configured to only affect the particular cores.
>
> Because each FIFO is per core, the following fields have been removed
> from the command JSON format: core_list, resource_id, name.
>
> Signed-off-by: Lukasz Krakowiak <lukaszx.krakowiak@intel.com>
> Signed-off-by: Lukasz Gosiewski <lukaszx.gosiewski@intel.com>
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> Tested-by: David Hunt <david.hunt@intel.com>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
>
> ---
> v2:
> * updated handling vm_name (use proper buff size)
> * rebase to master changes
>
> v3:
> * improvement to coding style
>
> v4:
> * rebase to tip of master
>
> v5:
> * merged docs into same patch as the code, as per mailing list policy
> * made changes out of review by Anatoly.
>
> v6:
> * Slight optimisation in the range of a for loop.
> ---
>   .../sample_app_ug/vm_power_management.rst     | 61 +++---------
>   examples/vm_power_manager/channel_manager.c   | 90 +++++++++++++-----
>   examples/vm_power_manager/channel_manager.h   |  7 +-
>   examples/vm_power_manager/channel_monitor.c   | 92 +++++++++++++------
>   examples/vm_power_manager/main.c              |  2 +-
>   5 files changed, 150 insertions(+), 102 deletions(-)
>
> diff --git a/doc/guides/sample_app_ug/vm_power_management.rst b/doc/guides/sample_app_ug/vm_power_management.rst
> index 5d9a26172..0ffff835e 100644
> --- a/doc/guides/sample_app_ug/vm_power_management.rst
> +++ b/doc/guides/sample_app_ug/vm_power_management.rst
> @@ -380,9 +380,16 @@ parsing functionality will not be present in the app.
>   
>   Sending a command or policy to the power manager application is achieved by
>   simply opening a fifo file, writing a JSON string to that fifo, and closing
> -the file.
> +the file. In actual implementation every core has own dedicated fifo[0..n],
> +where n is number of the last available core.
> +Having a dedicated fifo file per core allows using standard filesystem permissions
> +to ensure a given container can only write JSON commands into fifos it is allowed
> +to use.
>   
> -The fifo is at /tmp/powermonitor/fifo
> +The fifo is at /tmp/powermonitor/fifo[0..n]
> +
> +For example all cmds put to the /tmp/powermonitor/fifo7, will have
> +effect only on CPU[7].
>   
>   The JSON string can be a policy or instruction, and takes the following
>   format:
> @@ -405,19 +412,6 @@ arrays, etc. Examples of policies follow later in this document. The allowed
>   names and value types are as follows:
>   
>   
> -:Pair Name: "name"
> -:Description: Name of the VM or Host. Allows the parser to associate the
> -  policy with the relevant VM or Host OS.
> -:Type: string
> -:Values: any valid string
> -:Required: yes
> -:Example:
> -
> -    .. code-block:: javascript
> -
> -      "name", "ubuntu2"
> -
> -
>   :Pair Name: "command"
>   :Description: The type of packet we're sending to the power manager. We can be
>     creating or destroying a policy, or sending a direct command to adjust
> @@ -509,17 +503,6 @@ names and value types are as follows:
>   
>       "max_packet_thresh": 500000
>   
> -:Pair Name: "core_list"
> -:Description: The cores to which to apply the policy.
> -:Type: array of integers
> -:Values: array with list of virtual CPUs.
> -:Required: only policy CREATE/DESTROY
> -:Example:
> -
> -  .. code-block:: javascript
> -
> -    "core_list":[ 10, 11 ]
> -
>   :Pair Name: "workload"
>   :Description: When our policy is of type WORKLOAD, we need to specify how
>     heavy our workload is.
> @@ -566,17 +549,6 @@ names and value types are as follows:
>   
>       "unit", "SCALE_MAX"
>   
> -:Pair Name: "resource_id"
> -:Description: The core to which to apply the power command.
> -:Type: integer
> -:Values: valid core id for VM or host OS.
> -:Required: only POWER instruction
> -:Example:
> -
> -  .. code-block:: javascript
> -
> -    "resource_id": 10
> -
>   JSON API Examples
>   ~~~~~~~~~~~~~~~~~
>   
> @@ -585,12 +557,10 @@ Profile create example:
>     .. code-block:: javascript
>   
>       {"policy": {
> -      "name": "ubuntu",
>         "command": "create",
>         "policy_type": "TIME",
>         "busy_hours":[ 17, 18, 19, 20, 21, 22, 23 ],
> -      "quiet_hours":[ 2, 3, 4, 5, 6 ],
> -      "core_list":[ 11 ]
> +      "quiet_hours":[ 2, 3, 4, 5, 6 ]
>       }}
>   
>   Profile destroy example:
> @@ -598,8 +568,7 @@ Profile destroy example:
>     .. code-block:: javascript
>   
>       {"policy": {
> -      "name": "ubuntu",
> -      "command": "destroy",
> +      "command": "destroy"
>       }}
>   
>   Power command example:
> @@ -607,18 +576,16 @@ Power command example:
>     .. code-block:: javascript
>   
>       {"instruction": {
> -      "name": "ubuntu",
>         "command": "power",
> -      "unit": "SCALE_MAX",
> -      "resource_id": 10
> +      "unit": "SCALE_MAX"
>       }}
>   
>   To send a JSON string to the Power Manager application, simply paste the
> -example JSON string into a text file and cat it into the fifo:
> +example JSON string into a text file and cat it into the proper fifo:
>   
>     .. code-block:: console
>   
> -    cat file.json >/tmp/powermonitor/fifo
> +    cat file.json >/tmp/powermonitor/fifo[0..n]
>   
>   The console of the Power Manager application should indicate the command that
>   was just received via the fifo.
> diff --git a/examples/vm_power_manager/channel_manager.c b/examples/vm_power_manager/channel_manager.c
> index 0f5f9e287..2c1332257 100644
> --- a/examples/vm_power_manager/channel_manager.c
> +++ b/examples/vm_power_manager/channel_manager.c
> @@ -345,10 +345,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
> @@ -534,42 +546,70 @@ 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;
> +	struct channel_info *chan_infos[RTE_MAX_LCORE];
> +	int i;
>   
> -	fifo_path(socket_path, sizeof(socket_path));
> +	for (i = 0; i < RTE_MAX_LCORE; i++)
> +		chan_infos[i] = NULL;
>   
> -	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);
> +	for (i = 0; i < ci->core_count; i++) {
> +		if (ci->cd[i].global_enabled_cpus == 0)
> +			continue;
>   
> -	if (setup_host_channel_info(&chan_info, 0) < 0) {
> -		rte_free(chan_info);
> -		return 0;
> +		ret = fifo_path(socket_path, sizeof(socket_path), i);
> +		if (ret < 0)
> +			goto error;
> +
> +		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));
> +			goto error;
> +		}
> +		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);
> +			goto error;
> +		}
> +		chan_infos[i] = chan_info;
> +		rte_strlcpy(chan_info->channel_path, socket_path,
> +				sizeof(chan_info->channel_path));
> +
> +		if (setup_host_channel_info(&chan_info, i) < 0) {
> +			rte_free(chan_info);
> +			chan_infos[i] = NULL;
> +			goto error;
> +		}
> +		num_channels_enabled++;
>   	}
> -	num_channels_enabled++;
>   
>   	return num_channels_enabled;
> +error:
> +	/* Clean up the channels opened before we hit an error. */
> +	for (i = 0; i < ci->core_count; i++) {
> +		if (chan_infos[i] != NULL) {
> +			remove_channel_from_monitor(chan_infos[i]);
> +			close(chan_infos[i]->fd);
> +			rte_free(chan_infos[i]);
> +		}
> +	}
> +	return 0;
>   }
>   
>   int
> diff --git a/examples/vm_power_manager/channel_manager.h b/examples/vm_power_manager/channel_manager.h
> index 251d2163c..3766e93c8 100644
> --- a/examples/vm_power_manager/channel_manager.h
> +++ b/examples/vm_power_manager/channel_manager.h
> @@ -22,6 +22,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)
> @@ -206,13 +209,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 aab19ba57..b9a326e7d 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[RTE_MAX_LCORE];
>   
>   #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,20 +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")) {
> -			strlcpy(pkt->vm_name, json_string_value(value),
> -					sizeof(pkt->vm_name));
>   		} else if (!strcmp(key, "command")) {
>   			char command[32];
>   			strlcpy(command, json_string_value(value), 32);
> @@ -223,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);
> @@ -271,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;
>   }
> @@ -427,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;
> @@ -451,7 +485,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]);
> @@ -474,13 +508,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;
> @@ -801,6 +835,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 {
> @@ -832,13 +868,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,
> @@ -895,9 +933,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 bc15cb64e..54c704610 100644
> --- a/examples/vm_power_manager/main.c
> +++ b/examples/vm_power_manager/main.c
> @@ -434,7 +434,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);