> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Ananyev, > Konstantin > > > > > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Ananyev, > > > Konstantin > > > > > > > > > > > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Ananyev, > > > > > Konstantin > > > > > > > > > > > > > > > > > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of > Ananyev, > > > > > > > Konstantin > > > > > > > > > > > > > > > > How can we hide the callbacks since they are used by > inline > > > > > burst > > > > > > > functions. > > > > > > > > > > > > > > > > I probably I owe a better explanation to what I meant in > > > first > > > > > mail. > > > > > > > > Otherwise it sounds confusing. > > > > > > > > I'll try to write a more detailed one in next few days. > > > > > > > > > > > > > > Actually I gave it another thought over weekend, and might > be > > > we > > > > > can > > > > > > > hide rte_eth_dev_cb even in a simpler way. I'd use > > > eth_rx_burst() > > > > > as > > > > > > > an example, but the same principle applies to other 'fast' > > > > > functions. > > > > > > > > > > > > > > 1. Needed changes for PMDs rx_pkt_burst(): > > > > > > > a) change function prototype to accept 'uint16_t > port_id' > > > and > > > > > > > 'uint16_t queue_id', > > > > > > > instead of current 'void *'. > > > > > > > b) Each PMD rx_pkt_burst() will have to call > > > > > rte_eth_rx_epilog() > > > > > > > function at return. > > > > > > > This inline function will do all CB calls for > that > > > queue. > > > > > > > > > > > > > > To be more specific, let say we have some PMD: xyz with RX > > > > > function: > > > > > > > > > > > > > > uint16_t > > > > > > > xyz_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, > > > uint16_t > > > > > > > nb_pkts) > > > > > > > { > > > > > > > struct xyz_rx_queue *rxq = rx_queue; > > > > > > > uint16_t nb_rx = 0; > > > > > > > > > > > > > > /* do actual stuff here */ > > > > > > > .... > > > > > > > return nb_rx; > > > > > > > } > > > > > > > > > > > > > > It will be transformed to: > > > > > > > > > > > > > > uint16_t > > > > > > > xyz_recv_pkts(uint16_t port_id, uint16_t queue_id, struct > > > rte_mbuf > > > > > > > **rx_pkts, uint16_t nb_pkts) > > > > > > > { > > > > > > > struct xyz_rx_queue *rxq; > > > > > > > uint16_t nb_rx; > > > > > > > > > > > > > > rxq = _rte_eth_rx_prolog(port_id, queue_id); > > > > > > > if (rxq == NULL) > > > > > > > return 0; > > > > > > > nb_rx = _xyz_real_recv_pkts(rxq, rx_pkts, > nb_pkts); > > > > > > > return _rte_eth_rx_epilog(port_id, queue_id, > rx_pkts, > > > > > > > nb_pkts); > > > > > > > } > > > > > > > > > > > > > > And somewhere in ethdev_private.h: > > > > > > > > > > > > > > static inline void * > > > > > > > _rte_eth_rx_prolog(uint16_t port_id, uint16_t queue_id); > > > > > > > { > > > > > > > struct rte_eth_dev *dev = &rte_eth_devices[port_id]; > > > > > > > > > > > > > > #ifdef RTE_ETHDEV_DEBUG_RX > > > > > > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, NULL); > > > > > > > RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, NULL); > > > > > > > > > > > > > > if (queue_id >= dev->data->nb_rx_queues) { > > > > > > > RTE_ETHDEV_LOG(ERR, "Invalid RX > queue_id=%u\n", > > > > > > > queue_id); > > > > > > > return NULL; > > > > > > > } > > > > > > > #endif > > > > > > > return dev->data->rx_queues[queue_id]; > > > > > > > } > > > > > > > > > > > > > > static inline uint16_t > > > > > > > _rte_eth_rx_epilog(uint16_t port_id, uint16_t queue_id, > struct > > > > > rte_mbuf > > > > > > > **rx_pkts, const uint16_t nb_pkts); > > > > > > > { > > > > > > > struct rte_eth_dev *dev = &rte_eth_devices[port_id]; > > > > > > > > > > > > > > #ifdef RTE_ETHDEV_RXTX_CALLBACKS > > > > > > > struct rte_eth_rxtx_callback *cb; > > > > > > > > > > > > > > /* __ATOMIC_RELEASE memory order was used when the > > > > > > > * call back was inserted into the list. > > > > > > > * Since there is a clear dependency between > loading > > > > > > > * cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory > > > order is > > > > > > > * not required. > > > > > > > */ > > > > > > > cb = __atomic_load_n(&dev- > >post_rx_burst_cbs[queue_id], > > > > > > > __ATOMIC_RELAXED); > > > > > > > > > > > > > > if (unlikely(cb != NULL)) { > > > > > > > do { > > > > > > > nb_rx = cb->fn.rx(port_id, > queue_id, > > > > > rx_pkts, > > > > > > > nb_rx, > > > > > > > nb_pkts, > cb- > > > > > >param); > > > > > > > cb = cb->next; > > > > > > > } while (cb != NULL); > > > > > > > } > > > > > > > #endif > > > > > > > > > > > > > > rte_ethdev_trace_rx_burst(port_id, queue_id, (void > > > > > **)rx_pkts, > > > > > > > nb_rx); > > > > > > > return nb_rx; > > > > > > > } > > > > > > > > > > > > That would make the compiler inline _rte_eth_rx_epilog() into > the > > > > > driver when compiling the DPDK library. But > > > > > > RTE_ETHDEV_RXTX_CALLBACKS is a definition for the application > > > > > developer to use when compiling the DPDK application. > > > > > > > > > > I believe it is for both - user app and DPDK drivers. > > > > > AFAIK, they both have to use the same rte_config.h, otherwise > > > things > > > > > will be broken. > > > > > If let say RTE_ETHDEV_RXTX_CALLBACKS is not enabled in ethdev, > then > > > > > user wouldn't be able to add a callback at first place. > > > > > > > > In the case of RTE_ETHDEV_RXTX_CALLBACKS, it is independent: > > > > > > Not really. > > > There are few libraries within DPDK that do rely on rx/tx > callbacks: > > > bpf, latencystat, pdump, power. > > > > I do not consider these to be core libraries in DPDK. If these > libraries are used in an application, that application must be compiled > with > > RTE_ETHDEV_RXTX_CALLBACKS. > > > > > With the approach above their functionality will be broken - > > > setup functions will return success, but actual callbacks will not > be > > > invoked. > > > > I just took a look at bpf and latencystat. Bpf correctly checks for > the return code, and returns an error if ethdev has been compiled > without > > RTE_ETHDEV_RXTX_CALLBACKS. Latencystat checks for the return code, > but only logs the error and continues as if everything is good > > anyway. That is a bug in the latencystat library. > > If RTE_ETHDEV_RXTX_CALLBACKS Is enabled or disabled for both DPDK and > user app - everything will work as expected. > But, as I understand, you consider approach when > RTE_ETHDEV_RXTX_CALLBACKS Is enabled in the DPDK, but disabled in the > app. > Such approach will cause a problems with some libraries - as I > outlined above. > > > > > > From other side, some libraries do invoke rx/tx burst on their own: > ip- > > > pipeline, graph. > > > For them callback invocation will continue to work, even when > > > RTE_ETHDEV_RXTX_CALLBACKS is disabled in the app. > > > In general, building DPDK libs and user app with different > rte_config.h > > > is really a bad idea. > > > It might work in some cases, but I believe it is not supported and > user > > > should not rely on it. > > > If user needs to disable RTE_ETHDEV_RXTX_CALLBACKS in his app, then > the > > > proper way would be: > > > - update rte_config.h > > > - rebuild both DPDK and the app with new config > > > > In principle, I completely agree with your reasoning from a high > level perspective. > > > > However, accepting it would probably lead to the > RTE_ETHDEV_RXTX_CALLBACKS compile time configuration option being > completely > > removed, > > For now, we are not talking about removing or even deprecating > RTE_ETHDEV_RXTX_CALLBACKS. > What I am talking about - user has to use it (and other rte_config.h > options) properly. > He can't use different configs for DPDK libs and app and expect things > 'just work'. > This is not supported right now, I think it will never be. > If it works right now, this is just implementation specifics, which > user should not rely on.
I agree. > > > and ethdev callbacks being always supported. And I don't think such a > performance degradation of a core DPDK library should be > > accepted. > > As I said above, RTE_ETHDEV_RXTX_CALLBACKS Is still there. > If it is really critical for your app to disable it - you can still do > it, you just need to do it in a proper way. I hope so. This is exactly what I am pleading for: Keep the ability to disable RTE_ETHDEV_RXTX_CALLBACKS at compile time, so there is no performance impact for applications not using it. I also agree with the limitation that both library and application should be compiled with the same configuration. > > > <rant on> > > I was opposed to the "callback hooks" concept from the beginning, and > still am. The path that packets take through various functions and > > pipeline stages should be determined and implemented by the > application, not by the DPDK libraries. > > > > If we want to provide a standardized advanced IP pipeline for DPDK, > we could offer it as a middle layer library using the underlying DPDK > > libraries to implement various callbacks, IP fragmentation > reassembly, etc.. Don't tweak the core libraries (costing memory and/or > > performance) to support an increasing amount of supplemental > libraries, which may not be used by all applications. > > > > We don't want DPDK to become like the Linux IP stack, with callback > hooks and runtime installable protocol handling everywhere. All this > > cruft with their small performance degradations adds up! > > <rant off> > > > > > > > > > > > > > If it is not compiled with the DPDK library, attempts to install > > > callbacks from the application will fail with ENOTSUP. > > > > > > > > If it is not compiled with the DPDK application, no time will be > > > spent trying to determine if any there are any callbacks to call. > > > > > > > > > BTW, such change will allow us to make > RTE_ETHDEV_RXTX_CALLBACKS > > > > > internal for ethdev/PMD layer, which is a good thing from my > > > > > perspective. > > > > > > > > If it can be done without degrading performance for applications > not > > > using callbacks.
