[ovs-dev] [PATCH v3] netdev-linux: Ingress policing to use matchall if basic is, not available.
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 1 or
10240 packets for the packet burst filter. 1 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
---
lib/netdev-linux.c | 28 +++-
tests/system-offloads-traffic.at | 20 +---
2 files changed, 28 insertions(+), 20 deletions(-)
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 97bd21be4..e7d26de83 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);
@@ -2794,6 +2793,8 @@ netdev_linux_set_policing(struct netdev *netdev_,
uint32_t kbits_rate,
}
if (kbits_rate || kpkts_rate) {
+const char * cls_name = "basic";
+
error = tc_add_del_qdisc(ifindex, true, 0, TC_INGRESS);
if (error) {
VLOG_WARN_RL(&rl, "%s: adding policing qdisc failed: %s",
@@ -2803,19 +2804,28 @@ 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) {
+cls_name = "matchall";
+/* 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));
+VLOG_WARN_RL(&rl, "%s: adding cls_%s policing action failed: %s",
+netdev_name, cls_name, ovs_strerror(error));
goto out;
}
}
-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/1/'],
[0],[dnl
-pkts_rate 10 pkts_burst 10240
+pkts_rate 10 pkts_burst 1
])
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/1/'],
[0],[dnl
-pkts_rate 10 pkts_burst 10240
+pkts_rate 10 pkts_burst 1
])
AT_CHECK([tc -s -d filter show dev ovs-p0 ingress |
sed -n 's/.*\(
Re: [ovs-dev] [PATCH v3] netdev-linux: Ingress policing to use matchall if basic is, not available.
Bleep bloop. Greetings Mike Pattrick, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: ERROR: Author Mike Pattrick needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Mike Pattrick Lines checked: 134, Warnings: 1, Errors: 1 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] netdev-linux: Ingress policing to use matchall if basic is, not available.
On 12 Nov 2021, at 18:49, 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 1 or
> 10240 packets for the packet burst filter. 1 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
> ---
> lib/netdev-linux.c | 28 +++-
> tests/system-offloads-traffic.at | 20 +---
> 2 files changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 97bd21be4..e7d26de83 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);
> @@ -2794,6 +2793,8 @@ netdev_linux_set_policing(struct netdev *netdev_,
> uint32_t kbits_rate,
> }
>
> if (kbits_rate || kpkts_rate) {
> +const char * cls_name = "basic";
Changes look good to me, however, there should be no space after the *.
Maybe the maintainer can fix this on commit?
Acked-by: Eelco Chaudron
> +
> error = tc_add_del_qdisc(ifindex, true, 0, TC_INGRESS);
> if (error) {
> VLOG_WARN_RL(&rl, "%s: adding policing qdisc failed: %s",
> @@ -2803,19 +2804,28 @@ 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) {
> +cls_name = "matchall";
> +/* 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));
> +VLOG_WARN_RL(&rl, "%s: adding cls_%s policing action failed: %s",
> +netdev_name, cls_name, ovs_strerror(error));
> goto out;
> }
> }
>
> -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/1/'],
>[0],[dnl
> -pkts_rate 10 pkts_burst 10240
> +pkts_rate 10 pkts_burst 1
> ])
> 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_CH
Re: [ovs-dev] [PATCH v3] netdev-linux: Ingress policing to use matchall if basic is, not available.
On 11/12/21 18:49, 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. Hello, Mike. I'm not sure I understand the reason behind the logic here. If 'matchall' is better (as you stated in previous versions of a patch), why not try it first and fall back to 'basic' if not available? Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] netdev-linux: Ingress policing to use matchall if basic is, not available.
On Fri, Dec 3, 2021 at 9:29 AM Ilya Maximets wrote: > > On 11/12/21 18:49, 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. > > Hello, Mike. I'm not sure I understand the reason behind the logic here. > > If 'matchall' is better (as you stated in previous versions of a patch), > why not try it first and fall back to 'basic' if not available? I think I was going for maintaining the current behaviour as much as possible. The matchall classifier is technically more efficient than basic, but I wasn't able to find a noticeable difference in my testing. If you prefer, I can flip that logic? Cheers, Michael > Best regards, Ilya Maximets. > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] netdev-linux: Ingress policing to use matchall if basic is, not available.
On 12/3/21 17:37, Mike Pattrick wrote: > On Fri, Dec 3, 2021 at 9:29 AM Ilya Maximets wrote: >> >> On 11/12/21 18:49, 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. >> >> Hello, Mike. I'm not sure I understand the reason behind the logic here. >> >> If 'matchall' is better (as you stated in previous versions of a patch), >> why not try it first and fall back to 'basic' if not available? > > I think I was going for maintaining the current behaviour as much as > possible. The matchall classifier is technically more efficient than > basic, but I wasn't able to find a noticeable difference in my > testing. > > If you prefer, I can flip that logic? 'matchall' seems to be more accurate. At least from the configuration point of view, so it's probably better to use it if possible. > > Cheers, > Michael > >> Best regards, Ilya Maximets. >> > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
