On Fri, Jan 24, 2020 at 6:40 AM Flavio Leitner <[email protected]> wrote: > > On Wed, Jan 22, 2020 at 10:33:59AM -0800, William Tu wrote: > > On Wed, Jan 22, 2020 at 12:54 AM Flavio Leitner <[email protected]> wrote: > > > > > > > > > Hi Ben, > > > > > > Thanks for reviewing it! > > > > > > On Tue, Jan 21, 2020 at 01:35:39PM -0800, Ben Pfaff wrote: > > > > On Sat, Jan 18, 2020 at 12:08:06AM +0100, Ilya Maximets wrote: > > > > > On 18.01.2020 00:03, Stokes, Ian wrote: > > > > > > Thanks all for review/testing, pushed to master. > > > > > > > > > > OK, thanks Ian. > > > > > > > > > > @Ben, even though this patch already merged, I'd ask you to take a > > > > > look > > > > > at the code in case you'll spot some issues especially in non-DPDK > > > > > related > > > > > parts. > > > > > > > > I found the name dp_packet_hwol_is_ipv4(), and similar, confusing. The > > > > name suggested to me "test whether the packet is IPv4" not "test whether > > > > the packet has an offloaded IPv4 checksum". I guess the "hwol" is > > > > offload related but... I like the name dp_packet_hwol_tx_l4_checksum() > > > > much more, it makes it obvious at a glance that it's a > > > > checksum-offloading check. > > > > > > hwol = hardware offloading. I hear that all the time, but maybe there is a > > > better name. I will improve that if no one gets on it first. > > > > > > > In the case where we actually receive a 64 kB packet, I think that this > > > > code is going to be relatively inefficient. If I'm reading the code > > > > correctly (I did it quickly), then this is what happens: > > > > > > > > - The first 1500 bytes of the packet land in the first > > > > dp_packet. > > > > > > > > - The remaining 64000ish bytes land in the second dp_packet. > > > > It's not a dp_packet, it's a preallocated buffer per rxq (aux_bufs). > > > > struct netdev_rxq_linux { > > struct netdev_rxq up; > > bool is_tap; > > int fd; > > char *aux_bufs[NETDEV_MAX_BURST]; /* Batch of preallocated TSO buffers. > > */ > > }; > > > > > > > > > > - Then we expand the first dp_packet to the needed size and copy > > > > the remaining 64000 bytes into it. > > > > > > That's correct. > > > > > > > An alternative would be: > > > > > > > > - Set up the first dp_packet as currently. > > > > > > > > - Set up the second dp_packet so that the bytes are received > > > > into it starting at offset (mtu + headroom). > > > > > > > > - If more than mtu bytes are received, then copy those bytes > > > > into the headroom of the second dp_packet and return it to the > > > > caller instead of the first dp_packet. > > > > > > I wanted to avoid doing more extensive processing if it's not a TSO packet > > > to avoid performance regressions since it' very sensitive. Right now the > > > 64k > > > buffer is preallocated and is static for each queue to avoid the malloc > > > performance issue. Now for TSO case, we have more time per packet for > > > processing. > > > > Can we implement Ben's idea by > > 1) set size of aux_buf to 64k + mtu > > 2) create 2nd dp_packet using this aux_buf and copy first packet to > > first mtu bytes of aux_buf > > 3) since we steal this aux_bufs, allocate a new aux_buf by > > rxq->aux_bufs[i] = xmalloc(64k + mtu) > > 4) free the first dp_packet, and use the second dp_packet > > I did a quick experiment while at the conference and Ben's idea is > indeed a bit faster (2.7%) when the packet is not resized due to #1. > > If the buffer gets resized to what's actually used, then it becomes > a bit slower (1.8%).
Do we have to resize it? > > Anyways, feel free to have a look at the code[1]. Perhaps it could > be changed to be more efficient. Just send me a patch and I will be > happy to test again. > > [1] https://github.com/fleitner/ovs/tree/tso-cycles-ben Thanks! I tested it by applying https://github.com/fleitner/ovs/commit/f0f5f630645134bf3c46201de8ce3f44e4fd2c03 Implemented Ben suggestion. Signed-off-by: Flavio Leitner <[email protected]> Using iperf3 -c (ns0) -> veth peer -> OVS -> veth peer -> iperf3 -s (ns1) Test 100 second TCP without the patch [ 3] 0.0-100.0 sec 78.8 GBytes 6.77 Gbits/sec with the patch [ 3] 0.0-100.0 sec 94.5 GBytes 8.11 Gbits/sec I think it's pretty good improvement! Regards, William _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
