Hi, Konstantin
Thanks for your reviewing and sorry for my delayed response.
For your comments, we put forward several improvement plans below.
Best Regards
Feifei
> -----邮件原件-----
> 发件人: Konstantin Ananyev <konstantin.v.anan...@yandex.ru>
> 发送时间: Thursday, February 2, 2023 10:33 PM
> 收件人: Feifei Wang <feifei.wa...@arm.com>; tho...@monjalon.net;
> Ferruh Yigit <ferruh.yi...@amd.com>; Andrew Rybchenko
> <andrew.rybche...@oktetlabs.ru>
> 抄送: dev@dpdk.org; nd <n...@arm.com>; Honnappa Nagarahalli
> <honnappa.nagaraha...@arm.com>; Ruifeng Wang
> <ruifeng.w...@arm.com>
> 主题: Re: [PATCH v3 1/3] ethdev: enable direct rearm with separate API
>
> Hi Feifei,
>
> > 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.
>
>
> Thanks for your effort and thanks for taking comments provided into
> consideration.
> That approach looks much better then previous ones.
> Few nits below.
> Konstantin
>
> > Suggested-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> > Suggested-by: Ruifeng Wang <ruifeng.w...@arm.com>
> > Signed-off-by: Feifei Wang <feifei.wa...@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.w...@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> > ---
> > 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;
> > + /** Flush Rx descriptor in direct rearm mode */
> > + eth_rx_flush_descriptor_t rx_flush_descriptor;
> >
> > /**
> > * Device data that is shared between primary and secondary
> > processes @@ -504,6 +508,10 @@ typedef void
> (*eth_rxq_info_get_t)(struct rte_eth_dev *dev,
> > typedef void (*eth_txq_info_get_t)(struct rte_eth_dev *dev,
> > uint16_t tx_queue_id, struct rte_eth_txq_info *qinfo);
> >
> > +/**< @internal Get rearm data for a receive queue of an Ethernet
> > +device. */ typedef void (*eth_rxq_rearm_data_get_t)(struct rte_eth_dev
> *dev,
> > + uint16_t tx_queue_id, struct rte_eth_rxq_rearm_data
> > +*rxq_rearm_data);
> > +
> > typedef int (*eth_burst_mode_get_t)(struct rte_eth_dev *dev,
> > uint16_t queue_id, struct rte_eth_burst_mode *mode);
> >
> > @@ -1215,6 +1223,8 @@ struct eth_dev_ops {
> > eth_rxq_info_get_t rxq_info_get;
> > /** Retrieve Tx queue information */
> > eth_txq_info_get_t txq_info_get;
> > + /** Get Rx queue rearm data */
> > + eth_rxq_rearm_data_get_t rxq_rearm_data_get;
> > eth_burst_mode_get_t rx_burst_mode_get; /**< Get Rx burst
> mode */
> > eth_burst_mode_get_t tx_burst_mode_get; /**< Get Tx burst
> mode */
> > eth_fw_version_get_t fw_version_get; /**< Get firmware version
> */
> > diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
> > index 48090c879a..c5dd5e30f6 100644
> > --- a/lib/ethdev/ethdev_private.c
> > +++ b/lib/ethdev/ethdev_private.c
> > @@ -276,6 +276,8 @@ eth_dev_fp_ops_setup(struct rte_eth_fp_ops *fpo,
> > fpo->rx_queue_count = dev->rx_queue_count;
> > fpo->rx_descriptor_status = dev->rx_descriptor_status;
> > fpo->tx_descriptor_status = dev->tx_descriptor_status;
> > + fpo->tx_fill_sw_ring = dev->tx_fill_sw_ring;
> > + fpo->rx_flush_descriptor = dev->rx_flush_descriptor;
> >
> > fpo->rxq.data = dev->data->rx_queues;
> > fpo->rxq.clbk = (void **)(uintptr_t)dev->post_rx_burst_cbs;
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index
> > 5d5e18db1e..2af5cb42fe 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -3282,6 +3282,21 @@
> rte_eth_dev_set_rx_queue_stats_mapping(uint16_t port_id, uint16_t
> rx_queue_id,
> > stat_idx, STAT_QMAP_RX));
> > }
> >
> > +int
> > +rte_eth_dev_direct_rearm(uint16_t rx_port_id, uint16_t rx_queue_id,
> > + uint16_t tx_port_id, uint16_t tx_rx_queue_id,
> > + struct rte_eth_rxq_rearm_data *rxq_rearm_data) {
> > + int nb_rearm = 0;
> > +
> > + nb_rearm = rte_eth_tx_fill_sw_ring(tx_port_id, tx_rx_queue_id,
> > +rxq_rearm_data);
> > +
> > + if (nb_rearm > 0)
> > + return rte_eth_rx_flush_descriptor(rx_port_id, rx_queue_id,
> > +nb_rearm);
> > +
> > + return 0;
> > +}
> > +
> > int
> > rte_eth_dev_fw_version_get(uint16_t port_id, char *fw_version, size_t
> fw_size)
> > {
> > @@ -5323,6 +5338,43 @@ rte_eth_tx_queue_info_get(uint16_t port_id,
> uint16_t queue_id,
> > return 0;
> > }
> >
> > +int
> > +rte_eth_rx_queue_rearm_data_get(uint16_t port_id, uint16_t queue_id,
> > + struct rte_eth_rxq_rearm_data *rxq_rearm_data) {
> > + struct rte_eth_dev *dev;
> > +
> > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > + dev = &rte_eth_devices[port_id];
> > +
> > + if (queue_id >= dev->data->nb_rx_queues) {
> > + RTE_ETHDEV_LOG(ERR, "Invalid Rx queue_id=%u\n",
> queue_id);
> > + return -EINVAL;
> > + }
> > +
> > + if (rxq_rearm_data == NULL) {
> > + RTE_ETHDEV_LOG(ERR, "Cannot get ethdev port %u Rx
> queue %u rearm data to NULL\n",
> > + port_id, queue_id);
> > + return -EINVAL;
> > + }
> > +
> > + if (dev->data->rx_queues == NULL ||
> > + dev->data->rx_queues[queue_id] == NULL) {
> > + RTE_ETHDEV_LOG(ERR,
> > + "Rx queue %"PRIu16" of device with port_id=%"
> > + PRIu16" has not been setup\n",
> > + queue_id, port_id);
> > + return -EINVAL;
> > + }
> > +
> > + if (*dev->dev_ops->rxq_rearm_data_get == NULL)
> > + return -ENOTSUP;
> > +
> > + dev->dev_ops->rxq_rearm_data_get(dev, queue_id,
> rxq_rearm_data);
> > +
> > + return 0;
> > +}
> > +
> > int
> > rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
> > struct rte_eth_burst_mode *mode) 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.
> > + */
>
> I think this structure owes a lot more expalantion.
> What each fields suppose to do and what are the constraints, etc.
>
> In general, more doc will be needed to explain that feature.
You are right. We will add more explanation for it and also update the doc
To explain the feature.
> > +struct rte_eth_rxq_rearm_data {
> > + void *rx_sw_ring;
>
> That's misleading, we always suppose to store mbufs ptrs here, so why not
> be direct:
> struct rte_mbuf **rx_sw_ring;
>
Agree.
> > + uint16_t *rearm_start;
> > + uint16_t *rearm_nb;
>
> I know that for Intel NICs uint16_t is sufficient, wonder would it always be
> for other vendors?
> Another thing to consider the case when ring position wrapping?
> Again I know that it is not required for Intel NICs, but would it be
> sufficient
> for API that supposed to be general?
>
For this, we re-define this structure:
rte_eth_rxq_rearm_data {
void *rx_sw_ring;
uint16_t *rearm_start;
uint16_t *rearm_nb;
}
->
struct *rxq_recycle_info {
rte_mbuf **buf_ring;
uint16_t *offset = (uint16 *)(&rq->ci);
uint16_t *end;
uint16_t ring_size;
}
For the new structure, *offset is a pointer for rearm-start index of
Rx buffer ring (consumer index). *end is a pointer for rearm-end index
Of Rx buffer ring (producer index).
1. we look up different pmds, some pmds using 'uint_16t' as index size like
intel PMD,
some pmds using 'uint32_t' as index size like MLX5 or thunderx PMD.
For pmd using 'uint32_t', rearm starts at 'buf_ring[offset & (ring_size -1)]',
and 'uint16_t'
is enough for ring size.
2. Good question. In general path, there is a constraint that 'nb_rearm <
ring_size - rq->ci',
This can ensure no ring wrapping in rearm. Thus in direct-rearm, we will refer
to this to
solve ring wrapping.
>
> > +} __rte_cache_min_aligned;
> > +
> > /* Generic Burst mode flag definition, values can be ORed. */
> >
> > /**
> > @@ -3184,6 +3195,34 @@ int
> rte_eth_dev_set_rx_queue_stats_mapping(uint16_t port_id,
> > uint16_t rx_queue_id,
> > uint8_t stat_idx);
> >
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > +notice
> > + *
> > + * Directly put Tx freed buffers into Rx sw-ring and flush desc.
> > + *
> > + * @param rx_port_id
> > + * Port identifying the receive side.
> > + * @param rx_queue_id
> > + * The index of the receive queue identifying the receive side.
> > + * The value must be in the range [0, nb_rx_queue - 1] previously
> supplied
> > + * to rte_eth_dev_configure().
> > + * @param tx_port_id
> > + * Port identifying the transmit side.
> > + * @param tx_queue_id
> > + * The index of the transmit queue identifying the transmit side.
> > + * The value must be in the range [0, nb_tx_queue - 1] previously
> supplied
> > + * to rte_eth_dev_configure().
> > + * @param rxq_rearm_data
> > + * A pointer to a structure of type *rte_eth_txq_rearm_data* to be
> > filled.
> > + * @return
> > + * - 0: Success
> > + */
> > +__rte_experimental
> > +int rte_eth_dev_direct_rearm(uint16_t rx_port_id, uint16_t rx_queue_id,
> > + uint16_t tx_port_id, uint16_t tx_queue_id,
> > + struct rte_eth_rxq_rearm_data *rxq_rearm_data);
>
>
> I think we need one more parameter for that function 'uint16_t offset'
> or so.
> So _rearm_ will start to populate rx_sw_ring from *rearm_start + offset
> position. That way we can support populating from different sources.
> Or should 'offset' be part of truct rte_eth_rxq_rearm_data?
>
Agree, please see above, we will do the change in the structure.
> > +
> > /**
> > * Retrieve the Ethernet address of an Ethernet device.
> > *
> > @@ -4782,6 +4821,27 @@ int rte_eth_rx_queue_info_get(uint16_t
> port_id, uint16_t queue_id,
> > int rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
> > struct rte_eth_txq_info *qinfo);
> >
> > +/**
> > + * Get rearm data about given ports's Rx queue.
> > + *
> > + * @param port_id
> > + * The port identifier of the Ethernet device.
> > + * @param queue_id
> > + * The Rx queue on the Ethernet device for which rearm data
> > + * will be got.
> > + * @param rxq_rearm_data
> > + * A pointer to a structure of type *rte_eth_txq_rearm_data* to be
> > filled.
> > + *
> > + * @return
> > + * - 0: Success
> > + * - -ENODEV: If *port_id* is invalid.
> > + * - -ENOTSUP: routine is not supported by the device PMD.
> > + * - -EINVAL: The queue_id is out of range.
> > + */
> > +__rte_experimental
> > +int rte_eth_rx_queue_rearm_data_get(uint16_t port_id, uint16_t
> queue_id,
> > + struct rte_eth_rxq_rearm_data *rxq_rearm_data);
> > +
> > /**
> > * Retrieve information about the Rx packet burst mode.
> > *
> > @@ -6103,6 +6163,120 @@ static inline int
> rte_eth_tx_descriptor_status(uint16_t port_id,
> > return (*p->tx_descriptor_status)(qd, offset);
> > }
> >
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > +notice
> > + *
> > + * Fill Rx sw-ring with Tx buffers in direct rearm mode.
> > + *
> > + * @param port_id
> > + * The port identifier of the Ethernet device.
> > + * @param queue_id
> > + * The index of the transmit queue.
> > + * The value must be in the range [0, nb_tx_queue - 1] previously
> supplied
> > + * to rte_eth_dev_configure().
> > + * @param rxq_rearm_data
> > + * A pointer to a structure of type *rte_eth_rxq_rearm_data* to be filled
> with
> > + * the rearm data of a receive queue.
> > + * @return
> > + * - The number buffers correct to be filled in the Rx sw-ring.
> > + * - (-EINVAL) bad port or queue.
> > + * - (-ENODEV) bad port.
> > + * - (-ENOTSUP) if the device does not support this function.
> > + *
> > + */
> > +__rte_experimental
> > +static inline int rte_eth_tx_fill_sw_ring(uint16_t port_id,
> > + uint16_t queue_id, struct rte_eth_rxq_rearm_data *rxq_rearm_data)
> {
> > + struct rte_eth_fp_ops *p;
> > + void *qd;
> > +
> > +#ifdef RTE_ETHDEV_DEBUG_TX
> > + if (port_id >= RTE_MAX_ETHPORTS ||
> > + queue_id >= RTE_MAX_QUEUES_PER_PORT) {
> > + RTE_ETHDEV_LOG(ERR,
> > + "Invalid port_id=%u or queue_id=%u\n",
> > + port_id, queue_id);
> > + return -EINVAL;
> > + }
> > +#endif
> > +
> > + p = &rte_eth_fp_ops[port_id];
> > + qd = p->txq.data[queue_id];
> > +
> > +#ifdef RTE_ETHDEV_DEBUG_TX
> > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> > +
> > + if (qd == NULL) {
> > + RTE_ETHDEV_LOG(ERR, "Invalid Tx queue_id=%u for
> port_id=%u\n",
> > + queue_id, port_id);
> > + return -ENODEV;
> > + }
> > +#endif
> > +
> > + if (p->tx_fill_sw_ring == NULL)
> > + return -ENOTSUP;
> > +
> > + return p->tx_fill_sw_ring(qd, rxq_rearm_data); }
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> > +notice
> > + *
> > + * Flush Rx descriptor in direct rearm mode.
> > + *
> > + * @param port_id
> > + * The port identifier of the Ethernet device.
> > + * @param queue_id
> > + * The index of the receive queue.
> > + * The value must be in the range [0, nb_rx_queue - 1] previously
> supplied
> > + * to rte_eth_dev_configure().
> > + *@param nb_rearm
> > + * The number of Rx sw-ring buffers need to be flushed.
> > + * @return
> > + * - (0) if successful.
> > + * - (-EINVAL) bad port or queue.
> > + * - (-ENODEV) bad port.
> > + * - (-ENOTSUP) if the device does not support this function.
> > + */
> > +__rte_experimental
> > +static inline int rte_eth_rx_flush_descriptor(uint16_t port_id,
> > + uint16_t queue_id, uint16_t nb_rearm) {
> > + struct rte_eth_fp_ops *p;
> > + void *qd;
> > +
> > +#ifdef RTE_ETHDEV_DEBUG_RX
> > + if (port_id >= RTE_MAX_ETHPORTS ||
> > + queue_id >= RTE_MAX_QUEUES_PER_PORT) {
> > + RTE_ETHDEV_LOG(ERR,
> > + "Invalid port_id=%u or queue_id=%u\n",
> > + port_id, queue_id);
> > + return -EINVAL;
> > + }
> > +#endif
> > +
> > + p = &rte_eth_fp_ops[port_id];
> > + qd = p->rxq.data[queue_id];
> > +
> > +#ifdef RTE_ETHDEV_DEBUG_RX
> > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> > +
> > + if (qd == NULL) {
> > + RTE_ETHDEV_LOG(ERR, "Invalid Rx queue_id=%u for
> port_id=%u\n",
> > + queue_id, port_id);
> > + return -ENODEV;
> > + }
> > +#endif
> > +
> > + if (p->rx_flush_descriptor == NULL)
> > + return -ENOTSUP;
> > +
> > + return p->rx_flush_descriptor(qd, nb_rearm); }
> > +
> > /**
> > * @internal
> > * Helper routine for rte_eth_tx_burst().
> > diff --git a/lib/ethdev/rte_ethdev_core.h
> > b/lib/ethdev/rte_ethdev_core.h index dcf8adab92..5ecb57f6f0 100644
> > --- a/lib/ethdev/rte_ethdev_core.h
> > +++ b/lib/ethdev/rte_ethdev_core.h
> > @@ -56,6 +56,13 @@ typedef int (*eth_rx_descriptor_status_t)(void *rxq,
> uint16_t offset);
> > /** @internal Check the status of a Tx descriptor */
> > typedef int (*eth_tx_descriptor_status_t)(void *txq, uint16_t
> > offset);
> >
> > +/** @internal Fill Rx sw-ring with Tx buffers in direct rearm mode */
> > +typedef int (*eth_tx_fill_sw_ring_t)(void *txq,
> > + struct rte_eth_rxq_rearm_data *rxq_rearm_data);
> > +
> > +/** @internal Flush Rx descriptor in direct rearm mode */ typedef int
> > +(*eth_rx_flush_descriptor_t)(void *rxq, uint16_t nb_rearm);
> > +
> > /**
> > * @internal
> > * Structure used to hold opaque pointers to internal ethdev Rx/Tx
> > @@ -90,6 +97,8 @@ struct rte_eth_fp_ops {
> > eth_rx_queue_count_t rx_queue_count;
> > /** Check the status of a Rx descriptor. */
> > eth_rx_descriptor_status_t rx_descriptor_status;
> > + /** Flush Rx descriptor in direct rearm mode */
> > + eth_rx_flush_descriptor_t rx_flush_descriptor;
> > /** Rx queues data. */
> > struct rte_ethdev_qdata rxq;
> > uintptr_t reserved1[3];
> > @@ -106,6 +115,8 @@ struct rte_eth_fp_ops {
> > eth_tx_prep_t tx_pkt_prepare;
> > /** 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;
> > /** Tx queues data. */
> > struct rte_ethdev_qdata txq;
> > uintptr_t reserved2[3];
> > diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map index
> > 17201fbe0f..f39f02a69b 100644
> > --- a/lib/ethdev/version.map
> > +++ b/lib/ethdev/version.map
> > @@ -298,6 +298,12 @@ EXPERIMENTAL {
> > rte_flow_get_q_aged_flows;
> > rte_mtr_meter_policy_get;
> > rte_mtr_meter_profile_get;
> > +
> > + # added in 23.03
> > + rte_eth_dev_direct_rearm;
> > + rte_eth_rx_flush_descriptor;
> > + rte_eth_rx_queue_rearm_data_get;
> > + rte_eth_tx_fill_sw_ring;
> > };
> >
> > INTERNAL {
