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

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

On 3/7/2023 6:12 AM, Honnappa Nagarahalli wrote:
> <snip>
> 
>>
>> On 3/6/2023 1:26 PM, Morten Brørup wrote:
>>>> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
>>>> Sent: Monday, 6 March 2023 13.49
>>>>
>>>> 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.
>>>
>>> I got the impression that the supported use case is a prominent and important
>> use case.
>>>
>>
>> Can you please give real life samples for this use case, other than just showing
>> better performance number in the test bench? This helps to understand the
>> reasoning better.
> The very first patch started off with a constrained but prominent use case. Though, DPU based PCIe cards running DPDK applications with 1 or max 2 ports being used in tons of data centers is not a secret anymore and not a small use case that can be ignored.
> However, the design of the patch has changed significantly from then. Now the solution can be applied to any generic use case that uses run-to-completion model of DPDK. i.e. the mapping of the RX and TX ports can be done dynamically in the data plane threads. There is no need of static configuration from control plane.
> 
> On the test bench, we need to make up our mind. When we see improvements, we say it is just a test bench. On other occasions when the test bench does not show any improvements (but improvements are shown by other metrics), we say the test bench does not show any improvements.
> 
>>
>>> This is the primary argument for considering such a complex non-generic
>> feature.
> I am not sure what is the complexity here, can you please elaborate?

I am considering from user perspective.

OK, DPDK is already low level, but ethdev has only a handful of datapath
APIs (6 of them), and main ones are easy to comprehend:
rte_eth_rx_burst(port_id, queue_id, rx_pkts, nb_pkts);
rte_eth_tx_burst(port_id, queue_id, tx_pkts, nb_pkts);

They (magically) Rx/Tx buffers, easy to grasp.

Maybe rte_eth_tx_prepare() is a little less obvious (why/when to use
it), but still I believe simple.

Whoever looks to these APIs can figure out how to use in the application.

The other three is related to the descriptors and I am not sure about
their use-case, I assume they are mostly good for debugging.


But now we are adding new datapath APIs:
rte_eth_tx_fill_sw_ring(port_id, queue_id, rxq_rearm_data);
rte_eth_rx_flush_descriptor(port_id, queue_id, nb_rearm);

When you talk about SW ring and re-arming descriptors I believe you will
loose most of the users already, driver developers will know what it is,
you will know what that is, but people who are not close to the Ethernet
HW won't.

And these APIs will be very visible, not like one of many control plane
dev_ops. So this can confuse users who are not familiar with details.

Usage of these APIs comes with restrictions, it is possible that at some
percentage of users will miss these restrictions or miss-understand them
and will have issues.

Or many may be intimidated by them and stay away from using these APIs,
leaving them as a burden to maintain, to test, to fix. That is why I
think a real life usecase is needed, in that case at least we will know
some consumers will fix or let us know when they get broken.

It may be possible to hide details under driver and user only set an
offload flag, similar to FAST_FREE, but in that case feature will loose
flexibility and it will be even more specific, perhaps making it less
useful.


> I see other patches/designs (ex: proactive error recovery) which are way more complex to understand and comprehend.
> 
>>>
>>>>
>>>> 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 agree that we are touching some fundamental parts of the project. But, we also need to realize that those fundamental parts were not developed on architectures that have joined the project way later. Similarly, the use cases have evolved significantly from the original intended use cases. We cannot hold on to those fundamental designs if they affect the performance on other architectures while addressing prominent new use cases.
> Please note that this patch does not break any existing features or affect their performance in any negative way. The generic and originally intended use cases can benefit from this feature.
> 
>>>>
>>>>
>>>> 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.
>>>
>>> I agree that the complexity is very high, and thus requires extra consideration.
>> Your suggested techboard review and approval process seems like a good
>> solution.
> We can add to the agenda for the next Techboard meeting.
> 
>>>
>>> And the performance benefit of direct rearm should be compared to the
>> performance using the new zero-copy mempool API.
>>>
>>> -Morten
>>>