| Message ID | c926db1e84530740dec3a01b5d2f7171de29e166.camel@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] netdev-linux: Ingress policing to use matchall if basic is not available. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
One small nit on the below… On 18 Oct 2021, at 21:07, Mike Pattrick wrote: > Currently ingress policing uses the basic classifier to apply traffic > control filters if hardware offload is not enabled, else it uses > matchall. This change enables fallback onto the matchall classifier for > cases when the kernel is not built with basic support and hardware > offload is not in use. Basic is still selected first. > > The system tests are modified to allow either basic or matchall > classification on the ingestion filter, and to allow either 10000 or > 10240 packets for the packet burst filter. 10000 is accurate for kernel > 5.14 and the most recent iproute2, however, 10240 is left for > compatibility with older kernels. > > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > lib/netdev-linux.c | 21 ++++++++++++++------- > tests/system-offloads-traffic.at | 20 +++++++++----------- > 2 files changed, 23 insertions(+), 18 deletions(-) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index 97bd21be4..cb7ce1d2f 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -2776,8 +2776,7 @@ netdev_linux_set_policing(struct netdev *netdev_, uint32_t kbits_rate, > error = tc_add_matchall_policer(netdev_, kbits_rate, kbits_burst, > kpkts_rate, kpkts_burst); > } > - ovs_mutex_unlock(&netdev->mutex); > - return error; > + goto out; > } > > error = get_ifindex(netdev_, &ifindex); > @@ -2803,6 +2802,12 @@ netdev_linux_set_policing(struct netdev *netdev_, uint32_t kbits_rate, > < > error = tc_add_policer(netdev_, kbits_rate, kbits_burst, > kpkts_rate, kpkts_burst); > + if (error == ENOENT) { > + /* This error is returned when the basic classifier is missing. > + * Fall back to the matchall classifier. */ > + error = tc_add_matchall_policer(netdev_, kbits_rate, kbits_burst, > + kpkts_rate, kpkts_burst); > + } > if (error){ > VLOG_WARN_RL(&rl, "%s: adding policing action failed: %s", > netdev_name, ovs_strerror(error)); I think it might be nice to know which policer was failing in the end. Maybe something like this (not tested ;)? @@ -2793,6 +2793,8 @@ netdev_linux_set_policing(struct netdev *netdev_, uint32_t kbits_rate, } if (kbits_rate || kpkts_rate) { + bool used_matchall = false; + error = tc_add_del_qdisc(ifindex, true, 0, TC_INGRESS); if (error) { VLOG_WARN_RL(&rl, "%s: adding policing qdisc failed: %s", @@ -2805,12 +2807,14 @@ netdev_linux_set_policing(struct netdev *netdev_, uint32_t kbits_rate, if (error == ENOENT) { /* This error is returned when the basic classifier is missing. * Fall back to the matchall classifier. */ + used_matchall = true; error = tc_add_matchall_policer(netdev_, kbits_rate, kbits_burst, kpkts_rate, kpkts_burst); } if (error){ - VLOG_WARN_RL(&rl, "%s: adding policing action failed: %s", - netdev_name, ovs_strerror(error)); + VLOG_WARN_RL(&rl, "%s: adding %spolicing action failed: %s", + netdev_name, used_matchall ? "matchall " : "", + ovs_strerror(error)); goto out; } } > @@ -2810,12 +2815,14 @@ netdev_linux_set_policing(struct netdev *netdev_, uint32_t kbits_rate, > } > } > > - netdev->kbits_rate = kbits_rate; > - netdev->kbits_burst = kbits_burst; > - netdev->kpkts_rate = kpkts_rate; > - netdev->kpkts_burst = kpkts_burst; > - > out: > + if (!error) { > + netdev->kbits_rate = kbits_rate; > + netdev->kbits_burst = kbits_burst; > + netdev->kpkts_rate = kpkts_rate; > + netdev->kpkts_burst = kpkts_burst; > + } > + > if (!error || error == ENODEV) { > netdev->netdev_policing_error = error; > netdev->cache_valid |= VALID_POLICING; > diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at > index be63057bb..80bc1dd5c 100644 > --- a/tests/system-offloads-traffic.at > +++ b/tests/system-offloads-traffic.at > @@ -89,10 +89,8 @@ AT_CHECK([tc -o -s -d filter show dev ovs-p0 ingress | > [0],[dnl > rate 100Kbit burst 1280b > ]) > -AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | grep basic | > - sed -n 's/.*\(basic\).*/\1/; T; p; q'], [0], [dnl > -basic > -]) > +AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | > + egrep "basic|matchall" > /dev/null], [0]) > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > @@ -135,14 +133,13 @@ AT_CHECK([ovs-vsctl --columns=other_config list open], [0], [dnl > other_config : {hw-offload="false"} > ]) > AT_CHECK([tc -o -s -d filter show dev ovs-p0 ingress | > - sed -n 's/.*\(pkts_rate [[0-9]]*[[a-zA-Z]]* pkts_burst [[0-9]]*[[a-zA-Z]]*\).*/\1/; T; p; q'], > + sed -n 's/.*\(pkts_rate [[0-9]]*[[a-zA-Z]]* pkts_burst [[0-9]]*[[a-zA-Z]]*\).*/\1/; T; p; q' | > + sed -e 's/10240/10000/'], > [0],[dnl > -pkts_rate 100000 pkts_burst 10240 > +pkts_rate 100000 pkts_burst 10000 > ]) > AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | > - sed -n 's/.*\(basic\).*/\1/; T; p; q'], [0], [dnl > -basic > -]) > + egrep "basic|matchall" > /dev/null], [0]) > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > @@ -160,9 +157,10 @@ AT_CHECK([ovs-vsctl --columns=other_config list open], [0], [dnl > other_config : {hw-offload="true"} > ]) > AT_CHECK([tc -o -s -d filter show dev ovs-p0 ingress | > - sed -n 's/.*\(pkts_rate [[0-9]]*[[a-zA-Z]]* pkts_burst [[0-9]]*[[a-zA-Z]]*\).*/\1/; T; p; q'], > + sed -n 's/.*\(pkts_rate [[0-9]]*[[a-zA-Z]]* pkts_burst [[0-9]]*[[a-zA-Z]]*\).*/\1/; T; p; q' | > + sed -e 's/10240/10000/'], > [0],[dnl > -pkts_rate 100000 pkts_burst 10240 > +pkts_rate 100000 pkts_burst 10000 > ]) > AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | > sed -n 's/.*\(matchall\).*/\1/; T; p; q'], [0], [dnl > -- > 2.30.2 > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 97bd21be4..cb7ce1d2f 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -2776,8 +2776,7 @@ netdev_linux_set_policing(struct netdev *netdev_, uint32_t kbits_rate, error = tc_add_matchall_policer(netdev_, kbits_rate, kbits_burst, kpkts_rate, kpkts_burst); } - ovs_mutex_unlock(&netdev->mutex); - return error; + goto out; } error = get_ifindex(netdev_, &ifindex); @@ -2803,6 +2802,12 @@ netdev_linux_set_policing(struct netdev *netdev_, uint32_t kbits_rate, error = tc_add_policer(netdev_, kbits_rate, kbits_burst, kpkts_rate, kpkts_burst); + if (error == ENOENT) { + /* This error is returned when the basic classifier is missing. + * Fall back to the matchall classifier. */ + error = tc_add_matchall_policer(netdev_, kbits_rate, kbits_burst, + kpkts_rate, kpkts_burst); + } if (error){ VLOG_WARN_RL(&rl, "%s: adding policing action failed: %s", netdev_name, ovs_strerror(error)); @@ -2810,12 +2815,14 @@ netdev_linux_set_policing(struct netdev *netdev_, uint32_t kbits_rate, } } - netdev->kbits_rate = kbits_rate; - netdev->kbits_burst = kbits_burst; - netdev->kpkts_rate = kpkts_rate; - netdev->kpkts_burst = kpkts_burst; - out: + if (!error) { + netdev->kbits_rate = kbits_rate; + netdev->kbits_burst = kbits_burst; + netdev->kpkts_rate = kpkts_rate; + netdev->kpkts_burst = kpkts_burst; + } + if (!error || error == ENODEV) { netdev->netdev_policing_error = error; netdev->cache_valid |= VALID_POLICING; diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at index be63057bb..80bc1dd5c 100644 --- a/tests/system-offloads-traffic.at +++ b/tests/system-offloads-traffic.at @@ -89,10 +89,8 @@ AT_CHECK([tc -o -s -d filter show dev ovs-p0 ingress | [0],[dnl rate 100Kbit burst 1280b ]) -AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | grep basic | - sed -n 's/.*\(basic\).*/\1/; T; p; q'], [0], [dnl -basic -]) +AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | + egrep "basic|matchall" > /dev/null], [0]) OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP @@ -135,14 +133,13 @@ AT_CHECK([ovs-vsctl --columns=other_config list open], [0], [dnl other_config : {hw-offload="false"} ]) AT_CHECK([tc -o -s -d filter show dev ovs-p0 ingress | - sed -n 's/.*\(pkts_rate [[0-9]]*[[a-zA-Z]]* pkts_burst [[0-9]]*[[a-zA-Z]]*\).*/\1/; T; p; q'], + sed -n 's/.*\(pkts_rate [[0-9]]*[[a-zA-Z]]* pkts_burst [[0-9]]*[[a-zA-Z]]*\).*/\1/; T; p; q' | + sed -e 's/10240/10000/'], [0],[dnl -pkts_rate 100000 pkts_burst 10240 +pkts_rate 100000 pkts_burst 10000 ]) AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | - sed -n 's/.*\(basic\).*/\1/; T; p; q'], [0], [dnl -basic -]) + egrep "basic|matchall" > /dev/null], [0]) OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP @@ -160,9 +157,10 @@ AT_CHECK([ovs-vsctl --columns=other_config list open], [0], [dnl other_config : {hw-offload="true"} ]) AT_CHECK([tc -o -s -d filter show dev ovs-p0 ingress | - sed -n 's/.*\(pkts_rate [[0-9]]*[[a-zA-Z]]* pkts_burst [[0-9]]*[[a-zA-Z]]*\).*/\1/; T; p; q'], + sed -n 's/.*\(pkts_rate [[0-9]]*[[a-zA-Z]]* pkts_burst [[0-9]]*[[a-zA-Z]]*\).*/\1/; T; p; q' | + sed -e 's/10240/10000/'], [0],[dnl -pkts_rate 100000 pkts_burst 10240 +pkts_rate 100000 pkts_burst 10000 ]) AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | sed -n 's/.*\(matchall\).*/\1/; T; p; q'], [0], [dnl
Currently ingress policing uses the basic classifier to apply traffic control filters if hardware offload is not enabled, else it uses matchall. This change enables fallback onto the matchall classifier for cases when the kernel is not built with basic support and hardware offload is not in use. Basic is still selected first. The system tests are modified to allow either basic or matchall classification on the ingestion filter, and to allow either 10000 or 10240 packets for the packet burst filter. 10000 is accurate for kernel 5.14 and the most recent iproute2, however, 10240 is left for compatibility with older kernels. Signed-off-by: Mike Pattrick <mkp@redhat.com> --- lib/netdev-linux.c | 21 ++++++++++++++------- tests/system-offloads-traffic.at | 20 +++++++++----------- 2 files changed, 23 insertions(+), 18 deletions(-)