[dpdk-dev] [PATCH v4 3/4] eventdev: Add eventdev ethernet Rx adapter

[Rao, Nikhil]

On 9/22/2017 2:40 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Fri, 22 Sep 2017 02:47:13 +0530
>> From: Nikhil Rao <nikhil.rao at intel.com>
>> To: jerin.jacob at caviumnetworks.com, bruce.richardson at intel.com
>> CC: gage.eads at intel.com, dev at dpdk.org, thomas at monjalon.net,
>>   harry.van.haaren at intel.com, hemant.agrawal at nxp.com, nipun.gupta at nxp.com,
>>   narender.vangati at intel.com, erik.g.carrillo at intel.com,
>>   abhinandan.gujjar at intel.com, santosh.shukla at caviumnetworks.com
>> Subject: [PATCH v4 3/4] eventdev: Add eventdev ethernet Rx adapter
>> X-Mailer: git-send-email 2.7.4
>>
>> 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 at intel.com>
>> Signed-off-by: Gage Eads <gage.eads at intel.com>
>> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar at intel.com>
> 
> Overall it looks good. A few top-level comments to start with,
> 
> 1) Please split this patch to minimum two
> a) Specification header
> b) and the implementation
> 
> 2) Please add a new section in MAINTAINERS files and update the new files
> responsibility.
> 
> 3) The doxygen file is not hooked into the documentation build.
> Check the doc/api/doxy-api-index.md file. You can use "make doc-api-html"
> to verify the doxygen html output.
> 
> 4) Since the APIs looks good and if there is no other objection,
> Can you add a programmer guide for Rx adapter.
> If you are busy it is fine not have in next version. Post RC1 or RC2 is
> fine. What do you think?

OK, Thanks for the detailed review. Will add the programmer guide to RC1.

> 
> 
>> ---
>>   lib/librte_eventdev/rte_event_eth_rx_adapter.h |  384 ++++++++
>>   lib/librte_eventdev/rte_event_eth_rx_adapter.c | 1238 ++++++++++++++++++++++++
>>   lib/Makefile                                   |    2 +-
>>   lib/librte_eventdev/Makefile                   |    2 +
>>   lib/librte_eventdev/rte_eventdev_version.map   |   11 +-
>>   5 files changed, 1635 insertions(+), 2 deletions(-)
>>   create mode 100644 lib/librte_eventdev/rte_event_eth_rx_adapter.h
>>   create mode 100644 lib/librte_eventdev/rte_event_eth_rx_adapter.c
>>
>> +#ifndef _RTE_EVENT_ETH_RX_ADAPTER_
>> +#define _RTE_EVENT_ETH_RX_ADAPTER_
>> +
>> +/**
>> + * @file
>> + *
>> + * RTE Event Ethernet Rx Adapter
>> + *
>> + * An eventdev-based packet processing application enqueues/dequeues mbufs
>> + * to/from the event device. The application uses the adapter APIs to configure
>> + * the packet flow between the ethernet devices and event devices. Depending on
>> + * on the capabilties of the eventdev PMD, the adapter may use a EAL service
> 
> s/capabilties/capabilities
> 
>> + * core function for packet transfer or use internal PMD functions to configure
>> + * the packet transfer between the ethernet device and the event device.
>> + *
>> + * The ethernet Rx event adapter's functions are:
>> + *  - rte_event_eth_rx_adapter_create_ext()
>> + *  - rte_event_eth_rx_adapter_create()/free()
>> + *  - rte_event_eth_rx_adapter_queue_add()/del()
>> + *  - rte_event_eth_rx_adapter_start()/stop()
>> + *  - rte_event_eth_rx_adapter_stats_get()/reset()
>> + *
>> + * The applicaton creates an event to ethernet adapter using
> 
> How about,
> The application creates an ethernet device to event device adapter using
> 
>> + * rte_event_eth_rx_adapter_create_ext() or rte_event_eth_rx_adapter_create()
>> + * functions.
>> + * The adapter needs to know which ethernet rx queues to poll for mbufs as well
>> + * as event device parameters such as the event queue identifier, event
>> + * priority and scheduling type that the adapter should use when constructing
>> + * events. The rte_event_eth_rx_adapter_queue_add() function is provided for
>> + * this purpose.
>> + * The servicing weight parameter in the rte_event_eth_rx_adapter_queue_conf
>> + * is applicable when the Rx adapter uses a service core function and is
>> + * intended to provide application control of the polling frequency of ethernet
>> + * device receive queues, for example, the application may want to poll higher
>> + * priority queues with a higher frequency but at the same time not starve
>> + * lower priority queues completely. If this parameter is zero and the receive
>> + * interrupt is enabled when configuring the device, the receive queue is
>> + * interrupt driven; else, the queue is assigned a servicing weight of one.
>> + *
>> + * If the adapter uses a rte_service function, then the application is also
>> + * required to assign a core to the service function and control the service
>> + * core using the rte_service APIs. The rte_event_eth_rx_adapter_service_id_get
>> + * function can be used to retrieve the service function ID of the adapter in
>> + * this case.
>> + *
>> + * Note: Interrupt driven receive queues are currentely unimplemented.
> 
> s/currentely/currently
> 
>> + */
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>> +#include <stdint.h>
> 
> A Linefeed is norm here.
> 
>> +#include <rte_service.h>
>> +
>> +#include "rte_eventdev.h"
>> +
>> +#define RTE_MAX_EVENT_ETH_RX_ADAPTER_INSTANCE 32
> 
> Considering the name space, How about to change to
> RTE_EVENT_ETH_RX_ADAPTER_MAX_INSTANCE? and fix the missing the doxygen comments
> 
>> +
>> +/* struct rte_event_eth_rx_adapter_queue_conf flags definitions */
>> +#define RTE_EVENT_ETH_RX_ADAPTER_QUEUE_FLOW_ID_VALID	0x1
>> +/**< This flag indicates the flow identifier is valid
>> + * @see rte_event_eth_rx_adapter_queue_conf
> 
> @see rte_event_eth_rx_adapter_queue_conf::rx_queue_flags
> @see rte_event_eth_rx_adapter_queue_conf::ev::flow_id
> 
> 

OK.
>> + */
>> +
>> +/**
>> + * Function type used for adapter configuration callback. The callback is
>> + * used to fill in members of the struct rte_event_eth_rx_adapter_conf, this
>> + * callback is invoked when creating a SW service for packet transfer from
>> + * ethdev queues to the event device. The SW service is created within the
>> + * rte_event_eth_rx_adapter_queue_add() function if packet required.
> 
> "if packet is required", does not seem to be correct usage.
> I guess, you mean, if packet required to transfer from ethdev queues to
> the event device or something like that?
> 

Yes, the text should have read "if SW based packet transfers from ethdev 
queues to the event device are required".

>> + *
>> + * @param id
>> + *  Adapter identifier.
>> + *
>> + * @param dev_id
>> + *  Event device identifier.
>> + *
>> + * @conf
>> + *  Structure that needs to be populated by this callback.
>> + *
>> + * @arg
>> + *  Argument to the callback. This is the same as the conf_arg passed to the
>> + *  rte_event_eth_rx_adapter_create_ext()
>> + */
>> +typedef int (*rx_adapter_conf_cb) (uint8_t id, uint8_t dev_id,
>> +			struct rte_event_eth_rx_adapter_conf *conf,
>> +			void *arg);
> 
> Public symbols should start with rte_

OK.
> 
>> +
>> +/** Rx queue configuration structure */
>> +struct rte_event_eth_rx_adapter_queue_conf {
>> +	uint32_t rx_queue_flags;
>> +	 /**< Flags for handling received packets
>> +	  * @see RTE_EVENT_ETH_RX_ADAPTER_QUEUE_FLOW_ID_VALID
>> +	  */
>> +	uint16_t servicing_weight;
>> +	/**< Relative polling frequency of ethernet receive queue, if this
>> +	 * is set to zero, the Rx queue is interrupt driven (unless rx queue
>> +	 * interrupts are not enabled for the ethernet device)
> 
> IMO, You can mentione it as hint and applicable only when using with
> SW based Rx adapter or something on similar lines.
> 

OK.
>> +	 */
>> +	struct rte_event ev;
>> +	/**<
>> +	 *  The values from the following event fields will be used when
>> +	 *  enqueuing mbuf events:
>> +	 *   - event_queue_id: Targeted event queue ID for received packets.
>> +	 *   - event_priority: Event priority of packets from this Rx queue in
>> +	 *                     the event queue relative to other events.
>> +	 *   - sched_type: Scheduling type for packets from this Rx queue.
>> +	 *   - flow_id: If the RTE_ETH_RX_EVENT_ADAPTER_QUEUE_FLOW_ID_VALID bit
>> +	 *		is set in rx_queue_flags, this flow_id is used for all
>> +	 *		packets received from this queue. Otherwise the flow ID
>> +	 *		is set to the RSS hash of the src and dst IPv4/6
>> +	 *		address.
>> +	 *
>> +	 * The event adapter sets ev.event_type to RTE_EVENT_TYPE_ETHDEV in the
>> +	 * enqueued event
> 
> When we worked on a prototype, we figured out that we need a separate event type
> for RX adapter. Probably RTE_EVENT_TYPE_ETHDEV_RX_ADAPTER?
> The Reason is:
> - In the HW based Rx adapter case, the packet are coming directly to eventdev once it is configured.
> - So on a HW implementation of the event dequeue(), CPU needs to convert HW specific
> metadata to mbuf
> - The event dequeue() is used in two cases
> a) octeontx eventdev driver used with any external NIC
> b) octeontx eventdev driver used with integrated NIC(without service
> core to inject the packet)
> We need some identifier to understand case (a) and (b).So, in dequeue(), if the
> packet is from RTE_EVENT_TYPE_ETHDEV then we can do "HW specific metadata" to mbuf
> conversion and in another case (!RTE_EVENT_TYPE_ETHDEV) result in no mbuf
> conversion.
> 
> Application can check if it is an Ethernet type event by
> ev.event_type == RTE_EVENT_TYPE_ETHDEV || ev.event_type ==
> RTE_EVENT_TYPE_ETHDEV_RX_ADAPTER
> 

As per my understanding, the case (a) uses an in built port
Is it possible for the eventdev PMD to do the conversion based off the 
eventdev port ?

> Thoughts?
> 
> 
>> +/**
>> + * Add receive queue to an event adapter. After a queue has been
>> + * added to the event adapter, the result of the application calling
>> + * rte_eth_rx_burst(eth_dev_id, rx_queue_id, ..) is undefined.
>> + *
>> + * @param id
>> + *  Adapter identifier.
>> + *
>> + * @param eth_dev_id
>> + *  Port identifier of Ethernet device.
>> + *
>> + * @param rx_queue_id
>> + *  Ethernet device receive queue index.
>> + *  If rx_queue_id is -1, then all Rx queues configured for
>> + *  the device are added. If the ethdev Rx queues can only be
>> + *  connected to a single event queue then rx_queue_id is
>> + *  required to be -1.
>> + *
>> + * @param conf
>> + *  Additonal configuration structure of type *rte_event_eth_rx_adapte_conf*
>> + *
>> + * @see
> 
> You can add @ see to denote the multi event queue enqueue capability
> 
>> + * @return
>> + *  - 0: Success, Receive queue added correctly.
>> + *  - <0: Error code on failure.
>> + */
>> +int rte_event_eth_rx_adapter_queue_add(uint8_t id,
>> +			uint8_t eth_dev_id,
>> +			int32_t rx_queue_id,
>> +			const struct rte_event_eth_rx_adapter_queue_conf *conf);
>> +
>> +/**
>> + * Delete receive queue from an event adapter.
>> + *
>> + * @param id
>> + *  Adapter identifier.
>> + *
>> + * @param eth_dev_id
>> + *  Port identifier of Ethernet device.
>> + *
>> + * @param rx_queue_id
>> + *  Ethernet device receive queue index.
>> + *  If rx_queue_id is -1, then all Rx queues configured for
>> + *  the device are deleted. If the ethdev Rx queues can only be
>> + *  connected to a single event queue then rx_queue_id is
>> + *  required to be -1.
> 
> You can add @ see to denote the multi event queue enqueue capability
> 
>> + *
>> + * @return
>> + *  - 0: Success, Receive queue deleted correctly.
>> + *  - <0: Error code on failure.
>> + */
>> +int rte_event_eth_rx_adapter_queue_del(uint8_t id, uint8_t eth_dev_id,
>> +				       int32_t rx_queue_id);
>> +
>> +
>> +/**
>> + * Retrieve statistics for an adapter
>> + *
>> + * @param id
>> + *  Adapter identifier.
>> + *
>> + * @param stats
> 
> @param [out] stats
> 
>> + *  A pointer to structure used to retrieve statistics for an adapter.
>> + *
>> + * @return
>> + *  - 0: Success, retrieved successfully.
>> + *  - <0: Error code on failure.
>> + */
>> +int rte_event_eth_rx_adapter_stats_get(uint8_t id,
>> +				struct rte_event_eth_rx_adapter_stats *stats);
>> +
>> +
>> +/**
>> + * Retrieve the service ID of an adapter. If the adapter doesn't use
>> + * a rte_service function, this function returns -ESRCH
>> + *
>> + * @param id
>> + *  Adapter identifier.
> 
> @param [out] service_id
> 

OK will fix.

>> + *
>> + * @return
>> + *  - 0: Success, statistics reset successfully.
>> + *  - <0: Error code on failure, if the adapter doesn't use a rte_service
>> + * function, this function returns -ESRCH.
>> + */
>> +int rte_event_eth_rx_adapter_service_id_get(uint8_t id, uint32_t *service_id);
>> +
>> +#ifdef __cplusplus
>> +}
>> +#endif
>> +#endif	/* _RTE_EVENT_ETH_RX_ADAPTER_ */
>> diff --git a/lib/librte_eventdev/rte_event_eth_rx_adapter.c b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
>> new file mode 100644
>> index 000000000..d5b655dae
>> --- /dev/null
>> +
>> +struct rte_event_eth_rx_adapter {
>> +	/* event device identifier */
> 
> Every elements you can start with capital letter.
> 
>> +	uint8_t eventdev_id;
>> +	/* per ethernet device structure */
>> +	struct eth_device_info *eth_devices;
>> +	/* malloc name */
>> +	char mem_name[ETH_RX_ADAPTER_MEM_NAME_LEN];
>> +	/* socket identifier cached from eventdev */
>> +	int socket_id;
>> +
>> +	/* elements below are used by SW service */
>> +
>> +	/* event port identifier */
>> +	uint8_t event_port_id;
>> +	/* per adapter EAL service */
>> +	uint32_t service_id;
>> +	/* lock to serialize config updates with service function */
>> +	rte_spinlock_t rx_lock;
>> +	/* max mbufs processed in any service function invocation */
>> +	uint32_t max_nb_rx;
>> +	/* Receive queues that need to be polled */
>> +	struct eth_rx_poll_entry *eth_rx_poll;
>> +	/* size of the eth_rx_poll array */
>> +	uint16_t num_rx_polled;
>> +	/* Weighted round robin schedule */
>> +	uint32_t *wrr_sched;
>> +	/* wrr_sched[] size */
>> +	uint32_t wrr_len;
>> +	/* Next entry in wrr[] to begin polling */
>> +	uint32_t wrr_pos;
>> +	/* Event burst buffer */
>> +	struct rte_eth_event_enqueue_buffer event_enqueue_buffer;
>> +	/* per adapter stats */
>> +	struct rte_event_eth_rx_adapter_stats stats;
>> +	/* Block count, counts upto BLOCK_CNT_THRESHOLD */
>> +	uint16_t enq_block_count;
>> +	/* Block start ts */
>> +	uint64_t rx_enq_block_start_ts;
>> +	/* Configuration callback for rte_service configuration */
>> +	rx_adapter_conf_cb conf_cb;
>> +	/* Configuration callback argument */
>> +	void *conf_arg;
>> +	/* Service initialization state */
>> +	uint8_t service_inited;
>> +	/* Total count of Rx queues in adapter */
>> +	uint32_t nb_queues;
>> +} __rte_cache_aligned;
>> +
>> +/* Per eth device */
>> +struct eth_device_info {
>> +	struct rte_eth_dev *dev;
>> +	struct eth_rx_queue_info *rx_queue;
>> +	/* Set if ethdev->eventdev packet transfer uses a
>> +	 * hardware mechanism
>> +	 */
>> +	uint8_t internal_event_port;
>> +	/* set if the adapter is processing rx queues for
> 
> s/set/Set
> 
>> +	 * this eth device and packet processing has been
>> +	 * started, allows for the code to know if the PMD
>> +	 * rx_adapter_stop callback needs to be invoked
>> +	 */
>> +	uint8_t dev_rx_started;
>> +	/* if nb_dev_queues > 0, the start callback will
>> +	 * be invoked if not already invoked
>> +	 */
>> +	uint16_t nb_dev_queues;
>> +};
>> +
>> +static struct rte_event_eth_rx_adapter **rte_event_eth_rx_adapter;
> 
> Avoid using rte for Internal object(**rte_event_eth_rx_adapter)_
>OK.

>> +static struct rte_event_port_conf
>> +		create_port_conf[RTE_MAX_EVENT_ETH_RX_ADAPTER_INSTANCE];
> 
> IMO, this memory can be stored in adapter memory to avoid global variable.
> 
Yes, if create() and queue_add() are called from different processes, it 
wouldn't work.

>> +
>> +static uint8_t default_rss_key[] = {
>> +	0x6d, 0x5a, 0x56, 0xda, 0x25, 0x5b, 0x0e, 0xc2,
>> +	0x41, 0x67, 0x25, 0x3d, 0x43, 0xa3, 0x8f, 0xb0,
>> +	0xd0, 0xca, 0x2b, 0xcb, 0xae, 0x7b, 0x30, 0xb4,
>> +	0x77, 0xcb, 0x2d, 0xa3, 0x80, 0x30, 0xf2, 0x0c,
>> +	0x6a, 0x42, 0xb7, 0x3b, 0xbe, 0xac, 0x01, 0xfa,
>> +};
> 
> Looks like the scope of this array is only for rte_event_eth_rx_adapter_init,
> if so please move it to stack.
> 
OK.

> 
>> +static uint8_t *rss_key_be;
> 
> Can we remove this global variable add it in in adapter memory?
> 

There is currently struct rte_event_eth_rx_adapter 
**rte_event_eth_rx_adapter that is an array of pointers to the adapters. 
rss_key_be points to memory after this array.

are you thinking of something like:

struct {
struct rte_event_eth_rx_adapter **rte_event_eth_rx_adapter
uint8_t *rss_key_be;
} global;


>> +}
>> +
>> +static inline void
>> +fill_event_buffer(struct rte_event_eth_rx_adapter *rx_adapter,
>> +	uint8_t dev_id,
>> +	uint16_t rx_queue_id,
>> +	struct rte_mbuf **mbufs,
>> +	uint16_t num)
>> +{
>> +	uint32_t i;
>> +	struct eth_device_info *eth_device_info =
>> +					&rx_adapter->eth_devices[dev_id];
>> +	struct eth_rx_queue_info *eth_rx_queue_info =
>> +					&eth_device_info->rx_queue[rx_queue_id];
>> +
>> +	int32_t qid = eth_rx_queue_info->event_queue_id;
>> +	uint8_t sched_type = eth_rx_queue_info->sched_type;
>> +	uint8_t priority = eth_rx_queue_info->priority;
>> +	uint32_t flow_id;
>> +	struct rte_event events[BATCH_SIZE];
>> +	struct rte_mbuf *m = mbufs[0];
>> +	uint32_t rss_mask;
>> +	uint32_t rss;
>> +	int do_rss;
>> +
>> +	/* 0xffff ffff if PKT_RX_RSS_HASH is set, otherwise 0 */
>> +	rss_mask = ~(((m->ol_flags & PKT_RX_RSS_HASH) != 0) - 1);
>> +	do_rss = !rss_mask && !eth_rx_queue_info->flow_id_mask;
>> +
>> +	for (i = 0; i < num; i++) {
>> +		m = mbufs[i];
>> +		struct rte_event *ev = &events[i];
>> +
>> +		rss = do_rss ? do_softrss(m) : m->hash.rss;
>> +		flow_id =
>> +		    eth_rx_queue_info->flow_id &
>> +				eth_rx_queue_info->flow_id_mask;
>> +		flow_id |= rss & ~eth_rx_queue_info->flow_id_mask;
>> +
>> +		ev->flow_id = flow_id;
>> +		ev->op = RTE_EVENT_OP_NEW;
>> +		ev->sched_type = sched_type;
>> +		ev->queue_id = qid;
>> +		ev->event_type = RTE_EVENT_TYPE_ETHDEV;
> 
> Thoughts on changing to RTE_EVENT_TYPE_ETHDEV_RX_ADAPTER as a solution for the
> problem described earlier.
> 
> 
>> +		ev->sub_event_type = 0;
>> +		ev->priority = priority;
>> +		ev->mbuf = m;
>> +
>> +		buf_event_enqueue(rx_adapter, ev);
>> +	}
>> +}
>> +
>> +/*
>> + * Polls receive queues added to the event adapter and enqueues received
>> + * packets to the event device.
>> + *
>> + * The receive code enqueues initially to a temporary buffer, the
>> + * temporary buffer is drained anytime it holds >= BATCH_SIZE packets
>> + *
>> + * If there isn't space available in the temporary buffer, packets from the
>> + * Rx queue arent dequeued from the eth device, this backpressures the
> 
> s/arent/aren't
> 
> 
>> + * eth device, in virtual device enviroments this backpressure is relayed to the
> 
> s/enviroments/environments
> 
>> + * hypervisor's switching layer where adjustments can be made to deal with
>> + * it.
>> + */
>> +static inline uint32_t
>> +eth_rx_poll(struct rte_event_eth_rx_adapter *rx_adapter)
>> +static int
>> +event_eth_rx_adapter_service_func(void *args)
>> +{
>> +	struct rte_event_eth_rx_adapter *rx_adapter = args;
>> +	struct rte_eth_event_enqueue_buffer *buf;
>> +
>> +	buf = &rx_adapter->event_enqueue_buffer;
>> +	if (!rte_spinlock_trylock(&rx_adapter->rx_lock))
>> +		return 0;
>> +	if (eth_rx_poll(rx_adapter) == 0 && buf->count)
>> +		flush_event_buffer(rx_adapter);
>> +	rte_spinlock_unlock(&rx_adapter->rx_lock);
>> +	return 0;
>> +}
>> +
>> +static int
>> +rte_event_eth_rx_adapter_init(void)
>> +{
>> +	const char *name = "rte_event_eth_rx_adapter_array";
>> +	const struct rte_memzone *mz;
>> +	unsigned int sz;
>> +	unsigned int rss_key_off;
>> +
>> +	sz = sizeof(*rte_event_eth_rx_adapter) *
>> +	    RTE_MAX_EVENT_ETH_RX_ADAPTER_INSTANCE;
> 
> I think, you need to use size of struct rte_event_eth_rx_adapter here. if so,
> we need **rte_event_eth_rx_adapter here. Right?
> 
> test code
> struct abc {
> 
>          uint64_t a[64];
> };
> 
> struct abc **k;
> 
> int main()
> {
> 	printf("%d %d %d\n", sizeof(k), sizeof(*k), sizeof(**k));
> 
> 	return 0;
> }
> 
> $./a.out
> 8 8 512
> 

The struct rte_event_eth_rx_adapter gets allocated in 
rte_event_eth_rx_adapter_create_ext()

> 
>> +	sz = RTE_ALIGN(sz, RTE_CACHE_LINE_SIZE);
>> +	rss_key_off = sz;
>> +	sz = RTE_ALIGN(sz + sizeof(default_rss_key), RTE_CACHE_LINE_SIZE);
>> +
>> +	mz = rte_memzone_lookup(name);
>> +	if (!mz) {
>> +		mz = rte_memzone_reserve_aligned(name, sz, rte_socket_id(), 0,
>> +						 RTE_CACHE_LINE_SIZE);
> 
> How about passing the socket id from the rte_event_dev_socket_id()?
> if eventdev is in node !=0  then it may not correct thing?

rte_event_eth_rx_adapter[] is a global across all adapters, event dev ID 
is unknown at this point. The struct rte_event_eth_rx_adapter that 
rte_event_eth_rx_adapter[] gets allocated using rte_event_dev_socket_id()

> 
>> +		if (mz) {
>> +			rte_convert_rss_key((uint32_t *)default_rss_key,
>> +			    (uint32_t *)(uintptr_t)(mz->addr_64 + rss_key_off),
>> +			    RTE_DIM(default_rss_key));
>> +		} else {
>> +			RTE_EDEV_LOG_ERR("failed to reserve memzone err = %"
>> +					PRId32, rte_errno);
>> +			return -rte_errno;
>> +		}
>> +	}
>> +
>> +	rte_event_eth_rx_adapter = mz->addr;
>> +	rss_key_be = (uint8_t *)(mz->addr_64 + rss_key_off);
>> +	return 0;
>> +}
>> +
>> +static int
>> +default_conf_cb(uint8_t id, uint8_t dev_id,
>> +		struct rte_event_eth_rx_adapter_conf *conf, void *arg)
>> +{
>> +
>> +	ret = rte_event_port_setup(dev_id, port_id, port_conf);
>> +	if (ret) {
>> +		RTE_EDEV_LOG_ERR("failed to setup event port %u\n",
>> +					port_id);
> 
> return or add goto to exit from here to avoid calling rte_event_dev_start below
> 
Could do the return but I wanted to leave the device in the same state 
as it was at entry into this function. Thoughts ?

>> +	} else {
>> +		conf->event_port_id = port_id;
>> +		conf->max_nb_rx = 128;
>> +	}
>> +
>> +	if (started)
>> +		rte_event_dev_start(dev_id);
>> +	return ret;
>> +}
>> +
>> +static int
>> +init_service(struct rte_event_eth_rx_adapter *rx_adapter, uint8_t id)
>> +{
>> +		&rx_adapter_conf, rx_adapter->conf_arg);
>> +	if (ret) {
>> +		RTE_EDEV_LOG_ERR("confguration callback failed err = %" PRId32,
> 
> s/configuration/configuration
> 
>> +			ret);
>> +		goto err_done;
>> +	}
>> +	rx_adapter->event_port_id = rx_adapter_conf.event_port_id;
>> +	rx_adapter->max_nb_rx = rx_adapter_conf.max_nb_rx;
>> +	rx_adapter->service_inited = 1;
>> +	return 0;
>> +
>> +err_done:
>> +	rte_service_component_unregister(rx_adapter->service_id);
>> +	return ret;
>> +}
>> +
>> +static void
>> +update_queue_info(struct rte_event_eth_rx_adapter *rx_adapter,
>> +		struct eth_device_info *dev_info,
>> +		int32_t rx_queue_id,
>> +		uint8_t add)
>> +{
>> +	struct eth_rx_queue_info *queue_info;
>> +	int enabled;
>> +	uint16_t i;
>> +
>> +	if (!dev_info->rx_queue)
>> +		return;
>> +
>> +	if (rx_queue_id == -1) {
>> +		for (i = 0; i < dev_info->dev->data->nb_rx_queues; i++) {
>> +			queue_info = &dev_info->rx_queue[i];
>> +			enabled = queue_info->queue_enabled;
>> +			if (add) {
>> +				rx_adapter->nb_queues += !enabled;
>> +				dev_info->nb_dev_queues += !enabled;
>> +			} else {
>> +				rx_adapter->nb_queues -= enabled;
>> +				dev_info->nb_dev_queues -= enabled;
>> +			}
>> +			queue_info->queue_enabled = !!add;
> 
> See next comment.
> 
>> +		}
>> +	} else {
>> +		queue_info = &dev_info->rx_queue[rx_queue_id];
>> +		enabled = queue_info->queue_enabled;
>> +		if (add) {
>> +			rx_adapter->nb_queues += !enabled;
>> +			dev_info->nb_dev_queues += !enabled;
>> +		} else {
>> +			rx_adapter->nb_queues -= enabled;
>> +			dev_info->nb_dev_queues -= enabled;
>> +		}
>> +		queue_info->queue_enabled = !!add;
> 
> Duplicate code. Worth to make it static inline to avoid duplicate code
> 

OK.
>> +	}
>> +}
>> +
>> +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[ETH_RX_ADAPTER_SERVICE_NAME_LEN];
>> +
>> +	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)
> 
> Remove negative logic and change to conf_cb == NULL. Its DPDK coding
> standard. There is a lot similar instance in this file.Please fix those
>

OK.
>> +		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) {
>> +int
>> +rte_event_eth_rx_adapter_create(uint8_t id, uint8_t dev_id,
>> +		struct rte_event_port_conf *port_config)
>> +{
>> +	if (!port_config)
> 
> port_config == NULL
> 
>> +		return -EINVAL;
>> +	RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL);
>> +
>> +	create_port_conf[id] = *port_config;
>> +	return rte_event_eth_rx_adapter_create_ext(id, dev_id,
>> +					default_conf_cb,
>> +					&create_port_conf[id]);
>> +}
>> +
>> +
>> +int
>> +rte_event_eth_rx_adapter_queue_add(uint8_t id,
>> +		uint8_t eth_dev_id,
>> +		int32_t rx_queue_id,
>> +		const struct rte_event_eth_rx_adapter_queue_conf *queue_conf)
>> +{
>> +	int ret;
>> +	uint32_t rx_adapter_cap;
>> +	struct rte_event_eth_rx_adapter *rx_adapter;
>> +	struct rte_eventdev *dev;
>> +	struct eth_device_info *dev_info;
>> +	int start_service = 0;
> 
> Duplicate store to zero.
> 
>> +
>> +	RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL);
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(eth_dev_id, -EINVAL);
>> +
>> +	rx_adapter = id_to_rx_adapter(id);
>> +	if (!rx_adapter || !queue_conf)
>> +		return -EINVAL;
>> +
>> +	dev = &rte_eventdevs[rx_adapter->eventdev_id];
>> +	ret = (*dev->dev_ops->eth_rx_adapter_caps_get)(dev,
>> +						&rte_eth_devices[eth_dev_id],
>> +						&rx_adapter_cap);
>> +	if (ret) {
>> +		RTE_EDEV_LOG_ERR("Failed to get adapter caps edev %" PRIu8
>> +			"eth port %" PRIu8, id, eth_dev_id);
>> +		return ret;
>> +	}
>> +
>> +	if (!(rx_adapter_cap & RTE_EVENT_ETH_RX_ADAPTER_CAP_FLOW_ID) &&
>> +		!(queue_conf->rx_queue_flags &
>> +			RTE_EVENT_ETH_RX_ADAPTER_QUEUE_FLOW_ID_VALID)) {
>> +		RTE_EDEV_LOG_ERR("Flow ID required for configuration,"
>> +				" eth port: %" PRIu8 " adapter id: %" PRIu8,
>> +				eth_dev_id, id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if ((rx_adapter_cap & RTE_EVENT_ETH_RX_ADAPTER_CAP_SINGLE_EVENTQ) &&
>> +		(rx_queue_id != -1)) {
>> +		RTE_EDEV_LOG_ERR("Rx queues can only be connected to single "
>> +			"event queue id %u eth port %u", id, eth_dev_id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (rx_queue_id != -1 && (uint16_t)rx_queue_id >=
>> +			rte_eth_devices[eth_dev_id].data->nb_rx_queues) {
>> +		RTE_EDEV_LOG_ERR("Invalid rx queue_id %" PRIu16,
>> +			 (uint16_t)rx_queue_id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	start_service = 0;
> 
> See above comment.
> 
OK will fix.