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 <dcara...@redhat.com> > 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 <dcara...@redhat.com> > Reviewed-by: Marcelo Ricardo Leitner <marcelo.leit...@gmail.com> > Signed-off-by: David S. Miller <da...@davemloft.net> > > > 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? Thanks. > > 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
