On Mon, Mar 23, 2020 at 06:54:29PM +0100, Ilya Maximets wrote: > On 3/20/20 9:52 PM, William Tu wrote: > > On Fri, Mar 20, 2020 at 7:32 AM Ilya Maximets <[email protected]> wrote: > >> > >> 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 <[email protected]> > >>> --- > >>> v8: > >>> - 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 > >>> https://travis-ci.org/github/williamtu/ovs-travis/builds/661607017 > >>> > >>> v4: > >>> - 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) > >>> https://travis-ci.org/github/williamtu/ovs-travis/builds/661313463 > >>> 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 > >>> > >>> v3: > >>> - fix comments and some coding style > >>> - add valgrind check > >>> - travis: https://travis-ci.org/williamtu/ovs-travis/builds/660394007 > >>> v2: > >>> - 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; > >>> -#endif > >>> + *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' > >> wile > >> 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 > >> assign it. > >> > >> 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. > > > > OK. > > Another thing is that in the enum dp_packet_offload_mask, most of the > > members > > have only 1-bit, but only the DP_PACKET_OL_NONE_MASK has multiple bits. > > So I decided to move it out of enum dp_packet_offload_mask, just like other > > macros such as DP_PACKET_OL_TX_L4_MASK. > > > > So how about this diff to the current patch, using > > DP_PACKET_OL_SUPPORTED_MASK > > > > diff --git a/lib/dp-packet.c b/lib/dp-packet.c > > index e154ac13e4f2..72f6d09ac7f3 100644 > > --- a/lib/dp-packet.c > > +++ b/lib/dp-packet.c > > @@ -193,7 +193,7 @@ dp_packet_clone_with_headroom(const struct > > dp_packet *buffer, size_t headroom) > > offsetof(struct dp_packet, l2_pad_size)); > > > > *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; > > + *dp_packet_ol_flags_ptr(new_buffer) &= DP_PACKET_OL_SUPPORTED_MASK; > > > > if (dp_packet_rss_valid(buffer)) { > > dp_packet_set_rss_hash(new_buffer, dp_packet_get_rss_hash(buffer)); > > diff --git a/lib/dp-packet.h b/lib/dp-packet.h > > index ed277719c15e..f0f30892f407 100644 > > --- a/lib/dp-packet.h > > +++ b/lib/dp-packet.h > > @@ -49,8 +49,10 @@ enum OVS_PACKED_ENUM dp_packet_source { > > #ifdef DPDK_NETDEV > > /* DPDK mbuf ol_flags that are not really an offload flags. These are > > mostly > > * related to mbuf memory layout and OVS should not touch/clear them. */ > > -#define DPDK_MBUF_NON_OFFLOADING_MASK (EXT_ATTACHED_MBUF | \ > > - IND_ATTACHED_MBUF) > > +#define DP_PACKET_OL_SUPPORTED_MASK ~(EXT_ATTACHED_MBUF | \ > > + IND_ATTACHED_MBUF) > > The key point is to clear "only flags that we're really using", > not the "all flags except couple that we know we should not clear". > i.e. DP_PACKET_OL_SUPPORTED_MASK should have only bits covered by > enum dp_packet_offload_mask. > This could be done by literally 'or'ing of all the flags, or by > another MACRO trick.
I see, thanks! Let me work on another version William _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
