On 12 Jul 2022, at 11:14, Ilya Maximets wrote:
> On 6/10/22 16:37, Ilya Maximets wrote:
>> On 6/3/22 10:54, Eelco Chaudron wrote:
>>> This series adds support for the datapath action check_pkt_len for TC
>>> offload.
>>> It also includes some offload self-tests.
>>
>> Thanks for working on this, Eelco.
>> It is a nice feature and patches generally look good to me.
>>
>> There is one problem I noticed though with the act_police:
>> When mtu check is used with act_police , GSO is fully broken
>> in software TC datapath. Meaning that only packets that
>> are actually smaller than MTU can pass through the interface.
>> For TCP it means a huge performance degradation. In my testing,
>> iperf between 2 namespaces via veth pair normally gives
>> around 10 Gbps, while if the act_police with mtu 1514 is
>> configured on a veth pair, I can only get 15 Mbps.
>>
>> The issue was fixed in the most recent kernel with commit:
>>
>> commit 4ddc844eb81da59bfb816d8d52089aba4e59e269
>> Author: Davide Caratti <[email protected]>
>> Date: Thu Feb 10 18:56:08 2022 +0100
>>
>> net/sched: act_police: more accurate MTU policing
>>
>> in current Linux, MTU policing does not take into account that packets at
>> the TC ingress have the L2 header pulled. Thus, the same TC police action
>> (with the same value of tcfp_mtu) behaves differently for ingress/egress.
>> In addition, the full GSO size is compared to tcfp_mtu: as a consequence,
>> the policer drops GSO packets even when individual segments have the L2 +
>> L3 + L4 + payload length below the configured valued of tcfp_mtu.
>>
>> Improve the accuracy of MTU policing as follows:
>> - account for mac_len for non-GSO packets at TC ingress.
>> - compare MTU threshold with the segmented size for GSO packets.
>> Also, add a kselftest that verifies the correct behavior.
>>
>> Signed-off-by: Davide Caratti <[email protected]>
>> Reviewed-by: Marcelo Ricardo Leitner <[email protected]>
>> Signed-off-by: David S. Miller <[email protected]>
>>
>>
>> However, that commit was not backported to stable kernels
>> and not available in major distributions as a result.
>>
>> That makes me uncomfortable with adding use of this action
>> to OVS for a few reasons:
>>
>> - If the flow will not be fully offloaded to the hardware,
>> the TCP performance will suffer and will be dramatically
>> lower than just using OVS kernel datapath.
>>
>> - It's not possible to detect the fix in the kernel, so
>> there is no way to selectively disable only this one
>> feature. Users will have to choose between poor
>> performance in certain cases and disabling the HW offload
>> entirely.
>>
>> I guess, we could create a user-configurable option to enable
>> or disable offloading of this one action, but that doesn't
>> sound like a great solution and really not user-friendly.
>>
>> @Davide, do you think we can have your fix backported to
>> stable kernels? That would be much easier to work with.
>>
>> Any other thoughts on how to deal with the situation here
>> are welcome.
>
> So, the kernel fix got backported to stable kernels in upstream.
> And it seems to be slowly getting into distributions. So, I guess,
> it should be OK to accept the change now.
>
> Eelco, could you, please, rebase this patch set on top of the
> current master branch?
The meter patch that just went in is using the same TC action, so I need some
small refactoring on the patchset.
I will try to do it today, or latest tomorrow.
//Eelco
>>
>> Best regards, Ilya Maximets.
>>
>>>
>>> v2:
>>> - Add ACKs
>>> - Unified all the OVS_TRAFFIC_VSWITCHD_START macro's
>>> - Added section in the NEWS document
>>> v3:
>>> - Add ACKs
>>> - Was using TCA_CSUM_PARMS instead of TCA_POLICE_TBF
>>> v4:
>>> - Add ACKs
>>> - Use the existing dbinit-aux-args argument, rather than
>>> creating a new pre-vswitchd command option.
>>> - Removed ACKs for patch 4/5
>>>
>>> Eelco Chaudron (5):
>>> netdev-offload-tc: Move flow_put action handling to isolated function.
>>> netdev-offload-tc: Move flower_to_match action handling to isolated
>>> function.
>>> netdev-offload-tc: Handle check_pkt_len datapath action.
>>> system-offloads-traffic: Properly initialize offload before testing.
>>> tests: Add check_pkt_len action test to system-offload-traffic.
>>>
>>>
>>> NEWS | 2 +
>>> lib/netdev-offload-tc.c | 844 +++++++++++++++++++------------
>>> lib/tc.c | 455 +++++++++++++++--
>>> lib/tc.h | 12 +-
>>> tests/ofproto-macros.at | 3 +-
>>> tests/system-kmod-macros.at | 4 +-
>>> tests/system-offloads-traffic.at | 422 +++++++++++++++-
>>> tests/system-tso-macros.at | 4 +-
>>> tests/system-userspace-macros.at | 4 +-
>>> 9 files changed, 1376 insertions(+), 374 deletions(-)
>>>
>>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
