[ovs-dev] [PATCHv8 1/2] userspace: Enable TSO support for non-DPDK.
i.maximets at ovn.org
Fri Mar 20 14:32:09 UTC 2020
On 3/20/20 12:56 AM, William Tu wrote:
> This patch enables TSO support for non-DPDK use cases, and
> also add check-system-tso testsuite. Before TSO, we have to
> disable checksum offload, allowing the kernel to calculate the
> TCP/UDP packet checsum. With TSO, we can skip the checksum
> validation by enabling checksum offload, and with large packet
> size, we see better performance.
> Consider container to container use cases:
> iperf3 -c (ns0) -> veth peer -> OVS -> veth peer -> iperf3 -s (ns1)
> And I got around 6Gbps, similar to TSO with DPDK-enabled.
> Signed-off-by: William Tu <u9012063 at gmail.com>
> - make some namings more clear
> v7: more refactoring on functions
> - rss and flow mark related functions.
> - dp_packet_clone_with_headroom
> - fix definitions of DP_PACKET_OL_FLOW_MARK_MASK
> - travis: https://travis-ci.org/github/williamtu/ovs-travis/builds/663658338
> v6: fix indentation
> v5: feedback from Flavio
> - move some code around, break the long line
> - travis is now working
> - Avoid duplications of DPDK and non-DPDK code by
> refactoring the definition of DP_PACKET_OL flags
> and relevant functions
> - I got weird error in travis (I think this is unrelated)
> sindex.c:378:26: error: unknown type name ‘sqlite3_str’
> static int query_appendf(sqlite3_str *query, const char *fmt, ...)
> - test compile ok on dpdk and non-dpdk, make check-system-tso still
> has a couple fails
> - fix comments and some coding style
> - add valgrind check
> - travis: https://travis-ci.org/williamtu/ovs-travis/builds/660394007
> - add make check-system-tso test
> - combine logging for dpdk and non-dpdk
> - I'm surprised that most of the test cases passed.
> This is due to few tests using tcp/udp, so it does not trigger
> TSO. I saw only geneve/vxlan fails randomly, maybe we can
> check it later.
> lib/dp-packet.c | 6 +-
> lib/dp-packet.h | 566 +++++++++++++++++++-----------------------
> lib/userspace-tso.c | 5 -
> tests/.gitignore | 3 +
> tests/automake.mk | 21 ++
> tests/system-tso-macros.at | 31 +++
> tests/system-tso-testsuite.at | 26 ++
> 7 files changed, 333 insertions(+), 325 deletions(-)
> create mode 100644 tests/system-tso-macros.at
> create mode 100644 tests/system-tso-testsuite.at
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index cd2623500e3d..e154ac13e4f2 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -192,10 +192,8 @@ dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t headroom)
> sizeof(struct dp_packet) -
> offsetof(struct dp_packet, l2_pad_size));
> -#ifdef DPDK_NETDEV
> - new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
> - new_buffer->mbuf.ol_flags &= ~DPDK_MBUF_NON_OFFLOADING_FLAGS;
> + *dp_packet_ol_flags_ptr(new_buffer) = *dp_packet_ol_flags_ptr(buffer);
> + *dp_packet_ol_flags_ptr(new_buffer) &= ~DP_PACKET_OL_NONE_MASK;
I didn't finish reading the patch yet, but the name DP_PACKET_OL_NONE_MASK sounds
confusing to me. What I mean:
OL_NONE_MASK sounds like 'offloading none' --> 'no any ofloading'
NON_OFFLOADING_FLAGS --> flags that are present, but not related to offloading.
The negative name clearly states that there are some flags that we don't want
to clear. OL_NONE says that we're clearing all the offloading flags without
paying any attention to the fact that there are things that we don't want to clear.
It's not clear from the name that we should do ~ and & to this mask and not just
I don't think that my explanation makes any sense, though. :)
Anyway, since you've started to change not really related things, it might
make sense to create a positive mask instead of negative one, i.e.
DP_PACKET_OL_SUPPORTED/KNOWN/USED_MASK and clear only these flags on offloading reset.
This is possible right now since we removed support for dpdk ring (dpdkr) ports
and we don't need to clear flags that we do not use.
We need to do this anyway in this or in the follow up patch.
Relevant discussion: https://patchwork.ozlabs.org/patch/1198954/
Best regards, Ilya Maximets.
More information about the dev