| Message ID | 160bc3cabaf546fb15bb5172b49c5956f6f1e744.camel@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] vswitchd: Ingress policing to use matchall instead of basic | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
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@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 10/14/21 21:44, Michael Pattrick wrote: > Currently ingress policing uses the basic classifier to apply traffic > control filters. Hi, Michael. Thanks for submitting a patch! I'm not going to review technical details of implementation, I'll leave this to Simon and others, but I want to make a few high level comments. First, please increase the version number for a patch while sending new versions, this can be done by adding --subject-prefix='PATCH v2' or simply -v2 to the list of 'git format-patch' arguments. Otherwise it's confusing for reviewers and any automated patch tracking systems we have. And the patch name should start with 'netdev-linux:', not 'vswitchd:', as 'vswitchd' is way too broad area, and this patch is only really touching netdev-linux part. More comments below. > > However, cls_basic was removed from the upcoming RHEL9. This itself doesn't sound like a correct justification, as RHEL is not the only distribution in a world. Does other distributions getting rid of cls_basic too or was it deprecated in upstream linux kernel? > Basic is probably > not the proper classifier to use when the more accurage matchall What does "not the proper classifier" mean? I think, the difference between classifiers should be better described in a commit message. > classifier is available and already in use in the code base. Switching > from basic to matchall allows us to remove a function. Even though 'matchall' is used in a codebase, it's used only if HW offloading is enabled. HW offloading itself is a relatively high requirement, because it requires more recent kernel versions in most cases. 'matchall' is available starting from kernel v4.8, while 'basic' goes back to 2.16.12. It's fair to expect that users of HW offloading will use more recent kernel than 4.8. However, 4.4 is still a supported longterm kernel version and switching to 'matchall' for everyone means dropping support for policing in OVS for currently supported upstream kernels older than 4.8. Can we detect the support of 'matchall' and fall back if not available? Not sure how important this is, but that is something that definitely needs to be discussed and documented. > > I've also modified tests to detect when ingress policing fails to apply. > > Signed-off-by: Michael Pattrick <mkp@redhat.com> > --- > lib/netdev-linux.c | 85 +++++++++------------------------------------- > tests/ovs-vsctl.at | 24 ++++++------- > 2 files changed, 28 insertions(+), 81 deletions(-) > <snip> > 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' These tests are part of "unit" tests. They should not touch system interfaces, hence no 'ip' commands should be here. Also, these tests should be platform-independent, e.g. they should run on Windows and DSB systems too. Moreover, these tests are most of the time not started from a root user, so they will just fail. And they do fail all our CI runs because of that, you can see in the ovsrobot's reports on a patchwork: https://patchwork.ozlabs.org/project/openvswitch/patch/160bc3cabaf546fb15bb5172b49c5956f6f1e744.camel@redhat.com/ Another thing is that you're not changing tests/system-offload-traffic.at. So these tests will fail as they look for configured 'basic' policer. You may run these tests with 'make check-offloads'. Best regards, Ilya Maximets.
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)); @@ -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], [ - - ingress_policing_burst: 10 ingress_policing_rate: 100 ]) -OVS_VSCTL_CLEANUP +AT_CHECK([grep "adding policing action failed" ovs-vswitchd.log], [1], [], []) +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
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@redhat.com> --- lib/netdev-linux.c | 85 +++++++++------------------------------------- tests/ovs-vsctl.at | 24 ++++++------- 2 files changed, 28 insertions(+), 81 deletions(-)