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

William Tu u9012063 at gmail.com
Tue Mar 24 14:58:30 UTC 2020


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 <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)
> 
> 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




More information about the dev mailing list