From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 18C0A1B160 for ; Mon, 18 Sep 2017 17:36:34 +0200 (CEST) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Sep 2017 08:36:33 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,413,1500966000"; d="scan'208";a="153129731" Received: from irsmsx105.ger.corp.intel.com ([163.33.3.28]) by fmsmga006.fm.intel.com with ESMTP; 18 Sep 2017 08:36:32 -0700 Received: from irsmsx101.ger.corp.intel.com ([169.254.1.22]) by irsmsx105.ger.corp.intel.com ([169.254.7.75]) with mapi id 14.03.0319.002; Mon, 18 Sep 2017 16:36:31 +0100 From: "Van Haaren, Harry" To: "Rao, Nikhil" , "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" Thread-Topic: [PATCH 3/4] eventdev: Add eventdev ethernet Rx adapter Thread-Index: AQHTLHk7QVHMA2QYlECTUvGqxWKgnaK6ytyQ Date: Mon, 18 Sep 2017 15:36:31 +0000 Message-ID: References: <04bcb240-51fb-50dc-833c-60c33a420d6f@intel.com> <1505328781-23456-1-git-send-email-nikhil.rao@intel.com> In-Reply-To: <1505328781-23456-1-git-send-email-nikhil.rao@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMGQ2NmExMWMtYWQ4Ny00YWNlLTk2NWUtOGNhOTQwYjIyOTA2IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6ImZmcUdhdFFva1JjOUFTK1ptOVFkUWU5SGltNkYya0dqYitRMnpxMGxHeEk9In0= x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Mon, 18 Sep 2017 15:36:35 -0000 > From: Rao, Nikhil > Sent: Wednesday, September 13, 2017 7:53 PM > To: jerin.jacob@caviumnetworks.com; Richardson, Bruce > > Cc: Eads, Gage ; dev@dpdk.org; thomas@monjalon.net; = Van > Haaren, Harry ; hemant.agrawal@nxp.com; > nipun.gupta@nxp.com; Vangati, Narender ; Carr= illo, > Erik G ; Gujjar, Abhinandan S > > Subject: [PATCH 3/4] eventdev: Add eventdev ethernet Rx adapter >=20 > Add common APIs for configuring packet transfer from ethernet Rx > queues to event devices across HW & SW packet transfer mechanisms. > A detailed description of the adapter is contained in the header's > comments. >=20 > The adapter implementation uses eventdev PMDs to configure the packet > transfer if HW support is available and if not, it uses an EAL service > function that reads packets from ethernet Rx queues and injects these > as events into the event device. >=20 > Signed-off-by: Nikhil Rao > Signed-off-by: Gage Eads > Signed-off-by: Abhinandan Gujjar Generally looks good to me - I'll leave Acking and such to Jerin / others w= ho were more involved in the design process. > +++ 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[] =3D "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 > + > +#if RTE_BYTE_ORDER =3D=3D 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 a= bove. I don't have much experience with it - but it looks like there's some dupli= cation here. > +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 inter= nal visibility function..? Perhaps I missed something. A few more _functions() like this below. > +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 _pre= fixed version? > +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 =3D=3D NULL) { > + ret =3D rte_event_eth_rx_adapter_init(); > + if (ret) > + return ret; > + } > + > + rx_adapter =3D id_to_rx_adapter(id); > + if (rx_adapter !=3D NULL) { > + RTE_EDEV_LOG_ERR("Eth Rx adapter exists id =3D %" PRIu8, id); > + return -EEXIST; > + } > + > + socket_id =3D rte_event_dev_socket_id(dev_id); > + snprintf(mem_name, sizeof(adapter_mem_name) + 4, "%s%d", > + adapter_mem_name, id); > + > + rx_adapter =3D rte_zmalloc_socket(mem_name, sizeof(*rx_adapter), > + RTE_CACHE_LINE_SIZE, socket_id); > + if (rx_adapter =3D=3D NULL) { > + RTE_EDEV_LOG_ERR("failed to get mem for rx adapter"); > + return -ENOMEM; > + } > + > + rx_adapter->eventdev_id =3D dev_id; > + rx_adapter->socket_id =3D socket_id; > + rx_adapter->conf_cb =3D conf_cb; > + rx_adapter->conf_arg =3D conf_arg; > + strcpy(rx_adapter->mem_name, mem_name); > + rx_adapter->eth_devices =3D rte_zmalloc_socket(rx_adapter->mem_name, > + rte_eth_dev_count() * > + sizeof(struct eth_device_info), 0, > + socket_id); > + if (rx_adapter->eth_devices =3D=3D 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. There's also some checkpatch warnings on the patch page: http://dpdk.org/ml/archives/test-report/2017-July/024634.html In general - looks like really good work - these are just improvements, not= hing major discovered.