| Message ID | 1584662168-107175-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,PATCHv8,1/2] userspace: Enable TSO support for non-DPDK. | expand |
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
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.
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
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 >
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
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])
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