From: "Van Haaren, Harry" <harry.van.haaren@intel.com>
To: "Rao, Nikhil" <nikhil.rao@intel.com>,
"jerin.jacob@caviumnetworks.com" <jerin.jacob@caviumnetworks.com>,
"Richardson, Bruce" <bruce.richardson@intel.com>
Cc: "Eads, Gage" <gage.eads@intel.com>, "dev@dpdk.org" <dev@dpdk.org>,
"thomas@monjalon.net" <thomas@monjalon.net>,
"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
"nipun.gupta@nxp.com" <nipun.gupta@nxp.com>,
"Vangati, Narender" <narender.vangati@intel.com>,
"Carrillo, Erik G" <erik.g.carrillo@intel.com>,
"Gujjar, Abhinandan S" <abhinandan.gujjar@intel.com>
Subject: Re: [dpdk-dev] [PATCH 3/4] eventdev: Add eventdev ethernet Rx adapter
Date: Mon, 18 Sep 2017 15:36:31 +0000 [thread overview]
Message-ID: <E923DB57A917B54B9182A2E928D00FA650F9FAE6@IRSMSX101.ger.corp.intel.com> (raw)
In-Reply-To: <1505328781-23456-1-git-send-email-nikhil.rao@intel.com>
> From: Rao, Nikhil
> Sent: Wednesday, September 13, 2017 7:53 PM
> To: jerin.jacob@caviumnetworks.com; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: Eads, Gage <gage.eads@intel.com>; dev@dpdk.org; thomas@monjalon.net; Van
> Haaren, Harry <harry.van.haaren@intel.com>; hemant.agrawal@nxp.com;
> nipun.gupta@nxp.com; Vangati, Narender <narender.vangati@intel.com>; Carrillo,
> Erik G <erik.g.carrillo@intel.com>; Gujjar, Abhinandan S
> <abhinandan.gujjar@intel.com>
> Subject: [PATCH 3/4] eventdev: Add eventdev ethernet Rx adapter
>
> 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.
>
> 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.
>
> Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
> Signed-off-by: Gage Eads <gage.eads@intel.com>
> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
Generally looks good to me - I'll leave Acking and such to Jerin / others who were more involved in the design process.
<snip header, some implementation review below>
> +++ b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> @@ -0,0 +1,1239 @@
> +#include <rte_cycles.h>
> +#include <rte_common.h>
> +#include <rte_dev.h>
> +#include <rte_errno.h>
> +#include <rte_ethdev.h>
> +#include <rte_log.h>
> +#include <rte_malloc.h>
> +#include <rte_service_component.h>
> +#include <rte_thash.h>
> +
> +#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()
<snip>
> +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 == 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.
> +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.
> +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?
> +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.
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, nothing major discovered.
next prev parent reply other threads:[~2017-09-18 15:36 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-12 12:29 [dpdk-dev] [PATCH v3 0/4] eventdev: cover letter: ethernet Rx queue event adapter Nikhil Rao
2017-09-12 12:29 ` [dpdk-dev] [PATCH v3 1/4] eventdev: Add caps API and PMD callbacks for rte_event_eth_rx_adapter Nikhil Rao
2017-09-12 12:29 ` [dpdk-dev] [PATCH v3 2/4] eventdev: Add eth Rx adapter caps callback to SW evdev Nikhil Rao
2017-09-13 13:36 ` Nipun Gupta
2017-09-14 3:04 ` Rao, Nikhil
2017-09-14 4:37 ` Nipun Gupta
2017-09-12 12:29 ` [dpdk-dev] [PATCH v3 3/4] eventdev: Add eventdev ethernet Rx adapter Nikhil Rao
2017-09-12 4:17 ` Jerin Jacob
2017-09-13 9:41 ` Rao, Nikhil
2017-09-13 18:53 ` [dpdk-dev] [PATCH " Nikhil Rao
2017-09-15 6:07 ` Nipun Gupta
2017-09-18 4:54 ` Rao, Nikhil
2017-09-15 6:10 ` santosh
2017-09-15 11:10 ` Rao, Nikhil
2017-09-15 14:26 ` santosh
2017-09-18 15:36 ` Van Haaren, Harry [this message]
2017-09-19 15:25 ` Rao, Nikhil
2017-09-12 12:29 ` [dpdk-dev] [PATCH v3 4/4] eventdev: Add tests for event eth Rx adapter APIs Nikhil Rao
2017-09-15 6:07 ` santosh
2017-09-18 15:40 ` Van Haaren, Harry
2017-09-21 12:36 ` [dpdk-dev] [PATCH v3 0/4] eventdev: cover letter: ethernet Rx queue event adapter Jerin Jacob
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=E923DB57A917B54B9182A2E928D00FA650F9FAE6@IRSMSX101.ger.corp.intel.com \
--to=harry.van.haaren@intel.com \
--cc=abhinandan.gujjar@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=erik.g.carrillo@intel.com \
--cc=gage.eads@intel.com \
--cc=hemant.agrawal@nxp.com \
--cc=jerin.jacob@caviumnetworks.com \
--cc=narender.vangati@intel.com \
--cc=nikhil.rao@intel.com \
--cc=nipun.gupta@nxp.com \
--cc=thomas@monjalon.net \
/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).