From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id D013AF94 for ; Tue, 19 Sep 2017 17:25:27 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga105.jf.intel.com with ESMTP; 19 Sep 2017 08:25:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,418,1500966000"; d="scan'208";a="1196799516" Received: from nikhilr-mobl.amr.corp.intel.com (HELO [10.252.172.121]) ([10.252.172.121]) by fmsmga001.fm.intel.com with ESMTP; 19 Sep 2017 08:25:23 -0700 To: "Van Haaren, Harry" , "jerin.jacob@caviumnetworks.com" , "Richardson, Bruce" Cc: "Eads, Gage" , "dev@dpdk.org" , "thomas@monjalon.net" , "hemant.agrawal@nxp.com" , "nipun.gupta@nxp.com" , "Vangati, Narender" , "Carrillo, Erik G" , "Gujjar, Abhinandan S" References: <04bcb240-51fb-50dc-833c-60c33a420d6f@intel.com> <1505328781-23456-1-git-send-email-nikhil.rao@intel.com> From: "Rao, Nikhil" Message-ID: Date: Tue, 19 Sep 2017 20:55:22 +0530 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 3/4] eventdev: Add eventdev ethernet Rx adapter 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, 19 Sep 2017 15:25:28 -0000 On 9/18/2017 9:06 PM, Van Haaren, Harry wrote: >> From: Rao, Nikhil > >> +++ b/lib/librte_eventdev/rte_event_eth_rx_adapter.c >> @@ -0,0 +1,1239 @@ >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "rte_eventdev.h" >> +#include "rte_eventdev_pmd.h" >> +#include "rte_event_eth_rx_adapter.h" >> + >> +#define BATCH_SIZE 32 >> +#define BLOCK_CNT_THRESHOLD 10 >> +#define ETH_EVENT_BUFFER_SIZE (4*BATCH_SIZE) >> + >> +static const char adapter_mem_name[] = "rx_adapter_mem_"; > > This bit isn't really DPDK style - usually we allocate a specific size for a buffer, > and then print into it using a safe method such as snprintf() > > > >> +struct rte_event_eth_rx_adapter { >> + /* event device identifier */ >> + uint8_t eventdev_id; >> + /* per ethernet device structure */ >> + struct eth_device_info *eth_devices; >> + /* malloc name */ >> + char mem_name[sizeof(adapter_mem_name) + 4]; > > See above comment, and why the magic + 4? > If we had a statically sized buffer, then this wouldn't be an issue. > There are multiple other instances of adapter_mem_name being accessed, > they have this strange "+ 4" throughout. Please refactor. > > Assuming this code is all initialization and configuration only, lets > not save a few bytes at the expense of a few bugs :D > OK. >> + >> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN >> +#define BE_16(x) (uint16_t)((x) >> 8 | (x) << 8) >> +#else >> +#define BE_16(x) (x) >> +#endif >> + >> +#define NETWORK_ORDER(x) BE_16(x) > > There is a rte_byteorder.h header file, which handles details such as the above. > I don't have much experience with it - but it looks like there's some duplication here. > Agreed, looks redundant. These are compile time conversions, and I see that rte_byteorder does handle that case. >> +static int >> +_rte_event_eth_rx_adapter_queue_del(struct rte_event_eth_rx_adapter >> *rx_adapter, >> + struct eth_device_info *dev_info, >> + uint16_t rx_queue_id) >> +{ > > Why the _ prefix before the function? This is already a static function, so > it won't be available outside this translation unit. If it is not meant to be > externally visible, don't use the "rte" prefix and voila, you have an internal > visibility function..? Perhaps I missed something. > > A few more _functions() like this below. > No particular reason for the _ prefix, as I was refactoring common code, it looks like I may have cut & pasted the queue_add/del function names. Will implement your suggestion. >> +static int >> +_rx_adapter_ctrl(struct rte_event_eth_rx_adapter *rx_adapter, int start) >> +{ > > This function is only called from one place? > It can probably be placed in that function itself, given it is the non _prefixed version? > The idea here was to separate out the error checking from the core logic. I will put it back. > >> +int >> +rte_event_eth_rx_adapter_create_ext(uint8_t id, uint8_t dev_id, >> + rx_adapter_conf_cb conf_cb, void *conf_arg) >> +{ >> + struct rte_event_eth_rx_adapter *rx_adapter; >> + int ret; >> + int socket_id; >> + uint8_t i; >> + char mem_name[sizeof(adapter_mem_name) + 4]; > > Same as before. > >> + RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL); >> + RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, -EINVAL); >> + if (!conf_cb) >> + return -EINVAL; >> + >> + if (rte_event_eth_rx_adapter == NULL) { >> + ret = rte_event_eth_rx_adapter_init(); >> + if (ret) >> + return ret; >> + } >> + >> + rx_adapter = id_to_rx_adapter(id); >> + if (rx_adapter != NULL) { >> + RTE_EDEV_LOG_ERR("Eth Rx adapter exists id = %" PRIu8, id); >> + return -EEXIST; >> + } >> + >> + socket_id = rte_event_dev_socket_id(dev_id); >> + snprintf(mem_name, sizeof(adapter_mem_name) + 4, "%s%d", >> + adapter_mem_name, id); >> + >> + rx_adapter = rte_zmalloc_socket(mem_name, sizeof(*rx_adapter), >> + RTE_CACHE_LINE_SIZE, socket_id); >> + if (rx_adapter == NULL) { >> + RTE_EDEV_LOG_ERR("failed to get mem for rx adapter"); >> + return -ENOMEM; >> + } >> + >> + rx_adapter->eventdev_id = dev_id; >> + rx_adapter->socket_id = socket_id; >> + rx_adapter->conf_cb = conf_cb; >> + rx_adapter->conf_arg = conf_arg; >> + strcpy(rx_adapter->mem_name, mem_name); >> + rx_adapter->eth_devices = rte_zmalloc_socket(rx_adapter->mem_name, >> + rte_eth_dev_count() * >> + sizeof(struct eth_device_info), 0, >> + socket_id); >> + if (rx_adapter->eth_devices == NULL) { >> + RTE_EDEV_LOG_ERR("failed to get mem for eth devices\n"); >> + rte_free(rx_adapter); >> + return -ENOMEM; >> + } > > This (and the previous other -ERROR returns) don't free the resources > that have been taken by this function up to this point. Improved tidying > up a after an error would be good. > The rte_free() for rx_adapter is the only cleanup needed, unless you are talking about the cleanup for rte_event_eth_rx_adapter_init() function. > > There's also some checkpatch warnings on the patch page: > http://dpdk.org/ml/archives/test-report/2017-July/024634.html > These warnings are for the previous version. > In general - looks like really good work - these are just improvements, nothing major discovered. > Thanks for the review. Nikhil