From: "Hunt, David" <david.hunt@intel.com>
To: "Burakov, Anatoly" <anatoly.burakov@intel.com>, dev@dpdk.org
Cc: john.mcnamara@intel.com, stephen@networkplumber.org,
lei.a.yao@intel.com, bruce.richardson@intel.com
Subject: Re: [dpdk-dev] [PATCH v3 4/8] examples/power: add host channel to power manager
Date: Tue, 25 Sep 2018 14:55:56 +0100 [thread overview]
Message-ID: <d5d01781-8962-d8a3-1e4b-eb60be4cccfb@intel.com> (raw)
In-Reply-To: <8a415a74-fdf6-057d-3788-b6c9c0957fb0@intel.com>
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.hunt@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.
next prev parent reply other threads:[~2018-09-25 13:57 UTC|newest]
Thread overview: 110+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-30 10:54 [dpdk-dev] [PATCH v1 0/7] add json power policy interface for containers David Hunt
2018-08-30 10:54 ` [dpdk-dev] [PATCH v1 1/7] examples/power: add checks around hypervisor David Hunt
2018-08-30 16:59 ` Stephen Hemminger
2018-09-12 10:53 ` Hunt, David
2018-08-30 10:54 ` [dpdk-dev] [PATCH v1 2/7] lib/power: add changes for host commands/policies David Hunt
2018-08-30 16:59 ` Stephen Hemminger
2018-09-12 10:51 ` Hunt, David
2018-08-30 10:54 ` [dpdk-dev] [PATCH v1 3/7] examples/power: add necessary changes to guest app David Hunt
2018-08-30 10:54 ` [dpdk-dev] [PATCH v1 4/7] examples/power: add host channel to power manager David Hunt
2018-09-04 7:31 ` Yao, Lei A
2018-09-12 12:07 ` Hunt, David
2018-08-30 10:54 ` [dpdk-dev] [PATCH v1 5/7] examples/power: add json string handling David Hunt
2018-08-30 17:00 ` Stephen Hemminger
2018-08-31 13:55 ` Hunt, David
2018-09-12 10:54 ` Hunt, David
2018-08-30 10:54 ` [dpdk-dev] [PATCH v1 6/7] doc/vm_power_manager: add JSON interface API info David Hunt
2018-09-04 5:17 ` Yao, Lei A
2018-09-12 11:31 ` Hunt, David
2018-08-30 10:54 ` [dpdk-dev] [PATCH v1 7/7] examples/power: add json example files David Hunt
2018-09-12 14:49 ` [dpdk-dev] [PATCH v2 0/7] add json power policy interface for containers David Hunt
2018-09-12 14:49 ` [dpdk-dev] [PATCH v2 1/7] examples/power: add checks around hypervisor David Hunt
2018-09-12 14:49 ` [dpdk-dev] [PATCH v2 2/7] lib/power: add changes for host commands/policies David Hunt
2018-09-12 14:49 ` [dpdk-dev] [PATCH v2 3/7] examples/power: add necessary changes to guest app David Hunt
2018-09-12 14:49 ` [dpdk-dev] [PATCH v2 4/7] examples/power: add host channel to power manager David Hunt
2018-09-12 14:49 ` [dpdk-dev] [PATCH v2 5/7] examples/power: add json string handling David Hunt
2018-09-12 14:49 ` [dpdk-dev] [PATCH v2 6/7] doc/vm_power_manager: add JSON interface API info David Hunt
2018-09-12 14:49 ` [dpdk-dev] [PATCH v2 7/7] examples/power: add json example files David Hunt
2018-09-14 13:53 ` [dpdk-dev] [PATCH v3 0/8] add json power policy interface for containers David Hunt
2018-09-14 13:53 ` [dpdk-dev] [PATCH v3 1/8] examples/power: add checks around hypervisor David Hunt
2018-09-25 9:20 ` Burakov, Anatoly
2018-09-25 13:47 ` Hunt, David
2018-09-14 13:54 ` [dpdk-dev] [PATCH v3 2/8] lib/power: add changes for host commands/policies David Hunt
2018-09-25 9:21 ` Burakov, Anatoly
2018-09-25 13:47 ` Hunt, David
2018-09-14 13:54 ` [dpdk-dev] [PATCH v3 3/8] examples/power: add necessary changes to guest app David Hunt
2018-09-25 9:23 ` Burakov, Anatoly
2018-09-14 13:54 ` [dpdk-dev] [PATCH v3 4/8] examples/power: add host channel to power manager David Hunt
2018-09-25 9:48 ` Burakov, Anatoly
2018-09-25 13:55 ` Hunt, David [this message]
2018-09-14 13:54 ` [dpdk-dev] [PATCH v3 5/8] examples/power: add json string handling David Hunt
2018-09-25 11:27 ` Burakov, Anatoly
2018-09-25 14:00 ` Hunt, David
2018-09-25 14:15 ` Burakov, Anatoly
2018-09-25 15:15 ` Hunt, David
2018-09-25 15:31 ` Burakov, Anatoly
2018-09-14 13:54 ` [dpdk-dev] [PATCH v3 6/8] examples/power: add meson/ninja build support David Hunt
2018-09-14 14:01 ` Bruce Richardson
2018-09-14 13:54 ` [dpdk-dev] [PATCH v3 7/8] doc/vm_power_manager: add JSON interface API info David Hunt
2018-09-14 13:54 ` [dpdk-dev] [PATCH v3 8/8] examples/power: add json example files David Hunt
2018-09-26 13:40 ` [dpdk-dev] [PATCH v4 0/11] add json power policy interface for containers David Hunt
2018-09-26 13:40 ` [dpdk-dev] [PATCH v4 01/11] examples/power: add checks around hypervisor David Hunt
2018-09-26 13:54 ` Burakov, Anatoly
2018-09-26 13:40 ` [dpdk-dev] [PATCH v4 02/11] examples/power: allow for number of vms to be zero David Hunt
2018-09-26 13:54 ` Burakov, Anatoly
2018-09-26 13:40 ` [dpdk-dev] [PATCH v4 03/11] lib/power: add changes for host commands/policies David Hunt
2018-09-26 13:54 ` Burakov, Anatoly
2018-09-26 13:40 ` [dpdk-dev] [PATCH v4 04/11] examples/power: add necessary changes to guest app David Hunt
2018-09-26 13:40 ` [dpdk-dev] [PATCH v4 05/11] examples/power: add host channel to power manager David Hunt
2018-09-26 14:22 ` Burakov, Anatoly
2018-09-26 13:40 ` [dpdk-dev] [PATCH v4 06/11] examples/power: increase allowed number of clients David Hunt
2018-09-26 14:23 ` Burakov, Anatoly
2018-09-26 13:40 ` [dpdk-dev] [PATCH v4 07/11] examples/power: add json string handling David Hunt
2018-09-26 14:24 ` Burakov, Anatoly
2018-09-26 13:40 ` [dpdk-dev] [PATCH v4 08/11] examples/power: clean up verbose messages David Hunt
2018-09-26 14:25 ` Burakov, Anatoly
2018-09-26 13:40 ` [dpdk-dev] [PATCH v4 09/11] examples/power: add meson/ninja build support David Hunt
2018-09-26 13:40 ` [dpdk-dev] [PATCH v4 10/11] doc/vm_power_manager: add JSON interface API info David Hunt
2018-09-26 14:32 ` Kovacevic, Marko
2018-09-26 13:40 ` [dpdk-dev] [PATCH v4 11/11] examples/power: add json example files David Hunt
2018-09-26 15:58 ` Kovacevic, Marko
2018-09-26 16:14 ` Hunt, David
2018-09-26 16:37 ` [dpdk-dev] [PATCH v5 0/10] add json power policy interface for containers David Hunt
2018-09-26 16:37 ` [dpdk-dev] [PATCH v5 01/10] examples/power: add checks around hypervisor David Hunt
2018-09-26 16:37 ` [dpdk-dev] [PATCH v5 02/10] examples/power: allow for number of vms to be zero David Hunt
2018-09-26 16:37 ` [dpdk-dev] [PATCH v5 03/10] lib/power: add changes for host commands/policies David Hunt
2018-09-26 16:37 ` [dpdk-dev] [PATCH v5 04/10] examples/power: add necessary changes to guest app David Hunt
2018-09-26 16:37 ` [dpdk-dev] [PATCH v5 05/10] examples/power: add host channel to power manager David Hunt
2018-09-26 16:37 ` [dpdk-dev] [PATCH v5 06/10] examples/power: increase allowed number of clients David Hunt
2018-09-26 16:37 ` [dpdk-dev] [PATCH v5 07/10] examples/power: add json string handling David Hunt
2018-09-30 1:54 ` Yao, Lei A
2018-10-01 11:03 ` Hunt, David
2018-09-26 16:37 ` [dpdk-dev] [PATCH v5 08/10] examples/power: clean up verbose messages David Hunt
2018-09-26 16:37 ` [dpdk-dev] [PATCH v5 09/10] examples/power: add meson/ninja build support David Hunt
2018-09-26 16:37 ` [dpdk-dev] [PATCH v5 10/10] doc/vm_power_manager: add JSON interface API info David Hunt
2018-09-29 2:42 ` Yao, Lei A
2018-10-01 11:02 ` Hunt, David
2018-10-02 8:43 ` [dpdk-dev] [PATCH v6 0/10] add json power policy interface for containers David Hunt
2018-10-02 8:43 ` [dpdk-dev] [PATCH v6 01/10] examples/power: add checks around hypervisor David Hunt
2018-10-02 8:43 ` [dpdk-dev] [PATCH v6 02/10] examples/power: allow for number of vms to be zero David Hunt
2018-10-02 8:43 ` [dpdk-dev] [PATCH v6 03/10] lib/power: add changes for host commands/policies David Hunt
2018-10-02 8:43 ` [dpdk-dev] [PATCH v6 04/10] examples/power: add necessary changes to guest app David Hunt
2018-10-02 8:43 ` [dpdk-dev] [PATCH v6 05/10] examples/power: add host channel to power manager David Hunt
2018-10-02 8:43 ` [dpdk-dev] [PATCH v6 06/10] examples/power: increase allowed number of clients David Hunt
2018-10-02 8:43 ` [dpdk-dev] [PATCH v6 07/10] examples/power: add json string handling David Hunt
2018-10-02 8:43 ` [dpdk-dev] [PATCH v6 08/10] examples/power: clean up verbose messages David Hunt
2018-10-02 8:43 ` [dpdk-dev] [PATCH v6 09/10] examples/power: add meson/ninja build support David Hunt
2018-10-02 8:43 ` [dpdk-dev] [PATCH v6 10/10] doc/vm_power_manager: add JSON interface API info David Hunt
2018-10-17 13:05 ` [dpdk-dev] [PATCH v7 0/10] add json power policy interface for containers David Hunt
2018-10-17 13:05 ` [dpdk-dev] [PATCH v7 01/10] examples/power: add checks around hypervisor David Hunt
2018-10-17 13:05 ` [dpdk-dev] [PATCH v7 02/10] examples/power: allow for number of vms to be zero David Hunt
2018-10-17 13:05 ` [dpdk-dev] [PATCH v7 03/10] lib/power: add changes for host commands/policies David Hunt
2018-10-17 13:05 ` [dpdk-dev] [PATCH v7 04/10] examples/power: add necessary changes to guest app David Hunt
2018-10-17 13:05 ` [dpdk-dev] [PATCH v7 05/10] examples/power: add host channel to power manager David Hunt
2018-10-17 13:05 ` [dpdk-dev] [PATCH v7 06/10] examples/power: increase allowed number of clients David Hunt
2018-10-17 13:05 ` [dpdk-dev] [PATCH v7 07/10] examples/power: add json string handling David Hunt
2018-10-17 13:05 ` [dpdk-dev] [PATCH v7 08/10] examples/power: clean up verbose messages David Hunt
2018-10-17 13:05 ` [dpdk-dev] [PATCH v7 09/10] examples/power: add meson/ninja build support David Hunt
2018-10-17 13:05 ` [dpdk-dev] [PATCH v7 10/10] doc/vm_power_manager: add JSON interface API info David Hunt
2018-10-26 0:05 ` Thomas Monjalon
2018-10-26 8:45 ` [dpdk-dev] [PATCH v7 0/10] add json power policy interface for containers Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d5d01781-8962-d8a3-1e4b-eb60be4cccfb@intel.com \
--to=david.hunt@intel.com \
--cc=anatoly.burakov@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=john.mcnamara@intel.com \
--cc=lei.a.yao@intel.com \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).