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. 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
