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

Message ID 1584662168-107175-1-git-send-email-u9012063@gmail.com
State New
Headers show
Series
  • [ovs-dev,PATCHv8,1/2] userspace: Enable TSO support for non-DPDK.
Related show

Commit Message

William Tu March 19, 2020, 11:56 p.m. UTC
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@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

Comments

Flavio Leitner March 20, 2020, 12:28 p.m. UTC | #1
On Thu, Mar 19, 2020 at 04:56:07PM -0700, 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@gmail.com>
> ---

To the set
Acked-by: Flavio Leitner <fbl@sysclose.org>

Thanks William
fbl
Ilya Maximets March 20, 2020, 2:32 p.m. UTC | #2
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@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.
William Tu March 20, 2020, 8:52 p.m. UTC | #3
On Fri, Mar 20, 2020 at 7:32 AM Ilya Maximets <i.maximets@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@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
Ilya Maximets March 23, 2020, 5:54 p.m. UTC | #4
On 3/20/20 9:52 PM, William Tu wrote:
> On Fri, Mar 20, 2020 at 7:32 AM Ilya Maximets <i.maximets@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@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.

> +#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
>
William Tu March 24, 2020, 2:58 p.m. UTC | #5
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@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@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

Patch
diff mbox series

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;
 
     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 9f8991faad52..ed277719c15e 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -46,20 +46,57 @@  enum OVS_PACKED_ENUM dp_packet_source {
     DPBUF_AFXDP,               /* Buffer data from XDP frame. */
 };
 
+#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)
+#endif
+
 #define DP_PACKET_CONTEXT_SIZE 64
+#ifdef DPDK_NETDEV
+#define DEF_OL_FLAG(NAME, DPDK_DEF, GENERIC_DEF) NAME = DPDK_DEF
+#else
+#define DEF_OL_FLAG(NAME, DPDK_DEF, GENERIC_DEF) NAME = GENERIC_DEF
+#endif
 
-#ifndef DPDK_NETDEV
 /* Bit masks for the 'ol_flags' member of the 'dp_packet' structure. */
 enum dp_packet_offload_mask {
-    DP_PACKET_OL_RSS_HASH_MASK  = 0x1, /* Is the 'rss_hash' valid? */
-    DP_PACKET_OL_FLOW_MARK_MASK = 0x2, /* Is the 'flow_mark' valid? */
+    /* Reset offload. */
+    DEF_OL_FLAG(DP_PACKET_OL_NONE_MASK, DPDK_MBUF_NON_OFFLOADING_MASK, 0x0),
+    /* 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) */
+    DEF_OL_FLAG(DP_PACKET_OL_FLOW_MARK, PKT_RX_FDIR_ID, 0x2),
+    /* Bad L4 checksum in the packet. */
+    DEF_OL_FLAG(DP_PACKET_OL_RX_L4_CKSUM_BAD, PKT_RX_L4_CKSUM_BAD, 0x4),
+    /* Bad IP checksum in the packet. */
+    DEF_OL_FLAG(DP_PACKET_OL_RX_IP_CKSUM_BAD, PKT_RX_IP_CKSUM_BAD, 0x8),
+    /* Valid L4 checksum in the packet. */
+    DEF_OL_FLAG(DP_PACKET_OL_RX_L4_CKSUM_GOOD, PKT_RX_L4_CKSUM_GOOD, 0x10),
+    /* Valid IP checksum in the packet. */
+    DEF_OL_FLAG(DP_PACKET_OL_RX_IP_CKSUM_GOOD, PKT_RX_IP_CKSUM_GOOD, 0x20),
+    /* TCP Segmentation Offload. */
+    DEF_OL_FLAG(DP_PACKET_OL_TX_TCP_SEG, PKT_TX_TCP_SEG, 0x40),
+    /* Offloaded packet is IPv4. */
+    DEF_OL_FLAG(DP_PACKET_OL_TX_IPV4, PKT_TX_IPV4, 0x80),
+    /* Offloaded packet is IPv6. */
+    DEF_OL_FLAG(DP_PACKET_OL_TX_IPV6, PKT_TX_IPV6, 0x100),
+    /* Offload TCP checksum. */
+    DEF_OL_FLAG(DP_PACKET_OL_TX_TCP_CKSUM, PKT_TX_TCP_CKSUM, 0x200),
+    /* Offload UDP checksum. */
+    DEF_OL_FLAG(DP_PACKET_OL_TX_UDP_CKSUM, PKT_TX_UDP_CKSUM, 0x400),
+    /* Offload SCTP checksum. */
+    DEF_OL_FLAG(DP_PACKET_OL_TX_SCTP_CKSUM, PKT_TX_SCTP_CKSUM, 0x800),
 };
-#else
-/* 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_FLAGS (EXT_ATTACHED_MBUF | \
-                                        IND_ATTACHED_MBUF)
-#endif
+
+#define DP_PACKET_OL_TX_L4_MASK (DP_PACKET_OL_TX_TCP_CKSUM | \
+                                 DP_PACKET_OL_TX_UDP_CKSUM | \
+                                 DP_PACKET_OL_TX_SCTP_CKSUM)
+#define DP_PACKET_OL_RX_IP_CKSUM_MASK (DP_PACKET_OL_RX_IP_CKSUM_GOOD | \
+                                       DP_PACKET_OL_RX_IP_CKSUM_BAD)
+#define DP_PACKET_OL_RX_L4_CKSUM_MASK (DP_PACKET_OL_RX_L4_CKSUM_GOOD | \
+                                       DP_PACKET_OL_RX_L4_CKSUM_BAD)
 
 /* Buffer for holding packet data.  A dp_packet is automatically reallocated
  * as necessary if it grows too large for the available memory.
@@ -451,6 +488,45 @@  dp_packet_get_nd_payload(const struct dp_packet *b)
 }
 
 #ifdef DPDK_NETDEV
+static inline uint64_t *
+dp_packet_ol_flags_ptr(const struct dp_packet *b)
+{
+    return CONST_CAST(uint64_t *, &b->mbuf.ol_flags);
+}
+
+static inline uint32_t *
+dp_packet_rss_ptr(const struct dp_packet *b)
+{
+    return CONST_CAST(uint32_t *, &b->mbuf.hash.rss);
+}
+
+static inline uint32_t *
+dp_packet_flow_mark_ptr(const struct dp_packet *b)
+{
+    return CONST_CAST(uint32_t *, &b->mbuf.hash.fdir.hi);
+}
+
+#else
+static inline uint32_t *
+dp_packet_ol_flags_ptr(const struct dp_packet *b)
+{
+    return CONST_CAST(uint32_t *, &b->ol_flags);
+}
+
+static inline uint32_t *
+dp_packet_rss_ptr(const struct dp_packet *b)
+{
+    return CONST_CAST(uint32_t *, &b->rss_hash);
+}
+
+static inline uint32_t *
+dp_packet_flow_mark_ptr(const struct dp_packet *b)
+{
+    return CONST_CAST(uint32_t *, &b->flow_mark);
+}
+#endif
+
+#ifdef DPDK_NETDEV
 BUILD_ASSERT_DECL(offsetof(struct dp_packet, mbuf) == 0);
 
 static inline void
@@ -521,168 +597,6 @@  dp_packet_set_allocated(struct dp_packet *b, uint16_t s)
     b->mbuf.buf_len = s;
 }
 
-/* Returns 'true' if packet 'b' is marked for TCP segmentation offloading. */
-static inline bool
-dp_packet_hwol_is_tso(const struct dp_packet *b)
-{
-    return !!(b->mbuf.ol_flags & PKT_TX_TCP_SEG);
-}
-
-/* Returns 'true' if packet 'b' is marked for IPv4 checksum offloading. */
-static inline bool
-dp_packet_hwol_is_ipv4(const struct dp_packet *b)
-{
-    return !!(b->mbuf.ol_flags & PKT_TX_IPV4);
-}
-
-/* Returns the L4 cksum offload bitmask. */
-static inline uint64_t
-dp_packet_hwol_l4_mask(const struct dp_packet *b)
-{
-    return b->mbuf.ol_flags & PKT_TX_L4_MASK;
-}
-
-/* Returns 'true' if packet 'b' is marked for TCP checksum offloading. */
-static inline bool
-dp_packet_hwol_l4_is_tcp(const struct dp_packet *b)
-{
-    return (b->mbuf.ol_flags & PKT_TX_L4_MASK) == PKT_TX_TCP_CKSUM;
-}
-
-/* Returns 'true' if packet 'b' is marked for UDP checksum offloading. */
-static inline bool
-dp_packet_hwol_l4_is_udp(struct dp_packet *b)
-{
-    return (b->mbuf.ol_flags & PKT_TX_L4_MASK) == PKT_TX_UDP_CKSUM;
-}
-
-/* Returns 'true' if packet 'b' is marked for SCTP checksum offloading. */
-static inline bool
-dp_packet_hwol_l4_is_sctp(struct dp_packet *b)
-{
-    return (b->mbuf.ol_flags & PKT_TX_L4_MASK) == PKT_TX_SCTP_CKSUM;
-}
-
-/* Mark packet 'b' for IPv4 checksum offloading. */
-static inline void
-dp_packet_hwol_set_tx_ipv4(struct dp_packet *b)
-{
-    b->mbuf.ol_flags |= PKT_TX_IPV4;
-}
-
-/* Mark packet 'b' for IPv6 checksum offloading. */
-static inline void
-dp_packet_hwol_set_tx_ipv6(struct dp_packet *b)
-{
-    b->mbuf.ol_flags |= PKT_TX_IPV6;
-}
-
-/* Mark packet 'b' for TCP checksum offloading.  It implies that either
- * the packet 'b' is marked for IPv4 or IPv6 checksum offloading. */
-static inline void
-dp_packet_hwol_set_csum_tcp(struct dp_packet *b)
-{
-    b->mbuf.ol_flags |= PKT_TX_TCP_CKSUM;
-}
-
-/* Mark packet 'b' for UDP checksum offloading.  It implies that either
- * the packet 'b' is marked for IPv4 or IPv6 checksum offloading. */
-static inline void
-dp_packet_hwol_set_csum_udp(struct dp_packet *b)
-{
-    b->mbuf.ol_flags |= PKT_TX_UDP_CKSUM;
-}
-
-/* Mark packet 'b' for SCTP checksum offloading.  It implies that either
- * the packet 'b' is marked for IPv4 or IPv6 checksum offloading. */
-static inline void
-dp_packet_hwol_set_csum_sctp(struct dp_packet *b)
-{
-    b->mbuf.ol_flags |= PKT_TX_SCTP_CKSUM;
-}
-
-/* Mark packet 'b' for TCP segmentation offloading.  It implies that
- * either the packet 'b' is marked for IPv4 or IPv6 checksum offloading
- * and also for TCP checksum offloading. */
-static inline void
-dp_packet_hwol_set_tcp_seg(struct dp_packet *b)
-{
-    b->mbuf.ol_flags |= PKT_TX_TCP_SEG;
-}
-
-/* Returns the RSS hash of the packet 'p'.  Note that the returned value is
- * correct only if 'dp_packet_rss_valid(p)' returns true */
-static inline uint32_t
-dp_packet_get_rss_hash(const struct dp_packet *p)
-{
-    return p->mbuf.hash.rss;
-}
-
-static inline void
-dp_packet_set_rss_hash(struct dp_packet *p, uint32_t hash)
-{
-    p->mbuf.hash.rss = hash;
-    p->mbuf.ol_flags |= PKT_RX_RSS_HASH;
-}
-
-static inline bool
-dp_packet_rss_valid(const struct dp_packet *p)
-{
-    return p->mbuf.ol_flags & PKT_RX_RSS_HASH;
-}
-
-static inline void
-dp_packet_reset_offload(struct dp_packet *p)
-{
-    p->mbuf.ol_flags &= DPDK_MBUF_NON_OFFLOADING_FLAGS;
-}
-
-static inline bool
-dp_packet_ip_checksum_valid(const struct dp_packet *p)
-{
-    return (p->mbuf.ol_flags & PKT_RX_IP_CKSUM_MASK) ==
-            PKT_RX_IP_CKSUM_GOOD;
-}
-
-static inline bool
-dp_packet_ip_checksum_bad(const struct dp_packet *p)
-{
-    return (p->mbuf.ol_flags & PKT_RX_IP_CKSUM_MASK) ==
-            PKT_RX_IP_CKSUM_BAD;
-}
-
-static inline bool
-dp_packet_l4_checksum_valid(const struct dp_packet *p)
-{
-    return (p->mbuf.ol_flags & PKT_RX_L4_CKSUM_MASK) ==
-            PKT_RX_L4_CKSUM_GOOD;
-}
-
-static inline bool
-dp_packet_l4_checksum_bad(const struct dp_packet *p)
-{
-    return (p->mbuf.ol_flags & PKT_RX_L4_CKSUM_MASK) ==
-            PKT_RX_L4_CKSUM_BAD;
-}
-
-static inline bool
-dp_packet_has_flow_mark(const struct dp_packet *p, uint32_t *mark)
-{
-    if (p->mbuf.ol_flags & PKT_RX_FDIR_ID) {
-        *mark = p->mbuf.hash.fdir.hi;
-        return true;
-    }
-
-    return false;
-}
-
-static inline void
-dp_packet_set_flow_mark(struct dp_packet *p, uint32_t mark)
-{
-    p->mbuf.hash.fdir.hi = mark;
-    p->mbuf.ol_flags |= PKT_RX_FDIR_ID;
-}
-
 #else /* DPDK_NETDEV */
 
 static inline void
@@ -739,151 +653,6 @@  dp_packet_set_allocated(struct dp_packet *b, uint16_t s)
     b->allocated_ = s;
 }
 
-/* There are no implementation when not DPDK enabled datapath. */
-static inline bool
-dp_packet_hwol_is_tso(const struct dp_packet *b OVS_UNUSED)
-{
-    return false;
-}
-
-/* There are no implementation when not DPDK enabled datapath. */
-static inline bool
-dp_packet_hwol_is_ipv4(const struct dp_packet *b OVS_UNUSED)
-{
-    return false;
-}
-
-/* There are no implementation when not DPDK enabled datapath. */
-static inline uint64_t
-dp_packet_hwol_l4_mask(const struct dp_packet *b OVS_UNUSED)
-{
-    return 0;
-}
-
-/* There are no implementation when not DPDK enabled datapath. */
-static inline bool
-dp_packet_hwol_l4_is_tcp(const struct dp_packet *b OVS_UNUSED)
-{
-    return false;
-}
-
-/* There are no implementation when not DPDK enabled datapath. */
-static inline bool
-dp_packet_hwol_l4_is_udp(const struct dp_packet *b OVS_UNUSED)
-{
-    return false;
-}
-
-/* There are no implementation when not DPDK enabled datapath. */
-static inline bool
-dp_packet_hwol_l4_is_sctp(const struct dp_packet *b OVS_UNUSED)
-{
-    return false;
-}
-
-/* There are no implementation when not DPDK enabled datapath. */
-static inline void
-dp_packet_hwol_set_tx_ipv4(struct dp_packet *b OVS_UNUSED)
-{
-}
-
-/* There are no implementation when not DPDK enabled datapath. */
-static inline void
-dp_packet_hwol_set_tx_ipv6(struct dp_packet *b OVS_UNUSED)
-{
-}
-
-/* There are no implementation when not DPDK enabled datapath. */
-static inline void
-dp_packet_hwol_set_csum_tcp(struct dp_packet *b OVS_UNUSED)
-{
-}
-
-/* There are no implementation when not DPDK enabled datapath. */
-static inline void
-dp_packet_hwol_set_csum_udp(struct dp_packet *b OVS_UNUSED)
-{
-}
-
-/* There are no implementation when not DPDK enabled datapath. */
-static inline void
-dp_packet_hwol_set_csum_sctp(struct dp_packet *b OVS_UNUSED)
-{
-}
-
-/* There are no implementation when not DPDK enabled datapath. */
-static inline void
-dp_packet_hwol_set_tcp_seg(struct dp_packet *b OVS_UNUSED)
-{
-}
-
-/* Returns the RSS hash of the packet 'p'.  Note that the returned value is
- * correct only if 'dp_packet_rss_valid(p)' returns true */
-static inline uint32_t
-dp_packet_get_rss_hash(const struct dp_packet *p)
-{
-    return p->rss_hash;
-}
-
-static inline void
-dp_packet_set_rss_hash(struct dp_packet *p, uint32_t hash)
-{
-    p->rss_hash = hash;
-    p->ol_flags |= DP_PACKET_OL_RSS_HASH_MASK;
-}
-
-static inline bool
-dp_packet_rss_valid(const struct dp_packet *p)
-{
-    return p->ol_flags & DP_PACKET_OL_RSS_HASH_MASK;
-}
-
-static inline void
-dp_packet_reset_offload(struct dp_packet *p)
-{
-    p->ol_flags = 0;
-}
-
-static inline bool
-dp_packet_ip_checksum_valid(const struct dp_packet *p OVS_UNUSED)
-{
-    return false;
-}
-
-static inline bool
-dp_packet_ip_checksum_bad(const struct dp_packet *p OVS_UNUSED)
-{
-    return false;
-}
-
-static inline bool
-dp_packet_l4_checksum_valid(const struct dp_packet *p OVS_UNUSED)
-{
-    return false;
-}
-
-static inline bool
-dp_packet_l4_checksum_bad(const struct dp_packet *p OVS_UNUSED)
-{
-    return false;
-}
-
-static inline bool
-dp_packet_has_flow_mark(const struct dp_packet *p, uint32_t *mark)
-{
-    if (p->ol_flags & DP_PACKET_OL_FLOW_MARK_MASK) {
-        *mark = p->flow_mark;
-        return true;
-    }
-    return false;
-}
-
-static inline void
-dp_packet_set_flow_mark(struct dp_packet *p, uint32_t mark)
-{
-    p->flow_mark = mark;
-    p->ol_flags |= DP_PACKET_OL_FLOW_MARK_MASK;
-}
 #endif /* DPDK_NETDEV */
 
 static inline void
@@ -1112,6 +881,58 @@  dp_packet_batch_reset_cutlen(struct dp_packet_batch *batch)
     }
 }
 
+/* Returns the RSS hash of the packet 'p'.  Note that the returned value is
+ * correct only if 'dp_packet_rss_valid(p)' returns true */
+static inline uint32_t
+dp_packet_get_rss_hash(const struct dp_packet *p)
+{
+    return *dp_packet_rss_ptr(p);
+}
+
+static inline void
+dp_packet_set_rss_hash(struct dp_packet *p, uint32_t hash)
+{
+    *dp_packet_rss_ptr(p) = hash;
+    *dp_packet_ol_flags_ptr(p) |= DP_PACKET_OL_RSS_HASH;
+}
+
+static inline bool
+dp_packet_rss_valid(const struct dp_packet *p)
+{
+    return *dp_packet_ol_flags_ptr(p) & DP_PACKET_OL_RSS_HASH;
+}
+
+static inline void
+dp_packet_reset_offload(struct dp_packet *p)
+{
+    *dp_packet_ol_flags_ptr(p) &= DP_PACKET_OL_NONE_MASK;
+}
+
+static inline bool
+dp_packet_has_flow_mark(const struct dp_packet *p, uint32_t *mark)
+{
+    if (*dp_packet_ol_flags_ptr(p) & DP_PACKET_OL_FLOW_MARK) {
+        *mark = *dp_packet_flow_mark_ptr(p);
+        return true;
+    }
+
+    return false;
+}
+
+static inline void
+dp_packet_set_flow_mark(struct dp_packet *p, uint32_t mark)
+{
+    *dp_packet_flow_mark_ptr(p) = mark;
+    *dp_packet_ol_flags_ptr(p) |= DP_PACKET_OL_FLOW_MARK;
+}
+
+/* Returns the L4 cksum offload bitmask. */
+static inline uint64_t
+dp_packet_hwol_l4_mask(const struct dp_packet *b)
+{
+    return *dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_L4_MASK;
+}
+
 /* Return true if the packet 'b' requested L4 checksum offload. */
 static inline bool
 dp_packet_hwol_tx_l4_checksum(const struct dp_packet *b)
@@ -1119,6 +940,119 @@  dp_packet_hwol_tx_l4_checksum(const struct dp_packet *b)
     return !!dp_packet_hwol_l4_mask(b);
 }
 
+/* Returns 'true' if packet 'b' is marked for TCP segmentation offloading. */
+static inline bool
+dp_packet_hwol_is_tso(const struct dp_packet *b)
+{
+    return !!(*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_TCP_SEG);
+}
+
+/* Returns 'true' if packet 'b' is marked for IPv4 checksum offloading. */
+static inline bool
+dp_packet_hwol_is_ipv4(const struct dp_packet *b)
+{
+    return !!(*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_IPV4);
+}
+
+/* Returns 'true' if packet 'b' is marked for TCP checksum offloading. */
+static inline bool
+dp_packet_hwol_l4_is_tcp(const struct dp_packet *b)
+{
+    return (*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_L4_MASK) ==
+            DP_PACKET_OL_TX_TCP_CKSUM;
+}
+
+/* Returns 'true' if packet 'b' is marked for UDP checksum offloading. */
+static inline bool
+dp_packet_hwol_l4_is_udp(struct dp_packet *b)
+{
+    return (*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_L4_MASK) ==
+            DP_PACKET_OL_TX_UDP_CKSUM;
+}
+
+/* Returns 'true' if packet 'b' is marked for SCTP checksum offloading. */
+static inline bool
+dp_packet_hwol_l4_is_sctp(struct dp_packet *b)
+{
+    return (*dp_packet_ol_flags_ptr(b) & DP_PACKET_OL_TX_L4_MASK) ==
+            DP_PACKET_OL_TX_SCTP_CKSUM;
+}
+
+/* Mark packet 'b' for IPv4 checksum offloading. */
+static inline void
+dp_packet_hwol_set_tx_ipv4(struct dp_packet *b)
+{
+    *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_IPV4;
+}
+
+/* Mark packet 'b' for IPv6 checksum offloading. */
+static inline void
+dp_packet_hwol_set_tx_ipv6(struct dp_packet *b)
+{
+    *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_IPV6;
+}
+
+/* Mark packet 'b' for TCP checksum offloading.  It implies that either
+ * the packet 'b' is marked for IPv4 or IPv6 checksum offloading. */
+static inline void
+dp_packet_hwol_set_csum_tcp(struct dp_packet *b)
+{
+    *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TCP_CKSUM;
+}
+
+/* Mark packet 'b' for UDP checksum offloading.  It implies that either
+ * the packet 'b' is marked for IPv4 or IPv6 checksum offloading. */
+static inline void
+dp_packet_hwol_set_csum_udp(struct dp_packet *b)
+{
+    *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_UDP_CKSUM;
+}
+
+/* Mark packet 'b' for SCTP checksum offloading.  It implies that either
+ * the packet 'b' is marked for IPv4 or IPv6 checksum offloading. */
+static inline void
+dp_packet_hwol_set_csum_sctp(struct dp_packet *b)
+{
+    *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_SCTP_CKSUM;
+}
+
+/* Mark packet 'b' for TCP segmentation offloading.  It implies that
+ * either the packet 'b' is marked for IPv4 or IPv6 checksum offloading
+ * and also for TCP checksum offloading. */
+static inline void
+dp_packet_hwol_set_tcp_seg(struct dp_packet *b)
+{
+    *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TCP_SEG;
+}
+
+static inline bool
+dp_packet_ip_checksum_valid(const struct dp_packet *p)
+{
+    return (*dp_packet_ol_flags_ptr(p) & DP_PACKET_OL_RX_IP_CKSUM_MASK) ==
+            DP_PACKET_OL_RX_IP_CKSUM_GOOD;
+}
+
+static inline bool
+dp_packet_ip_checksum_bad(const struct dp_packet *p)
+{
+    return (*dp_packet_ol_flags_ptr(p) & DP_PACKET_OL_RX_IP_CKSUM_MASK) ==
+            DP_PACKET_OL_RX_IP_CKSUM_BAD;
+}
+
+static inline bool
+dp_packet_l4_checksum_valid(const struct dp_packet *p)
+{
+    return (*dp_packet_ol_flags_ptr(p) & DP_PACKET_OL_RX_L4_CKSUM_MASK) ==
+            DP_PACKET_OL_RX_L4_CKSUM_GOOD;
+}
+
+static inline bool
+dp_packet_l4_checksum_bad(const struct dp_packet *p)
+{
+    return (*dp_packet_ol_flags_ptr(p) & DP_PACKET_OL_RX_L4_CKSUM_MASK) ==
+            DP_PACKET_OL_RX_L4_CKSUM_BAD;
+}
+
 #ifdef  __cplusplus
 }
 #endif
diff --git a/lib/userspace-tso.c b/lib/userspace-tso.c
index 6a4a0149b7f5..f843c2a763ce 100644
--- a/lib/userspace-tso.c
+++ b/lib/userspace-tso.c
@@ -34,13 +34,8 @@  userspace_tso_init(const struct smap *ovs_other_config)
         static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
 
         if (ovsthread_once_start(&once)) {
-#ifdef DPDK_NETDEV
             VLOG_INFO("Userspace TCP Segmentation Offloading support enabled");
             userspace_tso = true;
-#else
-            VLOG_WARN("Userspace TCP Segmentation Offloading can not be enabled"
-                      "since OVS is built without DPDK support.");
-#endif
             ovsthread_once_done(&once);
         }
     }
diff --git a/tests/.gitignore b/tests/.gitignore
index 99fdf70d58cd..45b4f67b2a43 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -24,6 +24,9 @@ 
 /system-userspace-testsuite
 /system-userspace-testsuite.dir/
 /system-userspace-testsuite.log
+/system-tso-testsuite
+/system-tso-testsuite.dir/
+/system-tso-testsuite.log
 /system-offloads-testsuite
 /system-offloads-testsuite.dir/
 /system-offloads-testsuite.log
diff --git a/tests/automake.mk b/tests/automake.mk
index 81eb2a9b8222..66859d5377c3 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -4,6 +4,7 @@  EXTRA_DIST += \
 	$(SYSTEM_TESTSUITE_AT) \
 	$(SYSTEM_KMOD_TESTSUITE_AT) \
 	$(SYSTEM_USERSPACE_TESTSUITE_AT) \
+	$(SYSTEM_TSO_TESTSUITE_AT) \
 	$(SYSTEM_AFXDP_TESTSUITE_AT) \
 	$(SYSTEM_OFFLOADS_TESTSUITE_AT) \
 	$(SYSTEM_DPDK_TESTSUITE_AT) \
@@ -11,6 +12,7 @@  EXTRA_DIST += \
 	$(TESTSUITE) \
 	$(SYSTEM_KMOD_TESTSUITE) \
 	$(SYSTEM_USERSPACE_TESTSUITE) \
+	$(SYSTEM_TSO_TESTSUITE) \
 	$(SYSTEM_AFXDP_TESTSUITE) \
 	$(SYSTEM_OFFLOADS_TESTSUITE) \
 	$(SYSTEM_DPDK_TESTSUITE) \
@@ -154,6 +156,10 @@  SYSTEM_USERSPACE_TESTSUITE_AT = \
 	tests/system-userspace-macros.at \
 	tests/system-userspace-packet-type-aware.at
 
+SYSTEM_TSO_TESTSUITE_AT = \
+	tests/system-tso-testsuite.at \
+	tests/system-tso-macros.at
+
 SYSTEM_AFXDP_TESTSUITE_AT = \
 	tests/system-userspace-macros.at \
 	tests/system-afxdp-testsuite.at \
@@ -183,6 +189,7 @@  TESTSUITE = $(srcdir)/tests/testsuite
 TESTSUITE_PATCH = $(srcdir)/tests/testsuite.patch
 SYSTEM_KMOD_TESTSUITE = $(srcdir)/tests/system-kmod-testsuite
 SYSTEM_USERSPACE_TESTSUITE = $(srcdir)/tests/system-userspace-testsuite
+SYSTEM_TSO_TESTSUITE = $(srcdir)/tests/system-tso-testsuite
 SYSTEM_AFXDP_TESTSUITE = $(srcdir)/tests/system-afxdp-testsuite
 SYSTEM_OFFLOADS_TESTSUITE = $(srcdir)/tests/system-offloads-testsuite
 SYSTEM_DPDK_TESTSUITE = $(srcdir)/tests/system-dpdk-testsuite
@@ -296,6 +303,12 @@  check-offloads-valgrind: all $(valgrind_wrappers) $(check_DATA)
 	@echo '----------------------------------------------------------------------'
 	@echo 'Valgrind output can be found in tests/system-offloads-testsuite.dir/*/valgrind.*'
 	@echo '----------------------------------------------------------------------'
+check-tso-valgrind: all $(valgrind_wrappers) $(check_DATA)
+	$(SHELL) '$(SYSTEM_TSO_TESTSUITE)' -C tests VALGRIND='$(VALGRIND)' AUTOTEST_PATH='tests/valgrind:$(AUTOTEST_PATH)' -d $(TESTSUITEFLAGS) -j1
+	@echo
+	@echo '----------------------------------------------------------------------'
+	@echo 'Valgrind output can be found in tests/system-tso-testsuite.dir/*/valgrind.*'
+	@echo '----------------------------------------------------------------------'
 check-helgrind: all $(valgrind_wrappers) $(check_DATA)
 	-$(SHELL) '$(TESTSUITE)' -C tests CHECK_VALGRIND=true VALGRIND='$(HELGRIND)' AUTOTEST_PATH='tests/valgrind:$(AUTOTEST_PATH)' -d $(TESTSUITEFLAGS)
 
@@ -326,6 +339,10 @@  check-system-userspace: all
 	set $(SHELL) '$(SYSTEM_USERSPACE_TESTSUITE)' -C tests  AUTOTEST_PATH='$(AUTOTEST_PATH)'; \
 	"$$@" $(TESTSUITEFLAGS) -j1 || (test X'$(RECHECK)' = Xyes && "$$@" --recheck)
 
+check-system-tso: all
+	set $(SHELL) '$(SYSTEM_TSO_TESTSUITE)' -C tests  AUTOTEST_PATH='$(AUTOTEST_PATH)'; \
+	"$$@" $(TESTSUITEFLAGS) -j1 || (test X'$(RECHECK)' = Xyes && "$$@" --recheck)
+
 check-afxdp: all
 	set $(SHELL) '$(SYSTEM_AFXDP_TESTSUITE)' -C tests  AUTOTEST_PATH='$(AUTOTEST_PATH)' $(TESTSUITEFLAGS) -j1; \
 	"$$@" || (test X'$(RECHECK)' = Xyes && "$$@" --recheck)
@@ -367,6 +384,10 @@  $(SYSTEM_USERSPACE_TESTSUITE): package.m4 $(SYSTEM_TESTSUITE_AT) $(SYSTEM_USERSP
 	$(AM_V_GEN)$(AUTOTEST) -I '$(srcdir)' -o $@.tmp $@.at
 	$(AM_V_at)mv $@.tmp $@
 
+$(SYSTEM_TSO_TESTSUITE): package.m4 $(SYSTEM_TESTSUITE_AT) $(SYSTEM_TSO_TESTSUITE_AT) $(COMMON_MACROS_AT)
+	$(AM_V_GEN)$(AUTOTEST) -I '$(srcdir)' -o $@.tmp $@.at
+	$(AM_V_at)mv $@.tmp $@
+
 $(SYSTEM_AFXDP_TESTSUITE): package.m4 $(SYSTEM_TESTSUITE_AT) $(SYSTEM_AFXDP_TESTSUITE_AT) $(COMMON_MACROS_AT)
 	$(AM_V_GEN)$(AUTOTEST) -I '$(srcdir)' -o $@.tmp $@.at
 	$(AM_V_at)mv $@.tmp $@
diff --git a/tests/system-tso-macros.at b/tests/system-tso-macros.at
new file mode 100644
index 000000000000..406334f3e081
--- /dev/null
+++ b/tests/system-tso-macros.at
@@ -0,0 +1,31 @@ 
+# _ADD_BR([name])
+#
+# Expands into the proper ovs-vsctl commands to create a bridge with the
+# appropriate type and properties
+m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 datapath_type="netdev" protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15 fail-mode=secure ]])
+
+# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [=override])
+#
+# Creates a database and starts ovsdb-server, starts ovs-vswitchd
+# connected to that database, calls ovs-vsctl to create a bridge named
+# br0 with predictable settings, passing 'vsctl-args' as additional
+# commands to ovs-vsctl.  If 'vsctl-args' causes ovs-vsctl to provide
+# output (e.g. because it includes "create" commands) then 'vsctl-output'
+# specifies the expected output after filtering through uuidfilt.
+m4_define([OVS_TRAFFIC_VSWITCHD_START],
+  [
+   OVS_WAIT_WHILE([ip link show ovs-netdev])
+   _OVS_VSWITCHD_START([--disable-system])
+   dnl Add bridges, ports, etc.
+   OVS_WAIT_WHILE([ip link show br0])
+   AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:userspace-tso-enable=true])
+   AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| uuidfilt])], [0], [$2])
+])
+
+# CONFIGURE_VETH_OFFLOADS([VETH])
+#
+# Enable TCP segmentation offload and scatter-gather for veths.
+m4_define([CONFIGURE_VETH_OFFLOADS],
+    [AT_CHECK([ethtool -K $1 sg on], [0], [ignore], [ignore])]
+    [AT_CHECK([ethtool -K $1 tso on], [0], [ignore], [ignore])]
+)
diff --git a/tests/system-tso-testsuite.at b/tests/system-tso-testsuite.at
new file mode 100644
index 000000000000..99d748006a86
--- /dev/null
+++ b/tests/system-tso-testsuite.at
@@ -0,0 +1,26 @@ 
+AT_INIT
+
+AT_COPYRIGHT([Copyright (c) 2020 VMware, Inc.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at:
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.])
+
+m4_ifdef([AT_COLOR_TESTS], [AT_COLOR_TESTS])
+
+m4_include([tests/ovs-macros.at])
+m4_include([tests/ovsdb-macros.at])
+m4_include([tests/ofproto-macros.at])
+m4_include([tests/system-common-macros.at])
+m4_include([tests/system-userspace-macros.at])
+m4_include([tests/system-tso-macros.at])
+
+m4_include([tests/system-traffic.at])