On 12 Dec 2022, at 12:36, Roi Dayan wrote:
> On 12/12/2022 10:06, Roi Dayan wrote:
>>
>>
>> On 23/11/2022 13:19, 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.
>>>
>>> Signed-off-by: Eelco Chaudron <[email protected]>
>>> Acked-by: Roi Dayan <[email protected]>
>>> ---
>>> lib/tc.c | 8 +++++++-
>>> tests/system-offloads.at | 3 +--
>>> tests/system-traffic.at | 2 ++
>>> 3 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/lib/tc.c b/lib/tc.c
>>> index a66dc432f..934df2e5e 100644
>>> --- a/lib/tc.c
>>> +++ b/lib/tc.c
>>> @@ -1361,7 +1361,13 @@ 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;
>>> +
>>> + if (tm->firstuse == 0) {
>>> + 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 102e89a1f..9db68b2a0 100644
>>> --- a/tests/system-offloads.at
>>> +++ b/tests/system-offloads.at
>>> @@ -76,8 +76,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 07913a192..57ff83b51 100644
>>> --- a/tests/system-traffic.at
>>> +++ b/tests/system-traffic.at
>>> @@ -7002,6 +7002,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]]*),/,/'],
>>>
>>
>>
>> Hi Eelco,
>>
>> I missed but we found an issue with this commit.
>> hw offloaded rules dont get their firtuse attr being updated.
>> This cause tc hw offloaded rules to always report as used:never.
>> We added a specific check for this to catch this.
>>
>> This looks like a kernel bug that .stats_update op in tc actions
>> should have a wrapper like tcf_lastuse_update() in .act op to
>> update firstuse if 0.
>
> or maybe to update the call to the offload driver to do it as the sw
> can't tell and doing it in .stats_update won't be real. it will be
> the first time dump stats was done.
Hi Roi,
Thanks for noticing this!
I looked a bit at the kernel code and it looks like lastused is not initialized
as 0, which is the main problem for OVS. However, I think the following
modification to the patch should solve the problem:
- if (tm->firstuse == 0) {
+ /* 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());
Can you maybe test this with your HW, as I do not have a setup ready?
Thanks,
Eelco
>> Will you send a fix for the kernel?
>>
>> Since we don't want to break working with current kernels I think
>> we should avoid this patch for now and/or add a check to do this
>> with newer kernels only.
>>
>> Thanks,
>> Roi
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
