[ovs-dev] [PATCHv8 1/2] userspace: Enable TSO support for non-DPDK.

William Tu u9012063 at gmail.com
Fri Mar 20 20:52:47 UTC 2020


On Fri, Mar 20, 2020 at 7:32 AM Ilya Maximets <i.maximets at ovn.org> 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 <u9012063 at gmail.com>
> > ---
> > 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)
+#else
+#define DP_PACKET_OL_SUPPORTED_MASK ~0x8ff
 #endif

 #define DP_PACKET_CONTEXT_SIZE 64
@@ -62,8 +64,7 @@ enum OVS_PACKED_ENUM dp_packet_source {

 /* Bit masks for the 'ol_flags' member of the 'dp_packet' structure. */
 enum dp_packet_offload_mask {
-    /* Reset offload. */
-    DEF_OL_FLAG(DP_PACKET_OL_NONE_MASK, DPDK_MBUF_NON_OFFLOADING_MASK, 0x0),
+    /* Value 0 is not used. */
     /* Is the 'rss_hash' valid? */
     DEF_OL_FLAG(DP_PACKET_OL_RSS_HASH, PKT_RX_RSS_HASH, 0x1),
     /* Is the 'flow_mark' valid? (DPDK does not support) */
@@ -905,7 +906,7 @@ dp_packet_rss_valid(const struct dp_packet *p)
 static inline void
 dp_packet_reset_offload(struct dp_packet *p)
 {
-    *dp_packet_ol_flags_ptr(p) &= DP_PACKET_OL_NONE_MASK;
+    *dp_packet_ol_flags_ptr(p) &= ~DP_PACKET_OL_SUPPORTED_MASK;
 }

 static inline bool


More information about the dev mailing list