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

["Morten Brørup" <mb@smartsharesystems.com>]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> Konstantin
> 
> >
> > > 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.

In the case of RTE_ETHDEV_RXTX_CALLBACKS, it is independent:

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.