> From: Rao, Nikhil
> Sent: Wednesday, September 13, 2017 7:53 PM
> To: jerin.ja...@caviumnetworks.com; Richardson, Bruce
> <bruce.richard...@intel.com>
> Cc: Eads, Gage <gage.e...@intel.com>; dev@dpdk.org; tho...@monjalon.net; Van
> Haaren, Harry <harry.van.haa...@intel.com>; hemant.agra...@nxp.com;
> nipun.gu...@nxp.com; Vangati, Narender <narender.vang...@intel.com>; Carrillo,
> Erik G <erik.g.carri...@intel.com>; Gujjar, Abhinandan S
> <abhinandan.guj...@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....@intel.com>
> Signed-off-by: Gage Eads <gage.e...@intel.com>
> Signed-off-by: Abhinandan Gujjar <abhinandan.guj...@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.
