From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 346465B38 for ; Tue, 25 Sep 2018 11:49:31 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Sep 2018 02:49:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,301,1534834800"; d="scan'208";a="265512345" Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.55]) ([10.237.220.55]) by fmsmga005.fm.intel.com with ESMTP; 25 Sep 2018 02:48:34 -0700 To: David Hunt , 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> From: "Burakov, Anatoly" Message-ID: <8a415a74-fdf6-057d-3788-b6c9c0957fb0@intel.com> Date: Tue, 25 Sep 2018 10:48:33 +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: <20180914135406.52190-5-david.hunt@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Sep 2018 09:49:32 -0000 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 > --- 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(-) > > + "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; > + } > +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) */ > - 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); > @@ -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