| Message ID | 20230421151651.3032616-8-amorenoz@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Improve linux QoS for exotic and fast links | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Fri, Apr 21, 2023 at 05:16:51PM +0200, Adrian Moreno wrote: > Use TCA_POLICE_RATE64 if the rate cannot be expressed using 32bits. > > This breaks the 32Gbps barrier. The new barrier is ~4Tbps caused by > netdev's API expressing kbps rates using 32-bit integers. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2137643 > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> ... > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index eb2ca15aa..996561f2e 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -2385,6 +2385,26 @@ AT_CHECK([tc class show dev ovs-p1 | grep -q 'class htb .* HTB_CONF']) > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([Ingress Policy - 64-bit]) > +AT_SKIP_IF([test $HAVE_TC = no]) Hi Adrian, as per the same question I asked in patch 4/7 for different tests. Should this test be conditional on HAVE_TCA_STATS_PKT64 and HAVE_TCA_HTB_RATE64? If so, perhaps something like CHECK_TC_INGRESS_PPS() is appropriate. > +OVS_TRAFFIC_VSWITCHD_START() > +ADD_NAMESPACES(ns0) > +ADD_VETH(p0, ns0, br0, "10.1.1.1/24") > + > +AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_rate=50000000]) > +AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_burst=40000000]) > + > +AT_CHECK([tc -o -s -d filter show dev ovs-p0 ingress | > + sed -n 's/.*\(rate [[0-9]]*[[a-zA-Z]]* burst [[0-9]]*[[a-zA-Z]]*\).*/\1/; T; p; q'], > + [0],[dnl > +rate 50Gbit burst 1200575000b > +]) > + > +AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | > + grep -E "basic|matchall" > /dev/null], [0]) > +OVS_TRAFFIC_VSWITCHD_STOP > +AT_CLEANUP > + > AT_BANNER([conntrack]) > > AT_SETUP([conntrack - controller]) ...
On 4/21/23 17:16, Adrian Moreno wrote: > Use TCA_POLICE_RATE64 if the rate cannot be expressed using 32bits. > > This breaks the 32Gbps barrier. The new barrier is ~4Tbps caused by > netdev's API expressing kbps rates using 32-bit integers. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2137643 > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- > lib/netdev-linux.c | 17 ++++++++++------- > lib/netdev-linux.h | 2 +- > lib/tc.c | 2 ++ > tests/system-traffic.at | 20 ++++++++++++++++++++ > 4 files changed, 33 insertions(+), 8 deletions(-) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index a2bae300c..14831440e 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -494,7 +494,7 @@ 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, > +static int tc_add_policer(struct netdev *, uint64_t kbits_rate, > uint32_t kbits_burst, uint32_t kpkts_rate, > uint32_t kpkts_burst); > > @@ -2661,6 +2661,7 @@ nl_msg_put_act_police(struct ofpbuf *request, uint32_t index, > struct tc_police police; > size_t offset, act_offset; > uint32_t prio = 0; > + uint64_t bytes_rate = kbits_rate / 8 * 1000; > > if (!kbits_rate && !pkts_rate) { > return; > @@ -2672,7 +2673,10 @@ nl_msg_put_act_police(struct ofpbuf *request, uint32_t index, > nl_msg_act_police_start_nest(request, ++prio, &offset, &act_offset, > single_action); > if (police.rate.rate) { > - tc_put_rtab(request, TCA_POLICE_RATE, &police.rate, 0); > + tc_put_rtab(request, TCA_POLICE_RATE, &police.rate, bytes_rate); > + } > + if (bytes_rate > UINT32_MAX) { > + nl_msg_put_u64(request, TCA_POLICE_RATE64, bytes_rate); > } > if (pkts_rate) { > uint64_t pkt_burst_ticks; > @@ -2687,7 +2691,7 @@ nl_msg_put_act_police(struct ofpbuf *request, uint32_t index, > } > > static int > -tc_add_matchall_policer(struct netdev *netdev, uint32_t kbits_rate, > +tc_add_matchall_policer(struct netdev *netdev, uint64_t kbits_rate, > uint32_t kbits_burst, uint32_t kpkts_rate, > uint32_t kpkts_burst) > { > @@ -5703,9 +5707,8 @@ tc_policer_init(struct tc_police *tc_police, uint64_t kbits_rate, > * 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) > +tc_add_policer(struct netdev *netdev, uint64_t kbits_rate, > + uint32_t kbits_burst, uint32_t kpkts_rate, uint32_t kpkts_burst) > { > size_t basic_offset, police_offset; > struct ofpbuf request; > @@ -5739,7 +5742,7 @@ tc_add_policer(struct netdev *netdev, uint32_t kbits_rate, > } > > int > -tc_add_policer_action(uint32_t index, uint32_t kbits_rate, > +tc_add_policer_action(uint32_t index, uint64_t kbits_rate, > uint32_t kbits_burst, uint32_t pkts_rate, > uint32_t pkts_burst, bool update) > { > diff --git a/lib/netdev-linux.h b/lib/netdev-linux.h > index 9a416ce50..ec19b0ded 100644 > --- a/lib/netdev-linux.h > +++ b/lib/netdev-linux.h > @@ -29,7 +29,7 @@ struct netdev; > int netdev_linux_ethtool_set_flag(struct netdev *netdev, uint32_t flag, > const char *flag_name, bool enable); > int linux_get_ifindex(const char *netdev_name); > -int tc_add_policer_action(uint32_t index, uint32_t kbits_rate, > +int tc_add_policer_action(uint32_t index, uint64_t kbits_rate, > uint32_t kbits_burst, uint32_t pkts_rate, > uint32_t pkts_burst, bool update); > int tc_del_policer_action(uint32_t index, struct ofputil_meter_stats *stats); > diff --git a/lib/tc.c b/lib/tc.c > index 4c07e2216..289783562 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -1427,6 +1427,8 @@ static const struct nl_policy police_policy[] = { > [TCA_POLICE_RATE] = { .type = NL_A_UNSPEC, > .min_len = 1024, > .optional = true, }, > + [TCA_POLICE_RATE64] = { .type = NL_A_U32 , Extra space at the end before the comma. > + .optional = true, }, > [TCA_POLICE_PEAKRATE] = { .type = NL_A_UNSPEC, > .min_len = 1024, > .optional = true, }, > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index eb2ca15aa..996561f2e 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -2385,6 +2385,26 @@ AT_CHECK([tc class show dev ovs-p1 | grep -q 'class htb .* HTB_CONF']) > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([Ingress Policy - 64-bit]) > +AT_SKIP_IF([test $HAVE_TC = no]) > +OVS_TRAFFIC_VSWITCHD_START() > +ADD_NAMESPACES(ns0) > +ADD_VETH(p0, ns0, br0, "10.1.1.1/24") > + > +AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_rate=50000000]) > +AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_burst=40000000]) > + > +AT_CHECK([tc -o -s -d filter show dev ovs-p0 ingress | > + sed -n 's/.*\(rate [[0-9]]*[[a-zA-Z]]* burst [[0-9]]*[[a-zA-Z]]*\).*/\1/; T; p; q'], > + [0],[dnl > +rate 50Gbit burst 1200575000b > +]) > + > +AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | > + grep -E "basic|matchall" > /dev/null], [0]) > +OVS_TRAFFIC_VSWITCHD_STOP > +AT_CLEANUP > + > AT_BANNER([conntrack]) > > AT_SETUP([conntrack - controller])
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index a2bae300c..14831440e 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -494,7 +494,7 @@ 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, +static int tc_add_policer(struct netdev *, uint64_t kbits_rate, uint32_t kbits_burst, uint32_t kpkts_rate, uint32_t kpkts_burst); @@ -2661,6 +2661,7 @@ nl_msg_put_act_police(struct ofpbuf *request, uint32_t index, struct tc_police police; size_t offset, act_offset; uint32_t prio = 0; + uint64_t bytes_rate = kbits_rate / 8 * 1000; if (!kbits_rate && !pkts_rate) { return; @@ -2672,7 +2673,10 @@ nl_msg_put_act_police(struct ofpbuf *request, uint32_t index, nl_msg_act_police_start_nest(request, ++prio, &offset, &act_offset, single_action); if (police.rate.rate) { - tc_put_rtab(request, TCA_POLICE_RATE, &police.rate, 0); + tc_put_rtab(request, TCA_POLICE_RATE, &police.rate, bytes_rate); + } + if (bytes_rate > UINT32_MAX) { + nl_msg_put_u64(request, TCA_POLICE_RATE64, bytes_rate); } if (pkts_rate) { uint64_t pkt_burst_ticks; @@ -2687,7 +2691,7 @@ nl_msg_put_act_police(struct ofpbuf *request, uint32_t index, } static int -tc_add_matchall_policer(struct netdev *netdev, uint32_t kbits_rate, +tc_add_matchall_policer(struct netdev *netdev, uint64_t kbits_rate, uint32_t kbits_burst, uint32_t kpkts_rate, uint32_t kpkts_burst) { @@ -5703,9 +5707,8 @@ tc_policer_init(struct tc_police *tc_police, uint64_t kbits_rate, * 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) +tc_add_policer(struct netdev *netdev, uint64_t kbits_rate, + uint32_t kbits_burst, uint32_t kpkts_rate, uint32_t kpkts_burst) { size_t basic_offset, police_offset; struct ofpbuf request; @@ -5739,7 +5742,7 @@ tc_add_policer(struct netdev *netdev, uint32_t kbits_rate, } int -tc_add_policer_action(uint32_t index, uint32_t kbits_rate, +tc_add_policer_action(uint32_t index, uint64_t kbits_rate, uint32_t kbits_burst, uint32_t pkts_rate, uint32_t pkts_burst, bool update) { diff --git a/lib/netdev-linux.h b/lib/netdev-linux.h index 9a416ce50..ec19b0ded 100644 --- a/lib/netdev-linux.h +++ b/lib/netdev-linux.h @@ -29,7 +29,7 @@ struct netdev; int netdev_linux_ethtool_set_flag(struct netdev *netdev, uint32_t flag, const char *flag_name, bool enable); int linux_get_ifindex(const char *netdev_name); -int tc_add_policer_action(uint32_t index, uint32_t kbits_rate, +int tc_add_policer_action(uint32_t index, uint64_t kbits_rate, uint32_t kbits_burst, uint32_t pkts_rate, uint32_t pkts_burst, bool update); int tc_del_policer_action(uint32_t index, struct ofputil_meter_stats *stats); diff --git a/lib/tc.c b/lib/tc.c index 4c07e2216..289783562 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -1427,6 +1427,8 @@ static const struct nl_policy police_policy[] = { [TCA_POLICE_RATE] = { .type = NL_A_UNSPEC, .min_len = 1024, .optional = true, }, + [TCA_POLICE_RATE64] = { .type = NL_A_U32 , + .optional = true, }, [TCA_POLICE_PEAKRATE] = { .type = NL_A_UNSPEC, .min_len = 1024, .optional = true, }, diff --git a/tests/system-traffic.at b/tests/system-traffic.at index eb2ca15aa..996561f2e 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -2385,6 +2385,26 @@ AT_CHECK([tc class show dev ovs-p1 | grep -q 'class htb .* HTB_CONF']) OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([Ingress Policy - 64-bit]) +AT_SKIP_IF([test $HAVE_TC = no]) +OVS_TRAFFIC_VSWITCHD_START() +ADD_NAMESPACES(ns0) +ADD_VETH(p0, ns0, br0, "10.1.1.1/24") + +AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_rate=50000000]) +AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_burst=40000000]) + +AT_CHECK([tc -o -s -d filter show dev ovs-p0 ingress | + sed -n 's/.*\(rate [[0-9]]*[[a-zA-Z]]* burst [[0-9]]*[[a-zA-Z]]*\).*/\1/; T; p; q'], + [0],[dnl +rate 50Gbit burst 1200575000b +]) + +AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | + grep -E "basic|matchall" > /dev/null], [0]) +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP + AT_BANNER([conntrack]) AT_SETUP([conntrack - controller])
Use TCA_POLICE_RATE64 if the rate cannot be expressed using 32bits. This breaks the 32Gbps barrier. The new barrier is ~4Tbps caused by netdev's API expressing kbps rates using 32-bit integers. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2137643 Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- lib/netdev-linux.c | 17 ++++++++++------- lib/netdev-linux.h | 2 +- lib/tc.c | 2 ++ tests/system-traffic.at | 20 ++++++++++++++++++++ 4 files changed, 33 insertions(+), 8 deletions(-)