> -----Original Message----- > From: Ferruh Yigit <ferruh.yi...@amd.com> > Sent: Tuesday, March 7, 2023 2:41 PM > To: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>; Morten Brørup > <m...@smartsharesystems.com>; Feifei Wang <feifei.wa...@arm.com>; > tho...@monjalon.net; Andrew Rybchenko > <andrew.rybche...@oktetlabs.ru>; techbo...@dpdk.org > Cc: dev@dpdk.org; konstantin.v.anan...@yandex.ru; nd <n...@arm.com>; > Ruifeng Wang <ruifeng.w...@arm.com> > Subject: Re: [PATCH v3 1/3] ethdev: enable direct rearm with separate API > > 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.yi...@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.wa...@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.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> > >>>>>> --- > >>>>> > >>>>> 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. Thanks for clarifying Ferruh.
> > 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. I think the pktmbuf pool part is missed here. The user needs to create a pktmbuf pool by calling rte_pktmbuf_pool_create and has to pass the cache_size parameter. This requires the user to understand what is a cache, why it is required and how it affects the performance. There are further complexities associated with pktmbuf pool - creating a pool with external pinned memory, creating a pool with ops name etc. So, practically, the user needs to be aware of more details than just the RX and TX functions. > > 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. Agree, the names could be better. I personally do not want to separate out these two APIs as I do not think a use case (receiving and transmitting pkts across NICs of different types) exists to keep them separate. But, we did this based on feedback and to maintain a cleaner separation between RX and TX path. We will try to propose new names for these. > > 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. Agreed, there are several features already with restrictions. > > 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. Agree. > > > > 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 > >>>
