[ovs-dev,v5,13/15] netdev-offload-tc: If the flow has not been used, report it as such.

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 <echaudro@redhat.com>
Acked-by: Roi Dayan <roid@nvidia.com>
---
 lib/tc.c                 |    8 +++++++-
 tests/system-offloads.at |    3 +--
 tests/system-traffic.at  |    2 ++
 3 files changed, 10 insertions(+), 3 deletions(-)

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 <echaudro@redhat.com>
> Acked-by: Roi Dayan <roid@nvidia.com>
> ---
>  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.
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

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 <echaudro@redhat.com>
>> Acked-by: Roi Dayan <roid@nvidia.com>
>> ---
>>  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.

> 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

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 <echaudro@redhat.com>
>>> Acked-by: Roi Dayan <roid@nvidia.com>
>>> ---
>>>  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

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 <echaudro@redhat.com>
>>>> Acked-by: Roi Dayan <roid@nvidia.com>
>>>> ---
>>>>  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


> 
>>> 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
>

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 <echaudro@redhat.com>
>>>>> Acked-by: Roi Dayan <roid@nvidia.com>
>>>>> ---
>>>>>  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
>>