On 3/20/20 9:52 PM, William Tu wrote:
> 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)
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
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
