On Wed, Sep 22, 2021 at 8:38 PM Ananyev, Konstantin
<konstantin.anan...@intel.com> wrote:
>
>
> > > Hi Jerin,
> > >
> > > > > NOTE: This is just an RFC to start further discussion and collect the
> > > > > feedback.
> > > > > Due to significant amount of work, changes required are applied only
> > > > > to two
> > > > > PMDs so far: net/i40e and net/ice.
> > > > > So to build it you'll need to add:
> > > > > -Denable_drivers='common/*,mempool/*,net/ice,net/i40e'
> > > > > to your config options.
> > > >
> > > > >
> > > > > That approach was selected to avoid(/minimize) possible performance
> > > > > losses.
> > > > >
> > > > > So far I done only limited amount functional and performance testing.
> > > > > Didn't spot any functional problems, and performance numbers
> > > > > remains the same before and after the patch on my box (testpmd,
> > > > > macswap fwd).
> > > >
> > > >
> > > > Based on testing on octeonxt2. We see some regression in testpmd and
> > > > bit on l3fwd too.
> > > >
> > > > Without patch: 73.5mpps/core in testpmd iofwd
> > > > With out patch: 72 5mpps/core in testpmd iofwd
> > > >
> > > > Based on my understanding it is due to additional indirection.
> > >
> > > From your patch below, it looks like not actually additional indirection,
> > > but extra memory dereference - func and dev pointers are now stored
> > > at different places.
> >
> > Yup. I meant the same. We are on the same page.
> >
> > > Plus the fact that now we dereference rte_eth_devices[]
> > > data inside PMD function. Which probably prevents compiler and CPU to load
> > > rte_eth_devices[port_id].data and rte_eth_devices[port_id].
> > > pre_tx_burst_cbs[queue_id]
> > > in advance before calling actual RX/TX function.
> >
> > Yes.
> >
> > > About your approach: I don’t mind to add extra opaque 'void *data'
> > > pointer,
> > > but would prefer not to expose callback invocations code into inline
> > > function.
> > > Main reason for that - I think it still need to be reworked to allow
> > > adding/removing
> > > callbacks without stopping the device. Something similar to what was done
> > > for cryptodev
> > > callbacks. To be able to do that in future without another ABI breakage
> > > callbacks related part
> > > needs to be kept internal.
> > > Though what we probably can do: add two dynamic arrays of opaque pointers
> > > to rte_eth_burst_api.
> > > One for rx/tx queue data pointers, second for rx/tx callback pointers.
> > > To be more specific, something like:
> > >
> > > typedef uint16_t (*rte_eth_rx_burst_t)( void *rxq, struct rte_mbuf
> > > **rx_pkts, uint16_t nb_pkts, void *cbs);
> > > typedef uint16_t (*rte_eth_tx_burst_t)(void *txq, struct rte_mbuf
> > > **tx_pkts, uint16_t nb_pkts, void *cbs);
> > > ....
> > >
> > > struct rte_eth_burst_api {
> > > rte_eth_rx_burst_t rx_pkt_burst;
> > > /**< PMD receive function. */
> > > rte_eth_tx_burst_t tx_pkt_burst;
> > > /**< PMD transmit function. */
> > > rte_eth_tx_prep_t tx_pkt_prepare;
> > > /**< PMD transmit prepare function. */
> > > rte_eth_rx_queue_count_t rx_queue_count;
> > > /**< Get the number of used RX descriptors. */
> > > rte_eth_rx_descriptor_status_t rx_descriptor_status;
> > > /**< Check the status of a Rx descriptor. */
> > > rte_eth_tx_descriptor_status_t tx_descriptor_status;
> > > /**< Check the status of a Tx descriptor. */
> > > struct {
> > > void **queue_data; /* point to
> > > rte_eth_devices[port_id].data-> rx_queues */
> > > void **cbs; /* points to
> > > rte_eth_devices[port_id].post_rx_burst_cbs */
> > > } rx_data, tx_data;
> > > } __rte_cache_aligned;
> > >
> > > 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)
> > > {
> > > struct rte_eth_burst_api *p;
> > >
> > > if (port_id >= RTE_MAX_ETHPORTS || queue_id >=
> > > RTE_MAX_QUEUES_PER_PORT)
> > > return 0;
> > >
> > > p = &rte_eth_burst_api[port_id];
> > > return p->rx_pkt_burst(p->rx_data.queue_data[queue_id], rx_pkts,
> > > nb_pkts, p->rx_data.cbs[queue_id]);
> >
> >
> >
> > That works.
> >
> >
> > > }
> > >
> > > Same for TX.
> > >
> > > If that looks ok to everyone, I'll try to prepare next version based on
> > > that.
> >
> >
> > Looks good to me.
> >
> > > In theory that should avoid extra dereference problem and even reduce
> > > indirection.
> > > As a drawback data->rxq/txq should always be allocated for
> > > RTE_MAX_QUEUES_PER_PORT entries,
> > > but I presume that’s not a big deal.
> > >
> > > As a side question - is there any reason why rte_ethdev_trace_rx_burst()
> > > is invoked at very last point,
> > > while rte_ethdev_trace_tx_burst() after CBs but before actual
> > > tx_pkt_burst()?
> > > It would make things simpler if tracng would always be done either on
> > > entrance or exit of rx/tx_burst.
> >
> > exit is fine.
> >
> > >
> > > >
> > > > My suggestion to fix the problem by:
> > > > Removing the additional `data` redirection and pull callback function
> > > > pointers back
> > > > and keep rest as opaque as done in the existing patch like [1]
> > > >
> > > > I don't believe this has any real implication on future ABI stability
> > > > as we will not be adding
> > > > any new item in rte_eth_fp in any way as new features can be added in
> > > > slowpath
> > > > rte_eth_dev as mentioned in the patch.
> >
> > Ack
> >
> > I will happy to test again after the rework and report any performance
> > issues if any.
>
> Thanks Jerin, v2 is out:
> https://patches.dpdk.org/project/dpdk/list/?series=19084
> Please have a look, when you'll get a chance.
Tested the series. Looks good, No performance issue.
>
