Re: 回复: 回复: 回复: [PATCH v2 1/3] ethdev: add API for direct rearm mode

[Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>]

Hi Feifei,


>>>>>>> Add API for enabling direct rearm mode and for mapping RX and TX
>>>>>>> queues. Currently, the API supports 1:1(txq : rxq) mapping.
>>>>>>>
>>>>>>> Furthermore, to avoid Rx load Tx data directly, add API called
>>>>>>> 'rte_eth_txq_data_get' to get Tx sw_ring and its information.
>>>>>>>
>>>>>>> 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>
>>>>>>> ---
>>>>>>>      lib/ethdev/ethdev_driver.h   |  9 ++++
>>>>>>>      lib/ethdev/ethdev_private.c  |  1 +
>>>>>>>      lib/ethdev/rte_ethdev.c      | 37 ++++++++++++++
>>>>>>>      lib/ethdev/rte_ethdev.h      | 95
>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>      lib/ethdev/rte_ethdev_core.h |  5 ++
>>>>>>>      lib/ethdev/version.map       |  4 ++
>>>>>>>      6 files changed, 151 insertions(+)
>>>>>>>
>>>>>>> diff --git a/lib/ethdev/ethdev_driver.h
>>>>>>> b/lib/ethdev/ethdev_driver.h index 47a55a419e..14f52907c1 100644
>>>>>>> --- a/lib/ethdev/ethdev_driver.h
>>>>>>> +++ b/lib/ethdev/ethdev_driver.h
>>>>>>> @@ -58,6 +58,8 @@ 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;
>>>>>>> +	/**  Use Tx mbufs for Rx to rearm */
>>>>>>> +	eth_rx_direct_rearm_t rx_direct_rearm;
>>>>>>>
>>>>>>>      	/**
>>>>>>>      	 * Device data that is shared between primary and secondary
>>>>>>> processes @@ -486,6 +488,11 @@ typedef int
>>>>>> (*eth_rx_enable_intr_t)(struct rte_eth_dev *dev,
>>>>>>>      typedef int (*eth_rx_disable_intr_t)(struct rte_eth_dev *dev,
>>>>>>>      				    uint16_t rx_queue_id);
>>>>>>>
>>>>>>> +/**< @internal Get Tx information of a transmit queue of an
>>>>>>> +Ethernet device. */ typedef void (*eth_txq_data_get_t)(struct
>>>> rte_eth_dev *dev,
>>>>>>> +				      uint16_t tx_queue_id,
>>>>>>> +				      struct rte_eth_txq_data
>> *txq_data);
>>>>>>> +
>>>>>>>      /** @internal Release memory resources allocated by given
>>>>>>> Rx/Tx
>>>> queue.
>>>>>> */
>>>>>>>      typedef void (*eth_queue_release_t)(struct rte_eth_dev *dev,
>>>>>>>      				    uint16_t queue_id);
>>>>>>> @@ -1138,6 +1145,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 the address where Tx data is stored */
>>>>>>> +	eth_txq_data_get_t         txq_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..bfe16c7d77 100644
>>>>>>> --- a/lib/ethdev/ethdev_private.c
>>>>>>> +++ b/lib/ethdev/ethdev_private.c
>>>>>>> @@ -276,6 +276,7 @@ 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->rx_direct_rearm = dev->rx_direct_rearm;
>>>>>>>
>>>>>>>      	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 0c2c1088c0..0dccec2e4b 100644
>>>>>>> --- a/lib/ethdev/rte_ethdev.c
>>>>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>>>>> @@ -1648,6 +1648,43 @@ rte_eth_dev_is_removed(uint16_t port_id)
>>>>>>>      	return ret;
>>>>>>>      }
>>>>>>>
>>>>>>> +int
>>>>>>> +rte_eth_tx_queue_data_get(uint16_t port_id, uint16_t queue_id,
>>>>>>> +			struct rte_eth_txq_data *txq_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_tx_queues) {
>>>>>>> +		RTE_ETHDEV_LOG(ERR, "Invalid Tx queue_id=%u\n",
>>>>>> queue_id);
>>>>>>> +		return -EINVAL;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	if (txq_data == NULL) {
>>>>>>> +		RTE_ETHDEV_LOG(ERR, "Cannot get ethdev port %u
>> Tx
>>>>>> queue %u data to NULL\n",
>>>>>>> +			port_id, queue_id);
>>>>>>> +		return -EINVAL;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	if (dev->data->tx_queues == NULL ||
>>>>>>> +			dev->data->tx_queues[queue_id] == NULL) {
>>>>>>> +		RTE_ETHDEV_LOG(ERR,
>>>>>>> +			   "Tx queue %"PRIu16" of device with
>> port_id=%"
>>>>>>> +			   PRIu16" has not been setup\n",
>>>>>>> +			   queue_id, port_id);
>>>>>>> +		return -EINVAL;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	if (*dev->dev_ops->txq_data_get == NULL)
>>>>>>> +		return -ENOTSUP;
>>>>>>> +
>>>>>>> +	dev->dev_ops->txq_data_get(dev, queue_id, txq_data);
>>>>>>> +
>>>>>>> +	return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>>      static int
>>>>>>>      rte_eth_rx_queue_check_split(const struct rte_eth_rxseg_split
>>>> *rx_seg,
>>>>>>>      			     uint16_t n_seg, uint32_t *mbp_buf_size,
>> diff --git
>>>>>>> a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index
>>>>>>> 2e783536c1..daf7f05d62 100644
>>>>>>> --- a/lib/ethdev/rte_ethdev.h
>>>>>>> +++ b/lib/ethdev/rte_ethdev.h
>>>>>>> @@ -1949,6 +1949,23 @@ 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 Tx data.
>>>>>>> + * The main purpose is to load and store Tx queue data in direct
>>>>>>> +rearm
>>>>>> mode.
>>>>>>> + */
>>>>>>> +struct rte_eth_txq_data {
>>>>>>> +	uint64_t *offloads;
>>>>>>> +	void *tx_sw_ring;
>>>>>>> +	volatile void *tx_ring;
>>>>>>> +	uint16_t *tx_next_dd;
>>>>>>> +	uint16_t *nb_tx_free;
>>>>>>> +	uint16_t nb_tx_desc;
>>>>>>> +	uint16_t tx_rs_thresh;
>>>>>>> +	uint16_t tx_free_thresh;
>>>>>>> +} __rte_cache_min_aligned;
>>>>>>> +
>>>>>>
>>>>>> first of all it is not clear why this struct has to be in public
>>>>>> header, why it can't be in on of ethdev 'private' headers.
>>>>>> Second it looks like a snippet from private txq fields for some
>>>>>> Intel (and alike) PMDs (i40e, ice, etc.).
>>>>>> How it supposed to to be universal and be applicable for any PMD
>>>>>> that decides to implement this new API?
>>>>>>
>>>>>>
>>>>>>>      /* Generic Burst mode flag definition, values can be ORed. */
>>>>>>>
>>>>>>>      /**
>>>>>>> @@ -4718,6 +4735,27 @@ int rte_eth_remove_rx_callback(uint16_t
>>>>>> port_id, uint16_t queue_id,
>>>>>>>      int rte_eth_remove_tx_callback(uint16_t port_id, uint16_t
>> queue_id,
>>>>>>>      		const struct rte_eth_rxtx_callback *user_cb);
>>>>>>>
>>>>>>> +/**
>>>>>>> + * Get the address which Tx data is stored.
>>>>>>> + *
>>>>>>> + * @param port_id
>>>>>>> + *   The port identifier of the Ethernet device.
>>>>>>> + * @param queue_id
>>>>>>> + *   The Tx queue on the Ethernet device for which information
>>>>>>> + *   will be retrieved.
>>>>>>> + * @param txq_data
>>>>>>> + *   A pointer to a structure of type *rte_eth_txq_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_tx_queue_data_get(uint16_t port_id, uint16_t
>> queue_id,
>>>>>>> +		struct rte_eth_txq_data *txq_data);
>>>>>>> +
>>>>>>>      /**
>>>>>>>       * Retrieve information about given port's Rx queue.
>>>>>>>       *
>>>>>>> @@ -6209,6 +6247,63 @@ rte_eth_tx_buffer(uint16_t port_id,
>>>>>>> uint16_t
>>>>>> queue_id,
>>>>>>>      	return rte_eth_tx_buffer_flush(port_id, queue_id, buffer);
>>>>>>>      }
>>>>>>>
>>>>>>> +/**
>>>>>>> + * @warning
>>>>>>> + * @b EXPERIMENTAL: this API may change, or be removed, without
>>>>>>> +prior notice
>>>>>>> + *
>>>>>>> + * Put Tx buffers into Rx sw-ring and rearm descs.
>>>>>>> + *
>>>>>>> + * @param port_id
>>>>>>> + *   Port identifying the receive side.
>>>>>>> + * @param queue_id
>>>>>>> + *   The index of the transmit 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 txq_data
>>>>>>> + *   A pointer to a structure of type *rte_eth_txq_data* to be filled.
>>>>>>> + * @return
>>>>>>> + *   The number of direct-rearmed buffers.
>>>>>>> + */
>>>>>>> +__rte_experimental
>>>>>>> +static __rte_always_inline uint16_t
>>>>>>> +rte_eth_rx_direct_rearm(uint16_t port_id, uint16_t queue_id,
>>>>>>> +		struct rte_eth_txq_data *txq_data) {
>>>>>>> +	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 0;
>>>>>>> +	}
>>>>>>> +#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 0;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	if (!p->rx_direct_rearm)
>>>>>>
>>>>>> This check should be done always (unconditionally).
>>>>>> it is not a mandatory function for the driver (it can safely skip
>>>>>> to
>>>> implement it).
>>>>>>
>>>>>>> +		return -ENOTSUP;
>>>>>>
>>>>>> This function returns uint16_t, why signed integers here?
>>>>>>
>>>>>>
>>>>>>> +#endif
>>>>>>> +
>>>>>>> +	nb_rearm = p->rx_direct_rearm(qd, txq_data);
>>>>>>
>>>>>> So rx_direct_rearm() function knows how to extract data from TX
>> queue?
>>>>>> As I understand that is possible only in one case:
>>>>>> rx_direct_rearm() has full knowledge and acess of txq internals, etc.
>>>>>> That means that rxq and txq have to belong to the same driver and
>>>>>> device type.
>>>>>>
>>>>> Thanks for the comments, and I have some questions for this.
>>>>>
>>>>>> First of all, I still think it is not the best design choice.
>>>>>> If we going ahead with introducing this feature, it better be as
>>>>>> generic as possible.
>>>>>> Plus it mixes TX and RX code-paths together, while it would be much
>>>>>> better to to keep them independent as they are right now.
>>>>>>
>>>>>> Another thing with such approach - even for the same PMD, both TXQ
>>>>>> and RXQ can have different internal data format and behavior logic
>>>>>> (depending on port/queue configuration).
>>>>> 1. Here TXQ and RXQ have different internal format means the queue
>>>>> type and  descs can be different, right? If I understand correctly,
>>>>> based on your first strategy, is it means we will need different
>>>>> 'rearm_func' for different queue type in the same PMD?
>>>>
>>>> Yes, I think so.
>>>> If let say we have some PMD where depending on the config, there
>>>> cpuld be
>>>> 2 different RXQ formats: rxq_a and rxq_b, and 2 different txq
>>>> formats: txq_c, txq_d.
>>>> Then assuming PMD would like to support direct-rearm mode for all
>>>> four combinations, it needs 4 different rearm functions:
>>>>
>>>> rearm_txq_c_to_rxq_a()
>>>> rearm_txq_c_to_rxq_b()
>>>> rearm_txq_d_to_rxq_a()
>>>> rearm_txq_d_to_rxq_b()
>>>>
>>> Thank you for your detailed explanation, I can understand this.
> 
>>>>
>>>>>
>>>>>> So rx_direct_rearm() function selection have to be done based on
>>>>>> both RXQ and TXQ config.
>>>>>> So instead of rte_eth_tx_queue_data_get(), you'll probably need:
>>>>>> eth_rx_direct_rearm_t rte_eth_get_rx_direct_rearm_func(rx_port,
>>>>>> rx_queue, tx_port, tx_queue);
>>>>>> Then, it will be user responsibility to store it somewhere and call
>>>> periodically:
>>>>>>
>>>>>> control_path:
>>>>>> 	...
>>>>>> 	rearm_func = rte_eth_get_rx_direct_rearm_func(rxport, rxqueue,
>>>>>> 		 txport, txqueue);
>>>>>> data-path:
>>>>>> 	while(...) {
>>>>>> 		rearm_func(rxport, txport, rxqueue, txqueue);
>>>>>> 		rte_eth_rx_burst(rxport, rxqueue, ....);
>>>>>> 		rte_eth_tx_burst(txport, txqueue, ....);
>>>>>> 	}
>>>>>>
>>>>>>
>>>>>> In that case there seems absolutely no point to introduce struct
>>>>>> rte_eth_txq_data. rx_direct_rearm() accesses TXQ private data
>>>>>> directly anyway.
>>>>> 2. This is a very good proposal and it will be our first choice.
>>>>> Before working on it, I have a few questions about how to implement
>>>> 'rearm_func'.
>>>>> Like you say above, mixed Rx and Tx path code in 'rearm_func' means
>>>>> the hard-code is mixed like:
>>>>> rearm_func(...) {
>>>>>         ...
>>>>>        txep = &txq->sw_ring[txq->tx_next_dd - (txq->tx_rs_thresh - 1)];
>>>>>        for (...) {
>>>>>           rxep[i].mbuf = txep[i].mbuf;
>>>>>           mb0 = txep[i].mbuf;
>>>>>           paddr = mb0->buf_iova + RTE_PKTMBUF_HEADROOM;
>>>>>          dma_addr0 = vdupq_n_u64(paddr);
>>>>>          vst1q_u64((uint64_t *)&rxdp++->read, dma_addr0);
>>>>>        }
>>>>> }
>>>>> Is my understanding is right?
>>>>
>>>>
>>>> Sorry, I don't understand the question.
>>>> Can you probably elaborate a bit?
>>>
>>> Sorry for my unclear expression.
>>>
>>> I mean if we need two func which contains tx and rx paths code
>> respectively in rearm_func, like:
>>> rearm_func(...) {
>>>            rte_tx_fill_sw_ring;
>>>            rte_rx_rearm_descs;
>>> }
>>>
>>> Or just mixed tx and rx path code like I said before. I prefer 'rx and
>>> tx hard code mixed', because from the performance perspective, this can
>> reduce the cost of function calls.
>>
>> I suppose it depends on what we choose:
>> If we restrict it in a way that rxq and txq have to belong to the same PMD,
>> then I suppose this decision could be left to each particular PMD.
>> If we'd like to allow rearm to work accross different PMDs (i.e. it would allow
>> to rearm mlx with ice and visa-versa), then yes we need PMDs somehow to
>> expose rx_sw_ring abstraction to each other.
>> My preference would be the second one - as it will make this feature more
>> flexible and would help to adopt it more widely.
>> Though the first one is probably easier to implement, and as I udnerstand
>> you are leaning towards the first one.
>>
>> Konstantin
> 
> Hi, Konstantin
> 
> 1. After further consideration, I think we should give up  'rte_eth_get_rx_direct_rearm'  functions.
>   And just keep one API 'rte_eth_direct_rearm' which contains different queue types path in the pmd driver.
> 
> This is due to that application can dynamically configure queue-mapping in the data path,
> and thus expand direct-rearm usage scenarios based on this way. For example, consider a flow
> which received by one rx_queue(rxq_1) and sent by two different types tx_queues( type_a txq_1 and type_b txq_2),
> Users can dynamically map direct_rearm according to tx path.

That's good point, indeed for such case special function pointer doesn't 
look very conveniet.
I still think that the best way to deal with multiple sources would be
to expose some sort of rx_sw_ring abstraction.
Then it could be obtaiend once and passed as a paramter to actual rearm 
function.
In that case, user can even re-arm RX queue not from TX queue only,
but from other sources too (freed mbufs, etc.).
As I understand what you suggest is one unified function:
int rte_eth_rx_direct_rearm(rx_port, rx_queue, tx_port, tx_queue), correct?
I am not against it in principle, but does it mean at each invocation,
inside PMD layer, first thing you would have to do -
check that rx_queue and tx_queue are from the same device type and 
select proper behaviour (for different queue configs)?

> --------------------------------------------------------------------------------------
> rte_eth_rx_burst(rxq_1);
> 
> %routing table lookup to decide tx queue%
> txq_n = txq_1 or txq_2?
> 
> rte_eth_tx_burst(txq_n);
> rte_eth_direct_rearm(txq_n);
> --------------------------------------------------------------------------------------
> Thus, this can avoid repeatedly calling 'rte_eth_get_rx_direct_rearm' in the data path to reduce performance.
> 
> Furthermore, rte_eth_direct_rearm can be implemented as:
> --------------------------------------------------------------------------------------
> rte_eth_direct_rearm = i40e_eth_direct_rearm;
> i40e_eth_direct_rearm {
> 	rearm_queue_config_a path;
> 	rearm_queue_config_b path;
> 	rearm_queue_config_c path;
> 	rearm_queue_config_d path;
> }
> --------------------------------------------------------------------------------------
> This implementation refers to 'rte_eth_rx_burst' and 'rte_eth_tx_burst'. If there will be different
> queue types, the implementation will be in pmd layer.
> 
> 2. I'm not sure whether cross pmd usage is realistic.
> Consider the tradeoff between performance and different pmd map, maybe we can provide two
> different direct rearm mode for users. One is mixed Rx and Tx path and is used for the same pmd.
> The other is separate Rx and Tx path, and is used for different pmd.
> Best Regards
> Feifei
>>
>>
>>>>
>>>>>
>>>>>>
>>>>>> Another way - make rte_eth_txq_data totally opaque and allow PMD
>> to
>>>>>> store there some data that will help it to distinguish expected TXQ
>> format.
>>>>>> That will allow PMD to keep rx_direct_rearm() the same for all
>>>>>> supported TXQ formats (it will make decision internally based on
>>>>>> data stored in txq_data).
>>>>>> Though in that case you'll probably need one more dev-op to free
>>>> txq_data.
>>>>>
>>>
>