Hi Anatoly,
On 09/07/2019 16:12, Burakov, Anatoly wrote:
> On 09-Jul-19 4:07 PM, 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>
>>
>> ---
>> 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.
>> ---
>
> <snip>
>
>> - 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 < RTE_MAX_LCORE; i++) {
>
> You're going up to RTE_MAX_LCORE here, but up to ci->core_count in the
> original loop. Is that intentional?
>
> Other than that,
>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
>
Just to be totally clean, I've fixed the loop limit, and respun as v6. :)
Thanks for the review.
Rgds,
Dave.
@@ -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.
@@ -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 < RTE_MAX_LCORE; 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
@@ -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
@@ -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]);
}
@@ -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);