[dpdk-dev] [PATCH v3 4/8] examples/power: add host channel to power manager
Burakov, Anatoly
anatoly.burakov at intel.com
Tue Sep 25 11:48:33 CEST 2018
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.hunt at 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 :)
> 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?
> + 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?
> +
> + errno = 0;
> + ret = mkfifo(socket_path, 0666);
0666 seems like overly permissive to me?
> + if ((errno != EEXIST) && (ret < 0)) {
> + printf(" %d %d, %d\n", ret, EEXIST, errno);
This looks like a leftover debug printf?
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?
> + 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.
> + 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 :)
> + 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?
> #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?
> + 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 :)
> + 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?
--
Thanks,
Anatoly
More information about the dev
mailing list