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

Ilya Maximets i.maximets at ovn.org
Mon Mar 23 17:54:29 UTC 2020


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

The key point is to clear "only flags that we're really using",
not the "all flags except couple that we know we should not clear".
i.e. DP_PACKET_OL_SUPPORTED_MASK should have only bits covered by
enum dp_packet_offload_mask.
This could be done by literally 'or'ing of all the flags, or by
another MACRO trick.

> +#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
> 



More information about the dev mailing list