On Fri, Mar 20, 2020 at 7:32 AM Ilya Maximets <[email protected]> wrote:
>
> On 3/20/20 12:56 AM, William Tu wrote:
> > This patch enables TSO support for non-DPDK use cases, and
> > also add check-system-tso testsuite. Before TSO, we have to
> > disable checksum offload, allowing the kernel to calculate the
> > TCP/UDP packet checsum. With TSO, we can skip the checksum
> > validation by enabling checksum offload, and with large packet
> > size, we see better performance.
> >
> > Consider container to container use cases:
> > iperf3 -c (ns0) -> veth peer -> OVS -> veth peer -> iperf3 -s (ns1)
> > And I got around 6Gbps, similar to TSO with DPDK-enabled.
> >
> > Signed-off-by: William Tu <[email protected]>
> > ---
> > v8:
> > - make some namings more clear
> >
> > v7: more refactoring on functions
> > - rss and flow mark related functions.
> > - dp_packet_clone_with_headroom
> > - fix definitions of DP_PACKET_OL_FLOW_MARK_MASK
> > - travis:
> > https://travis-ci.org/github/williamtu/ovs-travis/builds/663658338
> >
> > v6: fix indentation
> >
> > v5: feedback from Flavio
> > - move some code around, break the long line
> > - travis is now working
> > https://travis-ci.org/github/williamtu/ovs-travis/builds/661607017
> >
> > v4:
> > - Avoid duplications of DPDK and non-DPDK code by
> > refactoring the definition of DP_PACKET_OL flags
> > and relevant functions
> > - I got weird error in travis (I think this is unrelated)
> > https://travis-ci.org/github/williamtu/ovs-travis/builds/661313463
> > sindex.c:378:26: error: unknown type name ‘sqlite3_str’
> > static int query_appendf(sqlite3_str *query, const char *fmt, ...)
> > - test compile ok on dpdk and non-dpdk, make check-system-tso still
> > has a couple fails
> >
> > v3:
> > - fix comments and some coding style
> > - add valgrind check
> > - travis: https://travis-ci.org/williamtu/ovs-travis/builds/660394007
> > v2:
> > - add make check-system-tso test
> > - combine logging for dpdk and non-dpdk
> > - I'm surprised that most of the test cases passed.
> > This is due to few tests using tcp/udp, so it does not trigger
> > TSO. I saw only geneve/vxlan fails randomly, maybe we can
> > check it later.
> > ---
> > lib/dp-packet.c | 6 +-
> > lib/dp-packet.h | 566
> > +++++++++++++++++++-----------------------
> > lib/userspace-tso.c | 5 -
> > tests/.gitignore | 3 +
> > tests/automake.mk | 21 ++
> > tests/system-tso-macros.at | 31 +++
> > tests/system-tso-testsuite.at | 26 ++
> > 7 files changed, 333 insertions(+), 325 deletions(-)
> > create mode 100644 tests/system-tso-macros.at
> > create mode 100644 tests/system-tso-testsuite.at
> >
> > diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> > index cd2623500e3d..e154ac13e4f2 100644
> > --- a/lib/dp-packet.c
> > +++ b/lib/dp-packet.c
> > @@ -192,10 +192,8 @@ dp_packet_clone_with_headroom(const struct dp_packet
> > *buffer, size_t headroom)
> > sizeof(struct dp_packet) -
> > offsetof(struct dp_packet, l2_pad_size));
> >
> > -#ifdef DPDK_NETDEV
> > - new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
> > - new_buffer->mbuf.ol_flags &= ~DPDK_MBUF_NON_OFFLOADING_FLAGS;
> > -#endif
> > + *dp_packet_ol_flags_ptr(new_buffer) = *dp_packet_ol_flags_ptr(buffer);
> > + *dp_packet_ol_flags_ptr(new_buffer) &= ~DP_PACKET_OL_NONE_MASK;
>
>
> I didn't finish reading the patch yet, but the name DP_PACKET_OL_NONE_MASK
> sounds
> confusing to me. What I mean:
>
> OL_NONE_MASK sounds like 'offloading none' --> 'no any ofloading'
> wile
> NON_OFFLOADING_FLAGS --> flags that are present, but not related to
> offloading.
>
> The negative name clearly states that there are some flags that we don't want
> to clear. OL_NONE says that we're clearing all the offloading flags without
> paying any attention to the fact that there are things that we don't want to
> clear.
>
> It's not clear from the name that we should do ~ and & to this mask and not
> just
> assign it.
>
> I don't think that my explanation makes any sense, though. :)
>
> Anyway, since you've started to change not really related things, it might
> make sense to create a positive mask instead of negative one, i.e.
> DP_PACKET_OL_SUPPORTED/KNOWN/USED_MASK and clear only these flags on
> offloading reset.
> This is possible right now since we removed support for dpdk ring (dpdkr)
> ports
> and we don't need to clear flags that we do not use.
> We need to do this anyway in this or in the follow up patch.
> Relevant discussion: https://patchwork.ozlabs.org/patch/1198954/
>
> Best regards, Ilya Maximets.
OK.
Another thing is that in the enum dp_packet_offload_mask, most of the members
have only 1-bit, but only the DP_PACKET_OL_NONE_MASK has multiple bits.
So I decided to move it out of enum dp_packet_offload_mask, just like other
macros such as DP_PACKET_OL_TX_L4_MASK.
So how about this diff to the current patch, using
DP_PACKET_OL_SUPPORTED_MASK
diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index e154ac13e4f2..72f6d09ac7f3 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -193,7 +193,7 @@ dp_packet_clone_with_headroom(const struct
dp_packet *buffer, size_t headroom)
offsetof(struct dp_packet, l2_pad_size));
*dp_packet_ol_flags_ptr(new_buffer) = *dp_packet_ol_flags_ptr(buffer);
- *dp_packet_ol_flags_ptr(new_buffer) &= ~DP_PACKET_OL_NONE_MASK;
+ *dp_packet_ol_flags_ptr(new_buffer) &= DP_PACKET_OL_SUPPORTED_MASK;
if (dp_packet_rss_valid(buffer)) {
dp_packet_set_rss_hash(new_buffer, dp_packet_get_rss_hash(buffer));
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index ed277719c15e..f0f30892f407 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -49,8 +49,10 @@ enum OVS_PACKED_ENUM dp_packet_source {
#ifdef DPDK_NETDEV
/* DPDK mbuf ol_flags that are not really an offload flags. These are mostly
* related to mbuf memory layout and OVS should not touch/clear them. */
-#define DPDK_MBUF_NON_OFFLOADING_MASK (EXT_ATTACHED_MBUF | \
- IND_ATTACHED_MBUF)
+#define DP_PACKET_OL_SUPPORTED_MASK ~(EXT_ATTACHED_MBUF | \
+ IND_ATTACHED_MBUF)
+#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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
