On 13 Dec 2022, at 16:03, Roi Dayan wrote:
> On 13/12/2022 16:27, Eelco Chaudron wrote:
>>
>>
>> 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
>>
>>
>
> Hi,
>
> yes this is fine. I ran a checkup again with the original patch and
> saw the used:never issue. after this fixup it's ok again.
> so looks ok.
>
> Thanks,
> Roi
Ok, will send out a v6.
//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
