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

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 1/4/2023 8:21 AM, Morten Brørup wrote:
>> 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.
> 

Hi Morten,

Yes it brings some performance, but not to generic use case, only to
specific and constraint use case.

And changes are relatively invasive comparing the usecase it supports,
like it adds new two inline datapath functions and a new dev_ops.

I am worried the unnecessary complexity and possible regressions in the
fundamental and simple parts of the project, with a good intention to
gain a few percentage performance in a specific usecase, can hurt the
project.


I can see this is compared to MBUF_FAST_FREE feature, but MBUF_FAST_FREE
is just an offload benefiting from existing offload infrastructure,
which requires very small update and logically change in application and
simple to implement in the drivers. So, they are not same from
complexity perspective.

Briefly, I am not comfortable with this change, I would like to see an
explicit approval and code review from techboard to proceed.


> 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().
> 
> 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.
> 
> 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.
> 
>> +	/** 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.
> 
> [...]
> 
>> 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.
>