On 12 Jul 2022, at 12:50, Eelco Chaudron wrote:
> 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.
The v5 was sent out!
https://patchwork.ozlabs.org/project/openvswitch/list/?series=309369
//Eelco
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
