| Message ID | 167456517013.1023551.3854059015230682384.stgit@ebuild.local |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | tests: Add system-traffic.at tests to check-offloads. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 1/24/23 13:59, Eelco Chaudron wrote: > If a tc flow was installed but has not yet been used, report it as such. > > In addition, add a delay to the "IGMP - flood under normal action" test > case to make it work with many repetitions. This delay is also present > in other ICMP/IGMP tests. > Should there be a Fixes tag? > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > Acked-by: Roi Dayan <roid@nvidia.com> > --- > lib/tc.c | 14 +++++++++++++- > tests/system-offloads.at | 3 +-- > tests/system-traffic.at | 2 ++ > 3 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/lib/tc.c b/lib/tc.c > index 447ab376e..c8f16874b 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -1366,7 +1366,19 @@ get_user_hz(void) > static void > nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower) > { > - uint64_t lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz()); > + uint64_t lastused; > + > + /* On creation both tm->install and tm->lastuse are set to jiffies > + * by the kernel. So if both values are the same, the flow has not been > + * used yet. > + * > + * Note that tm->firstuse can not be used due to some kernel bug, i.e., > + * hardware offloaded flows do not update tm->firstuse. */ > + if (tm->lastuse == tm->install) { > + lastused = 0; > + } else { > + lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz()); > + } > > if (flower->lastused < lastused) { > flower->lastused = lastused; > diff --git a/tests/system-offloads.at b/tests/system-offloads.at > index 57be710a7..f4adac7b3 100644 > --- a/tests/system-offloads.at > +++ b/tests/system-offloads.at > @@ -75,8 +75,7 @@ conntrack - multiple zones, local > conntrack - multi-stage pipeline, local > conntrack - ICMP related with NAT > conntrack - DNAT load balancing > -conntrack - DNAT load balancing with NC > -IGMP - flood under normal action" > +conntrack - DNAT load balancing with NC" > echo "$ovs_test_skip_list" | sed "s/<SPC>/ /g"]) > > m4_include([tests/system-traffic.at]) > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index b140a1e26..d162674c9 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -7153,6 +7153,8 @@ f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl > 00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl > 00 00 00 00 > /dev/null]) > > +sleep 1 > + > AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep -e .*ipv4 | sort | dnl > strip_stats | strip_used | strip_recirc | dnl > sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], OVS_WAIT_UNTIL_EQUAL() ?
On 26 Jan 2023, at 22:43, Ilya Maximets wrote: > On 1/24/23 13:59, Eelco Chaudron wrote: >> If a tc flow was installed but has not yet been used, report it as such. >> >> In addition, add a delay to the "IGMP - flood under normal action" test >> case to make it work with many repetitions. This delay is also present >> in other ICMP/IGMP tests. >> > > Should there be a Fixes tag? Yes, will add ‘f98e418fbdb6 ("tc: Add tc flower functions”)’ >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> Acked-by: Roi Dayan <roid@nvidia.com> >> --- >> lib/tc.c | 14 +++++++++++++- >> tests/system-offloads.at | 3 +-- >> tests/system-traffic.at | 2 ++ >> 3 files changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/lib/tc.c b/lib/tc.c >> index 447ab376e..c8f16874b 100644 >> --- a/lib/tc.c >> +++ b/lib/tc.c >> @@ -1366,7 +1366,19 @@ get_user_hz(void) >> static void >> nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower) >> { >> - uint64_t lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz()); >> + uint64_t lastused; >> + >> + /* On creation both tm->install and tm->lastuse are set to jiffies >> + * by the kernel. So if both values are the same, the flow has not been >> + * used yet. >> + * >> + * Note that tm->firstuse can not be used due to some kernel bug, i.e., >> + * hardware offloaded flows do not update tm->firstuse. */ >> + if (tm->lastuse == tm->install) { >> + lastused = 0; >> + } else { >> + lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz()); >> + } >> >> if (flower->lastused < lastused) { >> flower->lastused = lastused; >> diff --git a/tests/system-offloads.at b/tests/system-offloads.at >> index 57be710a7..f4adac7b3 100644 >> --- a/tests/system-offloads.at >> +++ b/tests/system-offloads.at >> @@ -75,8 +75,7 @@ conntrack - multiple zones, local >> conntrack - multi-stage pipeline, local >> conntrack - ICMP related with NAT >> conntrack - DNAT load balancing >> -conntrack - DNAT load balancing with NC >> -IGMP - flood under normal action" >> +conntrack - DNAT load balancing with NC" >> echo "$ovs_test_skip_list" | sed "s/<SPC>/ /g"]) >> >> m4_include([tests/system-traffic.at]) >> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >> index b140a1e26..d162674c9 100644 >> --- a/tests/system-traffic.at >> +++ b/tests/system-traffic.at >> @@ -7153,6 +7153,8 @@ f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl >> 00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl >> 00 00 00 00 > /dev/null]) >> >> +sleep 1 >> + >> AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep -e .*ipv4 | sort | dnl >> strip_stats | strip_used | strip_recirc | dnl >> sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'], > > OVS_WAIT_UNTIL_EQUAL() ? I removed the delay altogether after doing 50+ successful runs on my system :) Not sure why but it seems TC seems to need fewer delays. Will re-test all patches before sending them out again and remove all obsolete patches.
diff --git a/lib/tc.c b/lib/tc.c index 447ab376e..c8f16874b 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -1366,7 +1366,19 @@ get_user_hz(void) static void nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower) { - uint64_t lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz()); + uint64_t lastused; + + /* On creation both tm->install and tm->lastuse are set to jiffies + * by the kernel. So if both values are the same, the flow has not been + * used yet. + * + * Note that tm->firstuse can not be used due to some kernel bug, i.e., + * hardware offloaded flows do not update tm->firstuse. */ + if (tm->lastuse == tm->install) { + lastused = 0; + } else { + lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz()); + } if (flower->lastused < lastused) { flower->lastused = lastused; diff --git a/tests/system-offloads.at b/tests/system-offloads.at index 57be710a7..f4adac7b3 100644 --- a/tests/system-offloads.at +++ b/tests/system-offloads.at @@ -75,8 +75,7 @@ conntrack - multiple zones, local conntrack - multi-stage pipeline, local conntrack - ICMP related with NAT conntrack - DNAT load balancing -conntrack - DNAT load balancing with NC -IGMP - flood under normal action" +conntrack - DNAT load balancing with NC" echo "$ovs_test_skip_list" | sed "s/<SPC>/ /g"]) m4_include([tests/system-traffic.at]) diff --git a/tests/system-traffic.at b/tests/system-traffic.at index b140a1e26..d162674c9 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -7153,6 +7153,8 @@ f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 49 45 65 eb 4a e0 dnl 00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl 00 00 00 00 > /dev/null]) +sleep 1 + AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep -e .*ipv4 | sort | dnl strip_stats | strip_used | strip_recirc | dnl sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'],