From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <david.hunt@intel.com> Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id C0FE11B161 for <dev@dpdk.org>; Tue, 25 Sep 2018 15:57:09 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Sep 2018 06:57:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,302,1534834800"; d="scan'208";a="86400450" Received: from dhunt5-mobl2.ger.corp.intel.com (HELO [10.237.221.37]) ([10.237.221.37]) by orsmga003.jf.intel.com with ESMTP; 25 Sep 2018 06:55:58 -0700 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 References: <20180912144930.50578-1-david.hunt@intel.com> <20180914135406.52190-1-david.hunt@intel.com> <20180914135406.52190-5-david.hunt@intel.com> <8a415a74-fdf6-057d-3788-b6c9c0957fb0@intel.com> From: "Hunt, David" <david.hunt@intel.com> Message-ID: <d5d01781-8962-d8a3-1e4b-eb60be4cccfb@intel.com> Date: Tue, 25 Sep 2018 14:55:56 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <8a415a74-fdf6-057d-3788-b6c9c0957fb0@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH v3 4/8] examples/power: add host channel to power manager X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> X-List-Received-Date: Tue, 25 Sep 2018 13:57:10 -0000 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.