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 <[email protected]> > --- > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
