[ovs-dev] [PATCHv6 1/2] userspace: Enable TSO support for non-DPDK.
William Tu
u9012063 at gmail.com
Tue Mar 17 19:52:49 UTC 2020
On Sat, Mar 14, 2020 at 07:40:06PM -0300, Flavio Leitner wrote:
> On Thu, Mar 12, 2020 at 12:21:56PM -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 at gmail.com>
> > ---
> > 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.h | 397 ++++++++++++++++++------------------------
> > lib/userspace-tso.c | 5 -
> > tests/.gitignore | 3 +
> > tests/automake.mk | 21 +++
> > tests/system-tso-macros.at | 31 ++++
> > tests/system-tso-testsuite.at | 26 +++
> > 6 files changed, 254 insertions(+), 229 deletions(-)
> > create mode 100644 tests/system-tso-macros.at
> > create mode 100644 tests/system-tso-testsuite.at
> >
> > diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> > index 9f8991faad52..dce4b26f793b 100644
> > --- a/lib/dp-packet.h
> > +++ b/lib/dp-packet.h
> > @@ -47,14 +47,49 @@ enum OVS_PACKED_ENUM dp_packet_source {
> > };
> >
> > #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? */
> > + /* Is the 'rss_hash' valid? */
> > + DEF_OL_FLAG(DP_PACKET_OL_RSS_HASH_MASK, PKT_RX_RSS_HASH, 0x1),
> > + /* Is the 'flow_mark' valid? (DPDK does not support) */
> > + DEF_OL_FLAG(DP_PACKET_OL_FLOW_MARK_MASK, 0x0, 0x2),
>
> Sorry I didn't pick this up in the previous reviews. It seems that
> the above could be PKT_RX_FDIR_ID for DPDK and we could have a generic
> API for DPDK and non-DPDK as well, otherwise flow_mark becomes an
> exception:
>
> [...]
> DEF_OL_FLAG(DP_PACKET_OL_FLOW_MARK_MASK, PKT_RX_FDIR_ID, 0x2),
Thank you, my mistake not handling flow_mark and fdir.
>
> #ifdef DPDK_NETDEV
> 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_flow_mark_ptr(const struct dp_packet *b)
> {
> return CONST_CAST(uint32_t *, &b->flow_mark);
> }
> #endif
>
> 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_MASK) {
> *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_MASK;
> }
Good idea.
>
>
> > + /* 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
> > +
> > +#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)
> > +
> > +#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_FLAGS (EXT_ATTACHED_MBUF | \
> > @@ -451,6 +486,20 @@ 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);
> > +}
> > +#else
> > +static inline uint32_t *
> > +dp_packet_ol_flags_ptr(const struct dp_packet *b)
> > +{
> > + return CONST_CAST(uint32_t *, &b->ol_flags);
> > +}
> > +#endif
>
>
> And since you're adding those above, I think we should update other
> functions accessing that field as well, like:
> dp_packet_set_rss_hash()
also dp_packet_get_rss_hash()
> dp_packet_rss_valid()
> dp_packet_reset_offload()
> dp_packet_clone_with_headroom()
>
> What do you think?
Thanks!
I think now all the offload related functions
are in one place instead of two in #if DPDK_NETDEV #else.
I will work on the next version.
William
More information about the dev
mailing list