On Thu, Aug 27, 2020 at 12:13:51PM +0200, Morten Brørup wrote: > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Bruce Richardson > > Sent: Thursday, August 27, 2020 11:44 AM > > > > On Thu, Aug 27, 2020 at 11:31:15AM +0200, Morten Brørup wrote: > > > > From: Bruce Richardson [mailto:bruce.richard...@intel.com] > > > > Sent: Thursday, August 27, 2020 11:10 AM > > > > > > > > On Thu, Aug 27, 2020 at 10:40:11AM +0200, Morten Brørup wrote: > > > > > Jeff and Ethernet API maintainers Thomas, Ferruh and Andrew, > > > > > > > > > > I'm hijacking this patch thread to propose a small API > > modification > > > > that prevents unnecessarily performance degradations. > > > > > > > > > > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Jeff Guo > > > > > > Sent: Thursday, August 27, 2020 9:55 AM > > > > > > > > > > > > The limitation of burst size in vector rx was removed, since it > > > > should > > > > > > retrieve as much received packets as possible. And also the > > > > scattered > > > > > > receive path should use a wrapper function to achieve the goal > > of > > > > > > burst maximizing. > > > > > > > > > > > > This patch set aims to maximize vector rx burst for for > > > > > > ixgbe/i40e/ice/iavf PMDs. > > > > > > > > > > > > > > > > Now I'm going to be pedantic and say that it still doesn't > > conform to > > > > the rte_eth_rx_burst() API, because the API does not specify any > > > > minimum requirement for nb_pkts. > > > > > > > > > > In theory, that could also be fixed in the driver by calling the > > non- > > > > vector function from the vector functions if nb_pkts is too small > > for > > > > the vector implementation. > > > > > > > > > > However, I think that calling rte_eth_rx_burst() with a small > > nb_pkts > > > > is silly and not in the spirit of DPDK, and introducing an > > additional > > > > comparison for a small nb_pkts in the driver vector functions would > > > > degrade their performance (only slightly, but anyway). > > > > > > > > > > > > > Actually, I'd like to see a confirmed measurement showing a > > slowdown > > > > before > > > > we discard such an option. :-) > > > > > > Good point! > > > > > > > While I agree that using small bursts is > > > > not > > > > keeping with the design approach of DPDK of using large bursts to > > > > amortize > > > > costs and allow prefetching, there are cases where a user/app may > > want > > > > a > > > > small burst size, e.g. 4, for latency reasons, and we need a way to > > > > support > > > > that. > > > > > > > I assume that calling rte_eth_rx_burst() with nb_pkts=32 returns 4 > > packets if only 4 packets are available, so you would need to be > > extremely latency sensitive to call it with a smaller nb_pkts. I guess > > that high frequency trading is the only real life scenario here. > > > > > Yes, it really boils down to whether you are prepared to accept lower > > max throughput or dropped packets in order to gain lower latency. > > > > > > Since the path selection is dynamic, we need to either: > > > > a) provide a way for the user to specify that they will use smaller > > > > bursts > > > > and so that vector functions should not be used > > > > b) have the vector functions transparently fallback to the scalar > > ones > > > > if > > > > used with smaller bursts > > > > > > > > Of these, option b) is simpler, and should be low cost since any > > check > > > > is > > > > just once per burst, and - assuming an app is written using the > > same > > > > request-size each time - should be entirely predictable after the > > first > > > > call. > > > > > > > Why does everyone assume that DPDK applications are so simple that > > the branch predictor will cover the entire data path? I hear this > > argument over and over again, and by principle I disagree with it! > > > > > > > Fair enough, that was an assumption on my part. Do you see in your apps > > many cases where branches are getting mispredicted despite going the > > same > > way each time though the code? > > > We haven't looked deeply into this, but I don't think so. > > My objection is of a more general nature. As a library, DPDK cannot assume > that applications using it are simple, and - based on that assumption - take > away resources that could have been available for the application. > > The Intel general optimization guidelines specifies that code should be > arranged to be consistent with the static branch prediction algorithm: make > the fall-through code following a conditional branch be the likely target for > a branch with a forward target, and make the fall-through code following a > conditional branch be the unlikely target for a branch with a backward target. > > It also says: Conditional branches that are never taken do not consume BTB > resources. > > Somehow this last detail is completely ignored by DPDK developers. > > We put a lot of effort into conserving resources in most areas in DPDK, but > when it comes to the branch prediction target buffer (BTB), we gladly > organize code with branches turning the wrong way, thus unnecessarily > consuming BTB entries. And the argument goes: The branch predictor will catch > it after the first time. >
Looks like something to investigate more. Thanks for bringing this up. > > > How about c): add rte_eth_rx() and rte_eth_tx() functions for > > receiving/transmitting a single packet. The ring library has such > > functions. > > > > > > Optimized single-packet functions might even perform better than > > calling the burst functions with nb_pkts=1. Great for latency focused > > applications. :-) > > > > > That is another option, yes. > > A further option is to add to the vector code a one-off switch to check > > first > > time it's called that the request size is not lower than the min > > supported > > (again basing on the assumption that one is not going to be varying the > > burst size asked - which may not be true in call cases but won't leave > > us > > any worse off than we are now!). > > I certainly don't support this option. But it was worth mentioning. > Right. For now then, it seems like just documenting a minimum burst size is reasonable. /Bruce
