[ovs-dev] [PATCH] vswitchd: Ingress policing to use matchall instead of basic
Eelco Chaudron
echaudro at redhat.com
Fri Oct 15 08:53:28 UTC 2021
See some inline comments below…
On 14 Oct 2021, at 21:44, Michael Pattrick wrote:
> Currently ingress policing uses the basic classifier to apply traffic
> control filters.
>
> However, cls_basic was removed from the upcoming RHEL9. Basic is probably
> not the proper classifier to use when the more accurage matchall
> classifier is available and already in use in the code base. Switching
> from basic to matchall allows us to remove a function.
>
> I've also modified tests to detect when ingress policing fails to apply.
>
> Signed-off-by: Michael Pattrick <mkp at redhat.com>
> ---
> lib/netdev-linux.c | 85 +++++++++-------------------------------------
> tests/ovs-vsctl.at | 24 ++++++-------
> 2 files changed, 28 insertions(+), 81 deletions(-)
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 97bd21be4..d64b88512 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -486,10 +486,6 @@ static struct tcmsg *netdev_linux_tc_make_request(const struct netdev *,
> unsigned int flags,
> struct ofpbuf *);
>
> -static int tc_add_policer(struct netdev *, uint32_t kbits_rate,
> - uint32_t kbits_burst, uint32_t kpkts_rate,
> - uint32_t kpkts_burst);
> -
> static int tc_parse_qdisc(const struct ofpbuf *, const char **kind,
> struct nlattr **options);
> static int tc_parse_class(const struct ofpbuf *, unsigned int *queue_id,
> @@ -2662,6 +2658,20 @@ nl_msg_put_act_police(struct ofpbuf *request, struct tc_police police,
> }
> }
>
> +/* Adds a policer to 'netdev' with a rate of 'kbits_rate' and a burst size
> + * of 'kbits_burst', with a rate of 'kpkts_rate' and a burst size of
> + * 'kpkts_burst'.
> + *
> + * This function is equivalent to running:
> + * /sbin/tc filter add dev <devname> parent ffff: protocol all prio 49
> + * matchall police rate <kbits_rate>kbit burst <kbits_burst>k
> + * mtu 65535 drop
> + *
> + * The configuration and stats may be seen with the following command:
> + * /sbin/tc -s filter show dev <devname> parent ffff:
> + *
> + * Returns 0 if successful, otherwise a positive errno value.
> + */
> static int
> tc_add_matchall_policer(struct netdev *netdev, uint32_t kbits_rate,
> uint32_t kbits_burst, uint32_t kpkts_rate,
> @@ -2801,8 +2811,8 @@ netdev_linux_set_policing(struct netdev *netdev_, uint32_t kbits_rate,
> goto out;
> }
>
> - error = tc_add_policer(netdev_, kbits_rate, kbits_burst,
> - kpkts_rate, kpkts_burst);
> + 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));
If you make this change, maybe it might be good to refactor netdev_linux_set_policing()? Meaning that I think, in “if (netdev_is_flow_api_enabled())” it has a bug where it’s not setting the netdev->k*_* values.
> @@ -5589,69 +5599,6 @@ netdev_linux_tc_make_request(const struct netdev *netdev, int type,
> return tc_make_request(ifindex, type, flags, request);
> }
>
> -/* Adds a policer to 'netdev' with a rate of 'kbits_rate' and a burst size
> - * of 'kbits_burst', with a rate of 'kpkts_rate' and a burst size of
> - * 'kpkts_burst'.
> - *
> - * This function is equivalent to running:
> - * /sbin/tc filter add dev <devname> parent ffff: protocol all prio 49
> - * basic police rate <kbits_rate>kbit burst <kbits_burst>k
> - * mtu 65535 drop
> - *
> - * The configuration and stats may be seen with the following command:
> - * /sbin/tc -s filter show dev <devname> parent ffff:
> - *
> - * Returns 0 if successful, otherwise a positive errno value.
> - */
> -static int
> -tc_add_policer(struct netdev *netdev, uint32_t kbits_rate,
> - uint32_t kbits_burst, uint32_t kpkts_rate,
> - uint32_t kpkts_burst)
> -{
> - size_t basic_offset, police_offset;
> - struct tc_police tc_police;
> - struct ofpbuf request;
> - struct tcmsg *tcmsg;
> - int error;
> - int mtu = 65535;
> -
> - memset(&tc_police, 0, sizeof tc_police);
> - tc_police.action = TC_POLICE_SHOT;
> - tc_police.mtu = mtu;
> - tc_fill_rate(&tc_police.rate, ((uint64_t) kbits_rate * 1000)/8, mtu);
> -
> - /* The following appears wrong in one way: In networking a kilobit is
> - * usually 1000 bits but this uses 1024 bits.
> - *
> - * However if you "fix" those problems then "tc filter show ..." shows
> - * "125000b", meaning 125,000 bits, when OVS configures it for 1000 kbit ==
> - * 1,000,000 bits, whereas this actually ends up doing the right thing from
> - * tc's point of view. Whatever. */
> - tc_police.burst = tc_bytes_to_ticks(
> - tc_police.rate.rate, MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 8);
> - tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWTFILTER,
> - NLM_F_EXCL | NLM_F_CREATE, &request);
> - if (!tcmsg) {
> - return ENODEV;
> - }
> - tcmsg->tcm_parent = tc_make_handle(0xffff, 0);
> - tcmsg->tcm_info = tc_make_handle(49,
> - (OVS_FORCE uint16_t) htons(ETH_P_ALL));
> - nl_msg_put_string(&request, TCA_KIND, "basic");
> - basic_offset = nl_msg_start_nested(&request, TCA_OPTIONS);
> - police_offset = nl_msg_start_nested(&request, TCA_BASIC_ACT);
> - nl_msg_put_act_police(&request, tc_police, kpkts_rate, kpkts_burst);
> - nl_msg_end_nested(&request, police_offset);
> - nl_msg_end_nested(&request, basic_offset);
> -
> - error = tc_transact(&request, NULL);
> - if (error) {
> - return error;
> - }
> -
> - return 0;
> -}
> -
> static void
> read_psched(void)
> {
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index d6cd2c084..193bfc5c7 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -1670,22 +1670,22 @@ AT_BANNER([set ingress policing test])
>
> AT_SETUP([set ingress_policing_rate and ingress_policing_burst])
> AT_KEYWORDS([ingress_policing])
> -OVS_VSCTL_SETUP
> +AT_CHECK([ip link add a1 type veth])
> +AT_CHECK([ip link set a1 up])
> +on_exit 'ip link del a1'
> +OVS_VSWITCHD_START([add-port br0 a1])
> AT_CHECK([RUN_OVS_VSCTL_TOGETHER(
> - [add-br a],
> - [add-port a a1],
> [set interface a1 ingress_policing_rate=100],
> [set interface a1 ingress_policing_burst=10],
> [--columns=ingress_policing_burst,ingress_policing_rate list interface a1])],
> [0],
> [
If you clean this up, maybe also use p1 for port instead of a1.
> -
> -
Did the output change so you needed to delete these empty lines?
> ingress_policing_burst: 10
> ingress_policing_rate: 100
> ])
> -OVS_VSCTL_CLEANUP
> +AT_CHECK([grep "adding policing action failed" ovs-vswitchd.log], [1], [], [])
Don’t think this is needed? If it’s an WARN or up message is in the log OVS_VSWITCHD_STOP would fail.
Also in general not sure if this is the right place for the change, i.e. these are test cases for ovs-vsctl which are platform-independent, so adding Linux-specific ip commands/ports might not be the right thing to do here.
However, I think the “AT_SETUP([offloads - set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst - offloads disabled])” already tests this case.
> +OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> dnl ----------------------------------------------------------------------
> @@ -1693,20 +1693,20 @@ AT_BANNER([set ingress policing(kpkts) test])
>
> AT_SETUP([set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst])
> AT_KEYWORDS([ingress_policing_kpkts])
> -OVS_VSCTL_SETUP
> +AT_CHECK([ip link add a1 type veth])
> +AT_CHECK([ip link set a1 up])
> +on_exit 'ip link del a1'
> +OVS_VSWITCHD_START([add-port br0 a1])
> AT_CHECK([RUN_OVS_VSCTL_TOGETHER(
> - [add-br a],
> - [add-port a a1],
> [set interface a1 ingress_policing_kpkts_rate=100],
> [set interface a1 ingress_policing_kpkts_burst=10],
> [--columns=ingress_policing_kpkts_burst,ingress_policing_kpkts_rate list interface a1])],
> [0],
> [
>
> -
> -
> ingress_policing_kpkts_burst: 10
> ingress_policing_kpkts_rate: 100
> ])
> -OVS_VSCTL_CLEANUP
> +AT_CHECK([grep "adding policing action failed" ovs-vswitchd.log], [1], [], [])
> +OVS_VSWITCHD_STOP
> AT_CLEANUP
> --
> 2.31.1
>
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list