RE: [PATCH v3 1/3] ethdev: enable direct rearm with separate API

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

> From: Feifei Wang [mailto:Feifei.Wang2@arm.com]
> Sent: Wednesday, 4 January 2023 09.51
> 
> Hi, Morten
> 
> > 发件人: Morten Brørup <mb@smartsharesystems.com>
> > 发送时间: Wednesday, January 4, 2023 4:22 PM
> >
> > > From: Feifei Wang [mailto:feifei.wang2@arm.com]
> > > Sent: Wednesday, 4 January 2023 08.31
> > >
> > > Add 'tx_fill_sw_ring' and 'rx_flush_descriptor' API into direct
> rearm
> > > mode for separate Rx and Tx Operation. And this can support
> different
> > > multiple sources in direct rearm mode. For examples, Rx driver is
> > > ixgbe, and Tx driver is i40e.
> > >
> > > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Suggested-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > ---
> >
> > This feature looks very promising for performance. I am pleased to
> see
> > progress on it.
> >
> Thanks very much for your reviewing.
> 
> > Please confirm that the fast path functions are still thread safe,
> i.e. one EAL
> > thread may be calling rte_eth_rx_burst() while another EAL thread is
> calling
> > rte_eth_tx_burst().
> >
> For the multiple threads safe, like we say in cover letter, current
> direct-rearm support
> Rx and Tx in the same thread. If we consider multiple threads like
> 'pipeline model', there
> need to add 'lock' in the data path which can decrease the performance.
> Thus, the first step we do is try to enable direct-rearm in the single
> thread, and then we will consider
> to enable direct rearm in multiple threads and improve the performance.

OK, doing it in steps is a good idea for a feature like this - makes it easier to understand and review.

When proceeding to add support for the "pipeline model", perhaps the lockless principles from the rte_ring can be used in this feature too.

From a high level perspective, I'm somewhat worried that releasing a "work-in-progress" version of this feature in some DPDK version will cause API/ABI breakage discussions when progressing to the next steps of the implementation to make the feature more complete. Not only support for thread safety across simultaneous RX and TX, but also support for multiple mbuf pools per RX queue [1]. Marking the functions experimental should alleviate such discussions, but there is a risk of pushback to not break the API/ABI anyway.

[1]: https://elixir.bootlin.com/dpdk/v22.11.1/source/lib/ethdev/rte_ethdev.h#L1105

[...]

> > > --- a/lib/ethdev/ethdev_driver.h
> > > +++ b/lib/ethdev/ethdev_driver.h
> > > @@ -59,6 +59,10 @@ struct rte_eth_dev {
> > >  	eth_rx_descriptor_status_t rx_descriptor_status;
> > >  	/** Check the status of a Tx descriptor */
> > >  	eth_tx_descriptor_status_t tx_descriptor_status;
> > > +	/** Fill Rx sw-ring with Tx buffers in direct rearm mode */
> > > +	eth_tx_fill_sw_ring_t tx_fill_sw_ring;
> >
> > What is "Rx sw-ring"? Please confirm that this is not an Intel PMD
> specific
> > term and/or implementation detail, e.g. by providing a conceptual
> > implementation for a non-Intel PMD, e.g. mlx5.
> Rx sw_ring is used  to store mbufs in intel PMD. This is the same as
> 'rxq->elts'
> in mlx5. 

Sounds good.

Then all we need is consensus on a generic name for this, unless "Rx sw-ring" already is the generic name. (I'm not a PMD developer, so I might be completely off track here.) Naming is often debatable, so I'll stop talking about it now - I only wanted to highlight that we should avoid vendor-specific terms in public APIs intended to be implemented by multiple vendors. On the other hand... if no other vendors raise their voices before merging into the DPDK main repository, they forfeit their right to complain about it. ;-)

> Agree with that we need to providing a conceptual
> implementation for all PMDs.

My main point is that we should ensure that the feature is not too tightly coupled with the way Intel PMDs implement mbuf handling. Providing a conceptual implementation for a non-Intel PMD is one way of checking this.

The actual implementation in other PMDs could be left up to the various NIC vendors.