>> -----Original Message----- >> From: Pavan Nikhilesh Bhagavatula <pbhagavat...@marvell.com> >> Sent: Thursday, March 25, 2021 7:25 PM >> To: Jayatheerthan, Jay <jay.jayatheert...@intel.com>; Jerin Jacob >Kollanukkaran <jer...@marvell.com>; Carrillo, Erik G >> <erik.g.carri...@intel.com>; Gujjar, Abhinandan S ><abhinandan.guj...@intel.com>; McDaniel, Timothy >> <timothy.mcdan...@intel.com>; hemant.agra...@nxp.com; Van >Haaren, Harry <harry.van.haa...@intel.com>; mattias.ronnblom >> <mattias.ronnb...@ericsson.com>; Ma, Liang J ><liang.j...@intel.com>; Ray Kinsella <m...@ashroe.eu>; Neil Horman >> <nhor...@tuxdriver.com> >> Cc: dev@dpdk.org >> Subject: RE: [dpdk-dev v21.11] [PATCH v5 8/8] eventdev: simplify Rx >adapter event vector config >> >> >> From: pbhagavat...@marvell.com <pbhagavat...@marvell.com> >> >> Sent: Wednesday, March 24, 2021 10:35 AM >> >> To: jer...@marvell.com; Jayatheerthan, Jay >> ><jay.jayatheert...@intel.com>; Carrillo, Erik G >> ><erik.g.carri...@intel.com>; Gujjar, >> >> Abhinandan S <abhinandan.guj...@intel.com>; McDaniel, Timothy >> ><timothy.mcdan...@intel.com>; hemant.agra...@nxp.com; Van >> >> Haaren, Harry <harry.van.haa...@intel.com>; mattias.ronnblom >> ><mattias.ronnb...@ericsson.com>; Ma, Liang J >> >> <liang.j...@intel.com>; Ray Kinsella <m...@ashroe.eu>; Neil >Horman >> ><nhor...@tuxdriver.com> >> >> Cc: dev@dpdk.org; Pavan Nikhilesh <pbhagavat...@marvell.com> >> >> Subject: [dpdk-dev v21.11] [PATCH v5 8/8] eventdev: simplify Rx >> >adapter event vector config >> >> >> >> From: Pavan Nikhilesh <pbhagavat...@marvell.com> >> >> >> >> Include vector configuration into the structure >> >> ``rte_event_eth_rx_adapter_queue_conf`` used when configuring >rest >> >> of the Rx adapter ethernet device Rx queue parameters. >> >> This simplifies event vector configuration as it avoids splitting >> >> configuration per Rx queue. >> >> >> >> Signed-off-by: Pavan Nikhilesh <pbhagavat...@marvell.com> >> >> --- >> >> app/test-eventdev/test_pipeline_common.c | 16 +- >> >> lib/librte_eventdev/eventdev_pmd.h | 29 --- >> >> .../rte_event_eth_rx_adapter.c | 168 ++++++------------ >> >> .../rte_event_eth_rx_adapter.h | 27 --- >> >> lib/librte_eventdev/version.map | 1 - >> >> 5 files changed, 57 insertions(+), 184 deletions(-) >> >> >> >> diff --git a/app/test-eventdev/test_pipeline_common.c >b/app/test- >> >eventdev/test_pipeline_common.c >> >> index d5ef90500..76aee254b 100644 >> >> --- a/app/test-eventdev/test_pipeline_common.c >> >> +++ b/app/test-eventdev/test_pipeline_common.c >> >> @@ -331,7 +331,6 @@ pipeline_event_rx_adapter_setup(struct >> >evt_options *opt, uint8_t stride, >> >> uint16_t prod; >> >> struct rte_mempool *vector_pool = NULL; >> >> struct rte_event_eth_rx_adapter_queue_conf queue_conf; >> >> - struct rte_event_eth_rx_adapter_event_vector_config >> >vec_conf; >> >> >> >> memset(&queue_conf, 0, >> >> sizeof(struct >> >rte_event_eth_rx_adapter_queue_conf)); >> >> @@ -397,8 +396,12 @@ pipeline_event_rx_adapter_setup(struct >> >evt_options *opt, uint8_t stride, >> >> } >> >> >> >> if (cap & >> >RTE_EVENT_ETH_RX_ADAPTER_CAP_EVENT_VECTOR) { >> >> + queue_conf.vector_sz = opt- >> >>vector_size; >> >> + queue_conf.vector_timeout_ns = >> >> + opt->vector_tmo_nsec; >> >> queue_conf.rx_queue_flags |= >> >> >> > RTE_EVENT_ETH_RX_ADAPTER_QUEUE_EVENT_VECTOR; >> >> + queue_conf.vector_mp = vector_pool; >> >> } else { >> >> evt_err("Rx adapter doesn't support >> >event vector"); >> >> return -EINVAL; >> >> @@ -418,17 +421,6 @@ pipeline_event_rx_adapter_setup(struct >> >evt_options *opt, uint8_t stride, >> >> return ret; >> >> } >> >> >> >> - if (opt->ena_vector) { >> >> - vec_conf.vector_sz = opt->vector_size; >> >> - vec_conf.vector_timeout_ns = opt- >> >>vector_tmo_nsec; >> >> - vec_conf.vector_mp = vector_pool; >> >> - if >> >(rte_event_eth_rx_adapter_queue_event_vector_config( >> >> - prod, prod, -1, &vec_conf) < 0) { >> >> - evt_err("Failed to configure event >> >vectorization for Rx adapter"); >> >> - return -EINVAL; >> >> - } >> >> - } >> >> - >> >> if (!(cap & >> >RTE_EVENT_ETH_RX_ADAPTER_CAP_INTERNAL_PORT)) { >> >> uint32_t service_id = -1U; >> >> >> >> diff --git a/lib/librte_eventdev/eventdev_pmd.h >> >b/lib/librte_eventdev/eventdev_pmd.h >> >> index 0f724ac85..63b3bc4b5 100644 >> >> --- a/lib/librte_eventdev/eventdev_pmd.h >> >> +++ b/lib/librte_eventdev/eventdev_pmd.h >> >> @@ -667,32 +667,6 @@ typedef int >> >(*eventdev_eth_rx_adapter_vector_limits_get_t)( >> >> const struct rte_eventdev *dev, const struct rte_eth_dev >> >*eth_dev, >> >> struct rte_event_eth_rx_adapter_vector_limits *limits); >> >> >> >> -struct rte_event_eth_rx_adapter_event_vector_config; >> >> -/** >> >> - * Enable event vector on an given Rx queue of a ethernet devices >> >belonging to >> >> - * the Rx adapter. >> >> - * >> >> - * @param dev >> >> - * Event device pointer >> >> - * >> >> - * @param eth_dev >> >> - * Ethernet device pointer >> >> - * >> >> - * @param rx_queue_id >> >> - * The Rx queue identifier >> >> - * >> >> - * @param config >> >> - * Pointer to the event vector configuration structure. >> >> - * >> >> - * @return >> >> - * - 0: Success. >> >> - * - <0: Error code returned by the driver function. >> >> - */ >> >> -typedef int (*eventdev_eth_rx_adapter_event_vector_config_t)( >> >> - const struct rte_eventdev *dev, const struct rte_eth_dev >> >*eth_dev, >> >> - int32_t rx_queue_id, >> >> - const struct rte_event_eth_rx_adapter_event_vector_config >> >*config); >> >> - >> >> typedef uint32_t rte_event_pmd_selftest_seqn_t; >> >> extern int rte_event_pmd_selftest_seqn_dynfield_offset; >> >> >> >> @@ -1118,9 +1092,6 @@ struct rte_eventdev_ops { >> >> eventdev_eth_rx_adapter_vector_limits_get_t >> >> eth_rx_adapter_vector_limits_get; >> >> /**< Get event vector limits for the Rx adapter */ >> >> - eventdev_eth_rx_adapter_event_vector_config_t >> >> - eth_rx_adapter_event_vector_config; >> >> - /**< Configure Rx adapter with event vector */ >> >> >> >> eventdev_timer_adapter_caps_get_t timer_adapter_caps_get; >> >> /**< Get timer adapter capabilities */ >> >> diff --git a/lib/librte_eventdev/rte_event_eth_rx_adapter.c >> >b/lib/librte_eventdev/rte_event_eth_rx_adapter.c >> >> index c71990078..a1990637f 100644 >> >> --- a/lib/librte_eventdev/rte_event_eth_rx_adapter.c >> >> +++ b/lib/librte_eventdev/rte_event_eth_rx_adapter.c >> >> @@ -1882,6 +1882,25 @@ rxa_add_queue(struct >> >rte_event_eth_rx_adapter *rx_adapter, >> >> } else >> >> qi_ev->flow_id = 0; >> >> >> >> + if (conf->rx_queue_flags & >> >> + RTE_EVENT_ETH_RX_ADAPTER_QUEUE_EVENT_VECTOR) { >> >> + queue_info->ena_vector = 1; >> >> + qi_ev->event_type = >> >RTE_EVENT_TYPE_ETH_RX_ADAPTER_VECTOR; >> >> + rxa_set_vector_data(queue_info, conf->vector_sz, >> >> + conf->vector_timeout_ns, conf- >> >>vector_mp, >> >> + rx_queue_id, dev_info->dev->data- >> >>port_id); >> >> + rx_adapter->ena_vector = 1; >> >> + rx_adapter->vector_tmo_ticks = >> >> + rx_adapter->vector_tmo_ticks >> >> + ? RTE_MIN(queue_info->vector_data >> >> + .vector_timeout_ticks, >> >> + rx_adapter- >> >>vector_tmo_ticks) >> >> + : queue_info- >> >>vector_data.vector_timeout_ticks; >> >> + rx_adapter->vector_tmo_ticks <<= 1; >> > >> >Any reason why we left shift here ? Applicable in patch 4/8 as well. >> >> Just so that we have half the precision of the lowest timeout, helps to >> Maintain accuracy. > >Maybe I am missing something here. For e.g. if the lowest timeout is 20 >ticks, would you want 10 or 40 ? Currently, its set to 40.
My bad this should be Rshift, I will fix it in 4/8 too. > >> >> > >> >> + TAILQ_INIT(&rx_adapter->vector_list); >> > >> >Can doing TAILQ_INIT every time a queue is added cause existing >> >elements to be wiped out ? Applicable in patch 4/8 as well. >> >> I don't think queues can be added when adapter is already started >> rxa_sw_add Isn't thready safe. > >Yes, it takes lock before calling rxa_sa_add. For internal_port >implementation, I don't see any lock being taken. Besides, since its >adapter related, it would be more relevant in adapter create than in >queue add. Sure, I will move the init code to adapter create. > >> >> > >> >> + rx_adapter->prev_expiry_ts = 0; >> > >> >Can setting this every time a queue is added affect existing queues >> >created with vector support and passing traffic ? Applicable in patch >4/8 >> >as well. >> >> Same as above. >> > >Same reasoning as above. > >> > >> >> + } >> >> + >> >> rxa_update_queue(rx_adapter, dev_info, rx_queue_id, 1); >> >> if (rxa_polled_queue(dev_info, rx_queue_id)) { >> >> rx_adapter->num_rx_polled += !pollq; >> >> @@ -1907,44 +1926,6 @@ rxa_add_queue(struct >> >rte_event_eth_rx_adapter *rx_adapter, >> >> } >> >> } >> >> >> >> -static void >> >> -rxa_sw_event_vector_configure( >> >> - struct rte_event_eth_rx_adapter *rx_adapter, uint16_t >> >eth_dev_id, >> >> - int rx_queue_id, >> >> - const struct rte_event_eth_rx_adapter_event_vector_config >> >*config) >> >> -{ >> >> - struct eth_device_info *dev_info = &rx_adapter- >> >>eth_devices[eth_dev_id]; >> >> - struct eth_rx_queue_info *queue_info; >> >> - struct rte_event *qi_ev; >> >> - >> >> - if (rx_queue_id == -1) { >> >> - uint16_t nb_rx_queues; >> >> - uint16_t i; >> >> - >> >> - nb_rx_queues = dev_info->dev->data->nb_rx_queues; >> >> - for (i = 0; i < nb_rx_queues; i++) >> >> - rxa_sw_event_vector_configure(rx_adapter, >> >eth_dev_id, i, >> >> - config); >> >> - return; >> >> - } >> >> - >> >> - queue_info = &dev_info->rx_queue[rx_queue_id]; >> >> - qi_ev = (struct rte_event *)&queue_info->event; >> >> - queue_info->ena_vector = 1; >> >> - qi_ev->event_type = >> >RTE_EVENT_TYPE_ETH_RX_ADAPTER_VECTOR; >> >> - rxa_set_vector_data(queue_info, config->vector_sz, >> >> - config->vector_timeout_ns, config- >> >>vector_mp, >> >> - rx_queue_id, dev_info->dev->data->port_id); >> >> - rx_adapter->ena_vector = 1; >> >> - rx_adapter->vector_tmo_ticks = >> >> - rx_adapter->vector_tmo_ticks ? >> >> - RTE_MIN(config->vector_timeout_ns << 1, >> >> - rx_adapter->vector_tmo_ticks) : >> >> - config->vector_timeout_ns << 1; >> >> - rx_adapter->prev_expiry_ts = 0; >> >> - TAILQ_INIT(&rx_adapter->vector_list); >> >> -} >> >> - >> >> static int rxa_sw_add(struct rte_event_eth_rx_adapter >*rx_adapter, >> >> uint16_t eth_dev_id, >> >> int rx_queue_id, >> >> @@ -2258,6 +2239,7 @@ >> >rte_event_eth_rx_adapter_queue_add(uint8_t id, >> >> struct rte_event_eth_rx_adapter *rx_adapter; >> >> struct rte_eventdev *dev; >> >> struct eth_device_info *dev_info; >> >> + struct rte_event_eth_rx_adapter_vector_limits limits; >> >> >> >> RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(id, - >> >EINVAL); >> >> RTE_ETH_VALID_PORTID_OR_ERR_RET(eth_dev_id, -EINVAL); >> >> @@ -2294,6 +2276,39 @@ >> >rte_event_eth_rx_adapter_queue_add(uint8_t id, >> >> return -EINVAL; >> >> } >> >> >> >> + if (queue_conf->rx_queue_flags & >> >> + RTE_EVENT_ETH_RX_ADAPTER_QUEUE_EVENT_VECTOR) { >> > >> >Perhaps, you could move the previous if condition here and just >check >> >for (cap & RTE_EVENT_ETH_RX_ADAPTER_CAP_EVENT_VECTOR) >> >> No, the above check is not a capability check, it is to check if >application >> requested to enable vectorization for this queue or not. > >In current code, same condition check (queue_conf->rx_queue_flags & >RTE_EVENT_ETH_RX_ADAPTER_QUEUE_EVENT_VECTOR ) is done >twice next to each other, which could be avoided. > > if ((cap & >RTE_EVENT_ETH_RX_ADAPTER_CAP_EVENT_VECTOR) == 0 && >----> (queue_conf->rx_queue_flags & > RTE_EVENT_ETH_RX_ADAPTER_QUEUE_EVENT_VECTOR)) { > RTE_EDEV_LOG_ERR("Event vectorization is not >supported," > " eth port: %" PRIu16 " adapter id: %" >PRIu8, > eth_dev_id, id); > return -EINVAL; > } > >----> if (queue_conf->rx_queue_flags & > RTE_EVENT_ETH_RX_ADAPTER_QUEUE_EVENT_VECTOR) { > ... > } > Ah, I see it now, will fix it in next version. >> >> > >> >> + ret = rte_event_eth_rx_adapter_vector_limits_get( >> >> + rx_adapter->eventdev_id, eth_dev_id, &limits); >> >> + if (ret < 0) { >> >> + RTE_EDEV_LOG_ERR("Failed to get event >> >device vector limits," >> >> + " eth port: %" PRIu16 >> >> + " adapter id: %" PRIu8, >> >> + eth_dev_id, id); >> >> + return -EINVAL; >> >> + } >> >> + if (queue_conf->vector_sz < limits.min_sz || >> >> + queue_conf->vector_sz > limits.max_sz || >> >> + queue_conf->vector_timeout_ns < >> >limits.min_timeout_ns || >> >> + queue_conf->vector_timeout_ns > >> >limits.max_timeout_ns || >> >> + queue_conf->vector_mp == NULL) { >> >> + RTE_EDEV_LOG_ERR("Invalid event vector >> >configuration," >> >> + " eth port: %" PRIu16 >> >> + " adapter id: %" PRIu8, >> >> + eth_dev_id, id); >> >> + return -EINVAL; >> >> + } >> >> + if (queue_conf->vector_mp->elt_size < >> >> + (sizeof(struct rte_event_vector) + >> >> + (sizeof(uintptr_t) * queue_conf->vector_sz))) { >> >> + RTE_EDEV_LOG_ERR("Invalid event vector >> >configuration," >> >> + " eth port: %" PRIu16 >> >> + " adapter id: %" PRIu8, >> >> + eth_dev_id, id); >> >> + return -EINVAL; >> >> + } >> >> + } >> >> + >> >> if ((cap & >> >RTE_EVENT_ETH_RX_ADAPTER_CAP_MULTI_EVENTQ) == 0 && >> >> (rx_queue_id != -1)) { >> >> RTE_EDEV_LOG_ERR("Rx queues can only be connected >> >to single " >> >> @@ -2487,83 +2502,6 @@ >> >rte_event_eth_rx_adapter_queue_del(uint8_t id, uint16_t >eth_dev_id, >> >> return ret; >> >> } >> >> >> >> -int >> >> -rte_event_eth_rx_adapter_queue_event_vector_config( >> >> - uint8_t id, uint16_t eth_dev_id, int32_t rx_queue_id, >> >> - struct rte_event_eth_rx_adapter_event_vector_config *config) >> >> -{ >> >> - struct rte_event_eth_rx_adapter_vector_limits limits; >> >> - struct rte_event_eth_rx_adapter *rx_adapter; >> >> - struct rte_eventdev *dev; >> >> - uint32_t cap; >> >> - int ret; >> >> - >> >> - 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 = rxa_id_to_adapter(id); >> >> - if ((rx_adapter == NULL) || (config == NULL)) >> >> - return -EINVAL; >> >> - >> >> - dev = &rte_eventdevs[rx_adapter->eventdev_id]; >> >> - ret = rte_event_eth_rx_adapter_caps_get(rx_adapter- >> >>eventdev_id, >> >> - eth_dev_id, &cap); >> >> - if (ret) { >> >> - RTE_EDEV_LOG_ERR("Failed to get adapter caps edev >> >%" PRIu8 >> >> - "eth port %" PRIu16, >> >> - id, eth_dev_id); >> >> - return ret; >> >> - } >> >> - >> >> - if (!(cap & >> >RTE_EVENT_ETH_RX_ADAPTER_CAP_EVENT_VECTOR)) { >> >> - RTE_EDEV_LOG_ERR("Event vectorization is not >> >supported," >> >> - " eth port: %" PRIu16 " adapter id: %" >> >PRIu8, >> >> - eth_dev_id, id); >> >> - return -EINVAL; >> >> - } >> >> - >> >> - ret = rte_event_eth_rx_adapter_vector_limits_get( >> >> - rx_adapter->eventdev_id, eth_dev_id, &limits); >> >> - if (ret) { >> >> - RTE_EDEV_LOG_ERR("Failed to get vector limits edev >> >%" PRIu8 >> >> - "eth port %" PRIu16, >> >> - rx_adapter->eventdev_id, eth_dev_id); >> >> - return ret; >> >> - } >> >> - >> >> - if (config->vector_sz < limits.min_sz || >> >> - config->vector_sz > limits.max_sz || >> >> - config->vector_timeout_ns < limits.min_timeout_ns || >> >> - config->vector_timeout_ns > limits.max_timeout_ns || >> >> - config->vector_mp == NULL) { >> >> - RTE_EDEV_LOG_ERR("Invalid event vector >> >configuration," >> >> - " eth port: %" PRIu16 " adapter id: %" >> >PRIu8, >> >> - eth_dev_id, id); >> >> - return -EINVAL; >> >> - } >> >> - if (config->vector_mp->elt_size < >> >> - (sizeof(struct rte_event_vector) + >> >> - (sizeof(uintptr_t) * config->vector_sz))) { >> >> - RTE_EDEV_LOG_ERR("Invalid event vector >> >configuration," >> >> - " eth port: %" PRIu16 " adapter id: %" >> >PRIu8, >> >> - eth_dev_id, id); >> >> - return -EINVAL; >> >> - } >> >> - >> >> - if (cap & >> >RTE_EVENT_ETH_RX_ADAPTER_CAP_INTERNAL_PORT) { >> >> - RTE_FUNC_PTR_OR_ERR_RET( >> >> - *dev->dev_ops- >> >>eth_rx_adapter_event_vector_config, >> >> - -ENOTSUP); >> >> - ret = dev->dev_ops- >> >>eth_rx_adapter_event_vector_config( >> >> - dev, &rte_eth_devices[eth_dev_id], >> >rx_queue_id, config); >> >> - } else { >> >> - rxa_sw_event_vector_configure(rx_adapter, >> >eth_dev_id, >> >> - rx_queue_id, config); >> >> - } >> >> - >> >> - return ret; >> >> -} >> >> - >> >> int >> >> rte_event_eth_rx_adapter_vector_limits_get( >> >> uint8_t dev_id, uint16_t eth_port_id, >> >> diff --git a/lib/librte_eventdev/rte_event_eth_rx_adapter.h >> >b/lib/librte_eventdev/rte_event_eth_rx_adapter.h >> >> index 7407cde00..3f8b36229 100644 >> >> --- a/lib/librte_eventdev/rte_event_eth_rx_adapter.h >> >> +++ b/lib/librte_eventdev/rte_event_eth_rx_adapter.h >> >> @@ -171,9 +171,6 @@ struct >rte_event_eth_rx_adapter_queue_conf >> >{ >> >> * The event adapter sets ev.event_type to >> >RTE_EVENT_TYPE_ETHDEV in the >> >> * enqueued event. >> >> */ >> >> -}; >> >> - >> >> -struct rte_event_eth_rx_adapter_event_vector_config { >> >> uint16_t vector_sz; >> >> /**< >> >> * Indicates the maximum number for mbufs to combine and >> >form a vector. >> >> @@ -548,30 +545,6 @@ int >> >rte_event_eth_rx_adapter_vector_limits_get( >> >> uint8_t dev_id, uint16_t eth_port_id, >> >> struct rte_event_eth_rx_adapter_vector_limits *limits); >> >> >> >> -/** >> >> - * Configure event vectorization for a given ethernet device >queue, >> >that has >> >> - * been added to a event eth Rx adapter. >> >> - * >> >> - * @param id >> >> - * The identifier of the ethernet Rx event adapter. >> >> - * >> >> - * @param eth_dev_id >> >> - * The identifier of the ethernet device. >> >> - * >> >> - * @param rx_queue_id >> >> - * Ethernet device receive queue index. >> >> - * If rx_queue_id is -1, then all Rx queues configured for the >ethernet >> >device >> >> - * are configured with event vectorization. >> >> - * >> >> - * @return >> >> - * - 0: Success, Receive queue configured correctly. >> >> - * - <0: Error code on failure. >> >> - */ >> >> -__rte_experimental >> >> -int rte_event_eth_rx_adapter_queue_event_vector_config( >> >> - uint8_t id, uint16_t eth_dev_id, int32_t rx_queue_id, >> >> - struct rte_event_eth_rx_adapter_event_vector_config >> >*config); >> >> - >> >> #ifdef __cplusplus >> >> } >> >> #endif >> >> diff --git a/lib/librte_eventdev/version.map >> >b/lib/librte_eventdev/version.map >> >> index 902df0ae3..34c1c830e 100644 >> >> --- a/lib/librte_eventdev/version.map >> >> +++ b/lib/librte_eventdev/version.map >> >> @@ -142,7 +142,6 @@ EXPERIMENTAL { >> >> #added in 21.05 >> >> rte_event_vector_pool_create; >> >> rte_event_eth_rx_adapter_vector_limits_get; >> >> - rte_event_eth_rx_adapter_queue_event_vector_config; >> >> }; >> >> >> >> INTERNAL { >> >> -- >> >> 2.17.1