From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id BEB7DA00E6 for ; Tue, 9 Jul 2019 17:28:05 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 308D51B970; Tue, 9 Jul 2019 17:28:04 +0200 (CEST) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 4F62C1B95C for ; Tue, 9 Jul 2019 17:28:02 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Jul 2019 08:28:00 -0700 X-IronPort-AV: E=Sophos;i="5.63,470,1557212400"; d="scan'208";a="156209579" Received: from dhunt5-mobl4.ger.corp.intel.com (HELO [10.237.221.141]) ([10.237.221.141]) by orsmga007-auth.jf.intel.com with ESMTP/TLS/AES256-SHA; 09 Jul 2019 08:27:57 -0700 To: "thomas@monjalon.net" Cc: dev@dpdk.org, lukaszx.gosiewski@intel.com, anatoly.burakov@intel.com, Marcin Hajkowski , Lukasz Krakowiak References: <20190613092117.7252-2-marcinx.hajkowski@intel.com> <20190709152130.18279-1-david.hunt@intel.com> From: "Hunt, David" Message-ID: Date: Tue, 9 Jul 2019 16:27:57 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: <20190709152130.18279-1-david.hunt@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH v6] power: add fifo per core for JSON interface 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > > 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 > Signed-off-by: Lukasz Gosiewski > Signed-off-by: Marcin Hajkowski > Tested-by: David Hunt > Acked-by: Anatoly Burakov > > --- > 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 > #include > #include > +#include > > #include > #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);