From: "Rao, Nikhil" <nikhil.rao@intel.com>
To: "Van Haaren, Harry" <harry.van.haaren@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: Tue, 19 Sep 2017 20:55:22 +0530 [thread overview]
Message-ID: <a41f2984-cc00-44fa-bb9d-65c97d539782@intel.com> (raw)
In-Reply-To: <E923DB57A917B54B9182A2E928D00FA650F9FAE6@IRSMSX101.ger.corp.intel.com>
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 <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
>
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
next prev parent reply other threads:[~2017-09-19 15:25 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
2017-09-19 15:25 ` Rao, Nikhil [this message]
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=a41f2984-cc00-44fa-bb9d-65c97d539782@intel.com \
--to=nikhil.rao@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=harry.van.haaren@intel.com \
--cc=hemant.agrawal@nxp.com \
--cc=jerin.jacob@caviumnetworks.com \
--cc=narender.vangati@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).