Hi Anatoly,
On 25/9/2018 10:48 AM, Burakov, Anatoly wrote:
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.h...@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 :)
vm_power_manager is only expected to be a single instance looking after
the power across all cores in a system, so we do not expect multiple
instance of this to be running.
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?
Fixed in next version.
+ 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?
I'll add a function to generate this string. And I've dropped the ".0".
It was inherited from the VM channel code, and in this case there is no
index needed.
+
+ errno = 0;
+ ret = mkfifo(socket_path, 0666);
0666 seems like overly permissive to me?
changed to 660
+ if ((errno != EEXIST) && (ret < 0)) {
+ printf(" %d %d, %d\n", ret, EEXIST, errno);
This looks like a leftover debug printf?
Removed in next rev.
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?
Removed in next rev.
+ 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.
Sure. Changed to 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), "%sfifo.%u",
+ CHANNEL_MGR_SOCKET_PATH, 0);
Creating FIFO path again. Definitely needs a function :)
Function created. :)
+ 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?
Sure. I've split it out into it's own commit in next version, and
renamed it as MAX_CLIENTS.
#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?
I was sticking with the existing style, but I do prefer the new lines.
Will change in the new version.
+ 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 :)
One can never have enough newlines. :)
Removed in the next version.
+ 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?
Done in next version .
Thanks for the review, Anatoly.
Rgds,
Dave.
