From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Rao, Nikhil" Subject: Re: [PATCH v4 3/4] eventdev: Add eventdev ethernet Rx adapter Date: Sun, 24 Sep 2017 23:46:51 +0530 Message-ID: References: <1506028634-22998-1-git-send-email-nikhil.rao@intel.com> <1506028634-22998-4-git-send-email-nikhil.rao@intel.com> <20170922091032.GA5895@jerin> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: bruce.richardson@intel.com, gage.eads@intel.com, dev@dpdk.org, thomas@monjalon.net, harry.van.haaren@intel.com, hemant.agrawal@nxp.com, nipun.gupta@nxp.com, narender.vangati@intel.com, erik.g.carrillo@intel.com, abhinandan.gujjar@intel.com, santosh.shukla@caviumnetworks.com To: Jerin Jacob Return-path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id C89B72C36 for ; Sun, 24 Sep 2017 20:16:58 +0200 (CEST) In-Reply-To: <20170922091032.GA5895@jerin> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 9/22/2017 2:40 PM, Jerin Jacob wrote: > -----Original Message----- >> Date: Fri, 22 Sep 2017 02:47:13 +0530 >> From: Nikhil Rao >> To: jerin.jacob@caviumnetworks.com, bruce.richardson@intel.com >> CC: gage.eads@intel.com, dev@dpdk.org, thomas@monjalon.net, >> harry.van.haaren@intel.com, hemant.agrawal@nxp.com, nipun.gupta@nxp.com, >> narender.vangati@intel.com, erik.g.carrillo@intel.com, >> abhinandan.gujjar@intel.com, santosh.shukla@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 >> Signed-off-by: Gage Eads >> Signed-off-by: Abhinandan Gujjar > > 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 > > A Linefeed is norm here. > >> +#include >> + >> +#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 = >> + ð_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.