Re: [dpdk-dev] [PATCH] parray: introduce internal API for dynamic arrays

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> 
> On 6/21/2021 3:38 PM, Ananyev, Konstantin wrote:
> >
> >>
> >> On 6/21/2021 1:30 PM, Ananyev, Konstantin wrote:
> >>>
> >>>>
> >>>>> From: dev [mailto:dev-bounces@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.
> >>> 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.
> >>>
> >>
> >> It is possible to use binary drivers (.so) as plugin. Currently application can
> >> decide to use or not use Rx/Tx callbacks even with binary drivers, but this
> >> change adds a complexity to this usecase.
> >
> > Not sure I understand you here...
> > Can you explain a bit more what do you mean?
> >
> 
> Right now if I have a .so driver, I can decide to use or not to use the Rx/Tx
> callbacks by compiling application with relevant config, and .so will work for
> both without change.

True.

> With proposed change, if .so not enabled Rx/Tx callback, application won't able
> to use it.
> 
> Application and driver config should be compatible, and adding more compile time
> config to drivers that is also used in libraries is adding more points to sync,
> hence adding more complexity I believe to binary drivers usecase.

I agree - right now PMD doesn't use RTE_ETHDEV_RXTX_CALLBACKS,
and with that proposal we add extra config dependency to the PMD.
Also it makes PMD even more tightly coupled with rte_ethdev.
Though is that really an obstacle?
From my understanding dpdk libs and PMDs has to be build with the same config anyway.
Not following this rule can cause all sorts of troubles.

> >>
> >>>>
> >>>>>
> >>>>> Now, as you said above, in rte_ethdev.h we will keep only a flat array
> >>>>> with pointers to 'fast' functions:
> >>>>> struct {
> >>>>>      eth_rx_burst_t             rx_pkt_burst
> >>>>>       eth_tx_burst_t             tx_pkt_burst;
> >>>>>       eth_tx_prep_t              tx_pkt_prepare;
> >>>>>      .....
> >>>>> } rte_eth_dev_burst[];
> >>>>>
> >>>>> And rte_eth_rx_burst() will look like:
> >>>>>
> >>>>> static inline uint16_t
> >>>>> rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> >>>>>                  struct rte_mbuf **rx_pkts, const uint16_t nb_pkts)
> >>>>> {
> >>>>>     if (port_id >= RTE_MAX_ETHPORTS)
> >>>>>         return 0;
> >>>>>    return rte_eth_dev_burst[port_id](port_id, queue_id, rx_pkts,
> >>>>> nb_pkts);
> >>>>> }
> >>>>>
> >>>>> Yes, it will require changes in *all* PMDs, but as I said before the
> >>>>> changes will be a mechanic ones.
> >