Re: [dpdk-dev] [PATCH 4/4] port: fix ethdev writer burst too big

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

> -----Original Message-----
> From: Sanford, Robert [mailto:rsanford@akamai.com]
> Sent: Monday, April 11, 2016 9:37 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org
> Cc: Liang, Cunming <cunming.liang@intel.com>; Venkatesan, Venky
> <venky.venkatesan@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 4/4] port: fix ethdev writer burst too big
> 
> Hi Cristian,
> 
> Yes, I mostly agree with your suggestions:
> 1. We should fix the two obvious bugs (1a and 1b) right away. Jasvinder's
> patches look fine.
> 2. We should take no immediate action on the issue I raised about PMDs
> (vector IXGBE) not enqueuing more than 32 packets. We can discuss and
> debate; no patch for 16.04, perhaps something in 16.07.
> 
> 
> Let's start the discussion now, regarding vector IXGBE. You state
> "Internally it handles packets in batches [of] 32 (as your code snippets
> suggest), but there is no drop of excess packets taking place." I guess it
> depends on your definition of "drop". If I pass 33 packets to
> ixgbe_xmit_pkts_vec(), it will enqueue 32 packets, and return a value of
> 32. Can we agree on that?
> 

Yes, Steve Liang and I looked at the latest IXGBE vector code and it looks like you are right. The number of packets that get accepted is the minimum between number of packets provided by the user (33 in our case) and two thresholds, txq->tx_rs_thresh and txq->nb_tx_free, which are by default set to 32, which is the value that yields the best performance, hence only 32 packets get accepted.

It also looks virtually impossible to change this behaviour of IXGBE vector driver. As an example, let's say 33 packets are presented by the user, IXGBE picks the first 32 and tries to send them, but only 17 make it, so the other 15 have to be returned back to the user; then there is the 33rd packet that is picked, and this packet makes it. Since return value is a number (not a mask), how do you tell the user that packets 0 .. 16 and 32 made it, while the packets 17 .. 31 in the middle of the burst did not make it?

So the only real place to improve this is the port_ethdev_writer. I wonder whether there is nice way to combine existing behavior (burst treated as minimal requirement) with your proposal (burst treated as constant requirement) under the same code, and then pick between the two behaviors based on an input parameter provided when port is created?

> --
> Regards,
> Robert
> 
> 
> On 4/11/16 3:21 PM, "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
> wrote:
> 
> >Hi Robert,
> >
> >I am doing a quick summary below on the changes proposed by these
> patches:
> >
> >1. [PRIORITY 1] Bug fixing:
> >a) Fix buffer overflow issue in rte_port_ring.c (writer, writer_nodrop):
> >double the tx_buf buffer size (applicable for current code approach)
> >b) Fix issue with handling burst sizes bigger than 32: replace all
> >declarations of local variable bsz_size from uint32_t to uint64_t
> >
> >2. [PRIORITY 2] Treat burst size as a fixed/exact value for the TX burst
> >(Approach 2) instead of minimal value (current code, Approach 1) for
> >ethdev ports. Rationale is that some PMDs (like vector IXGBE) _might_
> >drop the excess packets in the burst.
> >
> >Additional input:
> >1. Bruce and I looked together at the code, it looks that vector IXGBE is
> >not doing this (anymore). Internally it handles packets in batches on 32
> >(as your code snippets suggest), but there is no drop of excess packets
> >taking place.
> >
> >2. Venky also suggested to keep a larger burst as a single burst
> >(Approach 1) rather than break the larger burst into a fixed/constant
> >size burst while buffering the excess packets until complete burst is met
> >in the future.
> >
> >Given this input and also the timing of the release, we think the best
> >option is:
> >- urgently send a quick patch to handle the bug fixes now
> >- keep the existing code (burst size used as minimal burst size
> >requirement, not constant) as is, at least for now, and if you feel it is
> >not the best choice, we can continue to debate it for 16.7 release.
> >What do you think?
> >
> >Jasvinder just send the bug fixing patches, hopefully they will make it
> >into the 16.4 release:
> >http://www.dpdk.org/ml/archives/dev/2016-April/037392.html
> >http://www.dpdk.org/ml/archives/dev/2016-April/037393.html
> >
> >Many thanks for your work on this, Robert!
> >
> >Regards,
> >Cristian