[ovs-dev] [PATCH] netdev-linux: Ingress policing to use matchall if basic is not available.

Eelco Chaudron echaudro at redhat.com
Fri Nov 12 13:52:41 UTC 2021


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 at 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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev



More information about the dev mailing list