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

[Feifei Wang]

Hi, Morten

> -----邮件原件-----
> 发件人: Morten Brørup <mb at smartsharesystems.com>
> 发送时间: Wednesday, January 4, 2023 4:22 PM
> 收件人: Feifei Wang <Feifei.Wang2 at arm.com>; thomas at monjalon.net;
> Ferruh Yigit <ferruh.yigit at amd.com>; Andrew Rybchenko
> <andrew.rybchenko at oktetlabs.ru>
> 抄送: dev at dpdk.org; konstantin.v.ananyev at yandex.ru; nd <nd at arm.com>;
> Honnappa Nagarahalli <Honnappa.Nagarahalli at arm.com>; Ruifeng Wang
> <Ruifeng.Wang at arm.com>
> 主题: RE: [PATCH v3 1/3] ethdev: enable direct rearm with separate API
> 
> > From: Feifei Wang [mailto:feifei.wang2 at 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 at arm.com>
> > Suggested-by: Ruifeng Wang <ruifeng.wang at arm.com>
> > Signed-off-by: Feifei Wang <feifei.wang2 at arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang at arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli at 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. 

> A few more comments below.
> 
> >  lib/ethdev/ethdev_driver.h   |  10 ++
> >  lib/ethdev/ethdev_private.c  |   2 +
> >  lib/ethdev/rte_ethdev.c      |  52 +++++++++++
> >  lib/ethdev/rte_ethdev.h      | 174
> +++++++++++++++++++++++++++++++++++
> >  lib/ethdev/rte_ethdev_core.h |  11 +++
> >  lib/ethdev/version.map       |   6 ++
> >  6 files changed, 255 insertions(+)
> >
> > diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> > index 6a550cfc83..bc539ec862 100644
> > --- 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. Agree with that we need to providing a conceptual implementation for
all PMDs.
> 
> Please note: I do not request the ability to rearm between drivers from
> different vendors, I only request that the public ethdev API uses generic
> terms and concepts, so any NIC vendor can implement the direct-rearm
> functions in their PMDs.
Agree with this. This is also our consideration. 
In the future plan, we plan to implement this function in different PMDs.

> 
> > +	/** Flush Rx descriptor in direct rearm mode */
> > +	eth_rx_flush_descriptor_t rx_flush_descriptor;
> 
> descriptor -> descriptors. There are more than one. Both in comment and
> function name.
Agree.
> 
> [...]
> 
> > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index
> > c129ca1eaf..381c3d535f 100644
> > --- a/lib/ethdev/rte_ethdev.h
> > +++ b/lib/ethdev/rte_ethdev.h
> > @@ -1818,6 +1818,17 @@ struct rte_eth_txq_info {
> >  	uint8_t queue_state;        /**< one of RTE_ETH_QUEUE_STATE_*. */
> >  } __rte_cache_min_aligned;
> >
> > +/**
> > + * @internal
> > + * Structure used to hold pointers to internal ethdev Rx rearm data.
> > + * The main purpose is to load Rx queue rearm data in direct rearm
> > mode.
> > + */
> > +struct rte_eth_rxq_rearm_data {
> > +	void *rx_sw_ring;
> > +	uint16_t *rearm_start;
> > +	uint16_t *rearm_nb;
> > +} __rte_cache_min_aligned;
> 
> Please add descriptions to the fields in this structure.
Agree.