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

Eelco Chaudron echaudro at redhat.com
Fri Nov 19 14:29:17 UTC 2021



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 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               | 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 <echaudro at redhat.com>

> +
>          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/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.27.0



More information about the dev mailing list