[ovs-dev] test: move check for tc ingress pps support to test script

Move check for tc ingress pps support to from aclocal to test script

This has several problems:

1. Stderror from failing commands is output when executing
   various make targets.

2. There are various failure conditions that lead
   to veth0 and veth1 being created by not cleaned up.

3. The check seems to execute for many make targets.
   And it attempts to temporarily modify system state.
   This seems inappopriate.

All these problems are addressed by this patch.

Signed-off-by: Simon Horman <simon.horman@corigine.com>
Reviewed-by: Louis Peens <louis.peens@corigine.com>
---
 tests/atlocal.in                 | 11 -----------
 tests/system-offloads-traffic.at | 23 +++++++++++++++++++++--
 2 files changed, 21 insertions(+), 13 deletions(-)

On 2/23/23 14:02, Simon Horman wrote:
> Move check for tc ingress pps support to from aclocal to test script
> 
> This has several problems:
> 
> 1. Stderror from failing commands is output when executing
>    various make targets.
> 
> 2. There are various failure conditions that lead
>    to veth0 and veth1 being created by not cleaned up.
> 
> 3. The check seems to execute for many make targets.
>    And it attempts to temporarily modify system state.
>    This seems inappopriate.
> 
> All these problems are addressed by this patch.

Thanks, Simon!  These are indeed very annoying problems.

I didn't test, but I have a few minor comment inline.

Best regards, Ilya Maximets.

> 
> Signed-off-by: Simon Horman <simon.horman@corigine.com>
> Reviewed-by: Louis Peens <louis.peens@corigine.com>
> ---
>  tests/atlocal.in                 | 11 -----------
>  tests/system-offloads-traffic.at | 23 +++++++++++++++++++++--
>  2 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/atlocal.in b/tests/atlocal.in
> index e02248f6f829..859668586299 100644
> --- a/tests/atlocal.in
> +++ b/tests/atlocal.in
> @@ -172,17 +172,6 @@ fi
>  # Set HAVE_TC
>  find_command tc
>  
> -# When HAVE_TC=yes, check if the current tc supports adding pps filter
> -SUPPORT_TC_INGRESS_PPS="no"
> -if test $HAVE_TC="yes"; then
> -    ip link add veth0 type veth peer name veth1
> -    tc qdisc add dev veth0 handle ffff: ingress
> -    if tc filter add dev veth0 parent ffff: u32 match u32 0 0 police pkts_rate 100 pkts_burst 10; then
> -        SUPPORT_TC_INGRESS_PPS="yes"
> -    fi
> -    ip link del veth0
> -fi
> -
>  # Set HAVE_TCPDUMP
>  find_command tcpdump
>  
> diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
> index f2bf9c0639aa..ff3e4c63d127 100644
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
> @@ -18,6 +18,25 @@ m4_define([OVS_CHECK_ACTIONS], [
>             [0], [$1])
>  ])
>  
> +m4_define([CHECK_TC_INGRESS_PPS],
> +[
> +    AT_SKIP_IF([test $HAVE_TC = "no"])
> +    AT_CHECK([ip link add veth0 type veth peer name veth1 || exit 77])

Since we're here, we should probably re-name these interfaces to
something more specific.  I can imagine a scenario where veth0
port already exists in the system.
Maybe s/veth/ovs_tc_pps/ or something like that?

> +    AT_CHECK([
> +        if ! tc qdisc add dev veth0 handle ffff: ingress; then
> +            ip link del veth0

Instead of deleting in every branch, we might add on_exit call somewhere
at the beginning of this function.  The port will stick around till the
end of the test, but I'm not sure if that's a problem if the name is
special enough.  Or we may keep one extra del at the very end.

With on_exit, can probably be transformed into something like (untested):

  AT_CHECK([tc qdisc add dev veth0 handle ffff: ingress || exit 77])

What do you think?

> +            exit 77
> +        fi
> +    ])
> +    AT_CHECK([
> +        if ! tc filter add dev veth0 parent ffff: \

'dnl' is prefered for the line break.  autotest refuses to print out
commands with '\' in them.

> +                 u32 match u32 0 0 police pkts_rate 100 pkts_burst 10; then
> +            ip link del veth0
> +            exit 77
> +        fi
> +    ])
> +    ip link del veth0
> +])
>  
>  AT_SETUP([offloads - ping between two ports - offloads disabled])
>  OVS_TRAFFIC_VSWITCHD_START()
> @@ -132,7 +151,7 @@ AT_CLEANUP
>  
>  AT_SETUP([offloads - set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst - offloads disabled])
>  AT_KEYWORDS([ingress_policing_kpkts])
> -AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
> +CHECK_TC_INGRESS_PPS()
>  OVS_TRAFFIC_VSWITCHD_START()
>  AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=false])
>  AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
> @@ -156,7 +175,7 @@ AT_CLEANUP
>  
>  AT_SETUP([offloads - set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst - offloads enabled])
>  AT_KEYWORDS([ingress_policing_kpkts])
> -AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
> +CHECK_TC_INGRESS_PPS()
>  OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . other_config:hw-offload=true])
>  AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
>  ADD_NAMESPACES(at_ns0)

On 2/23/23 15:19, Ilya Maximets wrote:
> On 2/23/23 14:02, Simon Horman wrote:
>> Move check for tc ingress pps support to from aclocal to test script
>>
>> This has several problems:
>>
>> 1. Stderror from failing commands is output when executing
>>    various make targets.
>>
>> 2. There are various failure conditions that lead
>>    to veth0 and veth1 being created by not cleaned up.
>>
>> 3. The check seems to execute for many make targets.
>>    And it attempts to temporarily modify system state.
>>    This seems inappopriate.
>>
>> All these problems are addressed by this patch.
> 
> Thanks, Simon!  These are indeed very annoying problems.
> 
> I didn't test, but I have a few minor comment inline.
> 
> Best regards, Ilya Maximets.
> 
>>
>> Signed-off-by: Simon Horman <simon.horman@corigine.com>
>> Reviewed-by: Louis Peens <louis.peens@corigine.com>
>> ---
>>  tests/atlocal.in                 | 11 -----------
>>  tests/system-offloads-traffic.at | 23 +++++++++++++++++++++--
>>  2 files changed, 21 insertions(+), 13 deletions(-)
>>
>> diff --git a/tests/atlocal.in b/tests/atlocal.in
>> index e02248f6f829..859668586299 100644
>> --- a/tests/atlocal.in
>> +++ b/tests/atlocal.in
>> @@ -172,17 +172,6 @@ fi
>>  # Set HAVE_TC
>>  find_command tc
>>  
>> -# When HAVE_TC=yes, check if the current tc supports adding pps filter
>> -SUPPORT_TC_INGRESS_PPS="no"
>> -if test $HAVE_TC="yes"; then
>> -    ip link add veth0 type veth peer name veth1
>> -    tc qdisc add dev veth0 handle ffff: ingress
>> -    if tc filter add dev veth0 parent ffff: u32 match u32 0 0 police pkts_rate 100 pkts_burst 10; then
>> -        SUPPORT_TC_INGRESS_PPS="yes"
>> -    fi
>> -    ip link del veth0
>> -fi
>> -
>>  # Set HAVE_TCPDUMP
>>  find_command tcpdump
>>  
>> diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
>> index f2bf9c0639aa..ff3e4c63d127 100644
>> --- a/tests/system-offloads-traffic.at
>> +++ b/tests/system-offloads-traffic.at
>> @@ -18,6 +18,25 @@ m4_define([OVS_CHECK_ACTIONS], [
>>             [0], [$1])
>>  ])
>>  
>> +m4_define([CHECK_TC_INGRESS_PPS],
>> +[
>> +    AT_SKIP_IF([test $HAVE_TC = "no"])
>> +    AT_CHECK([ip link add veth0 type veth peer name veth1 || exit 77])
> 
> Since we're here, we should probably re-name these interfaces to
> something more specific.  I can imagine a scenario where veth0
> port already exists in the system.
> Maybe s/veth/ovs_tc_pps/ or something like that?
> 
>> +    AT_CHECK([
>> +        if ! tc qdisc add dev veth0 handle ffff: ingress; then
>> +            ip link del veth0
> 
> Instead of deleting in every branch, we might add on_exit call somewhere
> at the beginning of this function.  The port will stick around till the
> end of the test, but I'm not sure if that's a problem if the name is
> special enough.  Or we may keep one extra del at the very end.
> 
> With on_exit, can probably be transformed into something like (untested):
> 
>   AT_CHECK([tc qdisc add dev veth0 handle ffff: ingress || exit 77])

Just AT_SKIP_IF([! tc qdisc add dev veth0 handle ffff: ingress]) should
work, I guess.

> 
> What do you think?
> 
>> +            exit 77
>> +        fi
>> +    ])
>> +    AT_CHECK([
>> +        if ! tc filter add dev veth0 parent ffff: \
> 
> 'dnl' is prefered for the line break.  autotest refuses to print out
> commands with '\' in them.
> 
>> +                 u32 match u32 0 0 police pkts_rate 100 pkts_burst 10; then
>> +            ip link del veth0
>> +            exit 77
>> +        fi
>> +    ])
>> +    ip link del veth0
>> +])
>>  
>>  AT_SETUP([offloads - ping between two ports - offloads disabled])
>>  OVS_TRAFFIC_VSWITCHD_START()
>> @@ -132,7 +151,7 @@ AT_CLEANUP
>>  
>>  AT_SETUP([offloads - set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst - offloads disabled])
>>  AT_KEYWORDS([ingress_policing_kpkts])
>> -AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
>> +CHECK_TC_INGRESS_PPS()
>>  OVS_TRAFFIC_VSWITCHD_START()
>>  AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=false])
>>  AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
>> @@ -156,7 +175,7 @@ AT_CLEANUP
>>  
>>  AT_SETUP([offloads - set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst - offloads enabled])
>>  AT_KEYWORDS([ingress_policing_kpkts])
>> -AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
>> +CHECK_TC_INGRESS_PPS()
>>  OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . other_config:hw-offload=true])
>>  AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
>>  ADD_NAMESPACES(at_ns0)
>

On Thu, Feb 23, 2023 at 03:19:37PM +0100, Ilya Maximets wrote:
> On 2/23/23 14:02, Simon Horman wrote:
> > Move check for tc ingress pps support to from aclocal to test script
> > 
> > This has several problems:
> > 
> > 1. Stderror from failing commands is output when executing
> >    various make targets.
> > 
> > 2. There are various failure conditions that lead
> >    to veth0 and veth1 being created by not cleaned up.
> > 
> > 3. The check seems to execute for many make targets.
> >    And it attempts to temporarily modify system state.
> >    This seems inappopriate.
> > 
> > All these problems are addressed by this patch.
> 
> Thanks, Simon!  These are indeed very annoying problems.
> 
> I didn't test, but I have a few minor comment inline.
> 
> Best regards, Ilya Maximets.
> 
> > 
> > Signed-off-by: Simon Horman <simon.horman@corigine.com>
> > Reviewed-by: Louis Peens <louis.peens@corigine.com>
> > ---
> >  tests/atlocal.in                 | 11 -----------
> >  tests/system-offloads-traffic.at | 23 +++++++++++++++++++++--
> >  2 files changed, 21 insertions(+), 13 deletions(-)
> > 
> > diff --git a/tests/atlocal.in b/tests/atlocal.in
> > index e02248f6f829..859668586299 100644
> > --- a/tests/atlocal.in
> > +++ b/tests/atlocal.in
> > @@ -172,17 +172,6 @@ fi
> >  # Set HAVE_TC
> >  find_command tc
> >  
> > -# When HAVE_TC=yes, check if the current tc supports adding pps filter
> > -SUPPORT_TC_INGRESS_PPS="no"
> > -if test $HAVE_TC="yes"; then
> > -    ip link add veth0 type veth peer name veth1
> > -    tc qdisc add dev veth0 handle ffff: ingress
> > -    if tc filter add dev veth0 parent ffff: u32 match u32 0 0 police pkts_rate 100 pkts_burst 10; then
> > -        SUPPORT_TC_INGRESS_PPS="yes"
> > -    fi
> > -    ip link del veth0
> > -fi
> > -
> >  # Set HAVE_TCPDUMP
> >  find_command tcpdump
> >  
> > diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
> > index f2bf9c0639aa..ff3e4c63d127 100644
> > --- a/tests/system-offloads-traffic.at
> > +++ b/tests/system-offloads-traffic.at
> > @@ -18,6 +18,25 @@ m4_define([OVS_CHECK_ACTIONS], [
> >             [0], [$1])
> >  ])
> >  
> > +m4_define([CHECK_TC_INGRESS_PPS],
> > +[
> > +    AT_SKIP_IF([test $HAVE_TC = "no"])
> > +    AT_CHECK([ip link add veth0 type veth peer name veth1 || exit 77])
> 
> Since we're here, we should probably re-name these interfaces to
> something more specific.  I can imagine a scenario where veth0
> port already exists in the system.
> Maybe s/veth/ovs_tc_pps/ or something like that?

Yes, good idea.

Actually, I had noticed that too.
But forgot to do anything about it.

> > +    AT_CHECK([
> > +        if ! tc qdisc add dev veth0 handle ffff: ingress; then
> > +            ip link del veth0
> 
> Instead of deleting in every branch, we might add on_exit call somewhere
> at the beginning of this function.  The port will stick around till the
> end of the test, but I'm not sure if that's a problem if the name is
> special enough.  Or we may keep one extra del at the very end.
> 
> With on_exit, can probably be transformed into something like (untested):
> 
>   AT_CHECK([tc qdisc add dev veth0 handle ffff: ingress || exit 77])
> 
> What do you think?

Yes, I thought you might say that.
It is certainly the ugliest part of this patch.

I was concerned that the cleanup would happen to late
and the veth0/veth1 scheme may conflict.

But if we move to a different scheme, as you suggested above,
then on_exit may work. I will give it a try.

> 
> > +            exit 77
> > +        fi
> > +    ])
> > +    AT_CHECK([
> > +        if ! tc filter add dev veth0 parent ffff: \
> 
> 'dnl' is prefered for the line break.  autotest refuses to print out
> commands with '\' in them.
> 
> > +                 u32 match u32 0 0 police pkts_rate 100 pkts_burst 10; then
> > +            ip link del veth0
> > +            exit 77
> > +        fi
> > +    ])
> > +    ip link del veth0
> > +])
> >  
> >  AT_SETUP([offloads - ping between two ports - offloads disabled])
> >  OVS_TRAFFIC_VSWITCHD_START()
> > @@ -132,7 +151,7 @@ AT_CLEANUP
> >  
> >  AT_SETUP([offloads - set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst - offloads disabled])
> >  AT_KEYWORDS([ingress_policing_kpkts])
> > -AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
> > +CHECK_TC_INGRESS_PPS()
> >  OVS_TRAFFIC_VSWITCHD_START()
> >  AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=false])
> >  AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
> > @@ -156,7 +175,7 @@ AT_CLEANUP
> >  
> >  AT_SETUP([offloads - set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst - offloads enabled])
> >  AT_KEYWORDS([ingress_policing_kpkts])
> > -AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
> > +CHECK_TC_INGRESS_PPS()
> >  OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . other_config:hw-offload=true])
> >  AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
> >  ADD_NAMESPACES(at_ns0)
>

On Thu, Feb 23, 2023 at 04:09:10PM +0100, Ilya Maximets wrote:
> On 2/23/23 15:19, Ilya Maximets wrote:
> > On 2/23/23 14:02, Simon Horman wrote:

...

> >> +    AT_CHECK([
> >> +        if ! tc qdisc add dev veth0 handle ffff: ingress; then
> >> +            ip link del veth0
> > 
> > Instead of deleting in every branch, we might add on_exit call somewhere
> > at the beginning of this function.  The port will stick around till the
> > end of the test, but I'm not sure if that's a problem if the name is
> > special enough.  Or we may keep one extra del at the very end.
> > 
> > With on_exit, can probably be transformed into something like (untested):
> > 
> >   AT_CHECK([tc qdisc add dev veth0 handle ffff: ingress || exit 77])
> 
> Just AT_SKIP_IF([! tc qdisc add dev veth0 handle ffff: ingress]) should
> work, I guess.

The reason I used AT_CHECK rather than AT_SKIP_IF was to
capture stderr/stdout. Which serves two purposes:

1. It avoids mess on the console - although that could also
   be avoided with something like >& /dev/null
2. The capture output is useful in debugging
   e.g. when developing this script.

I can move to AT_SKIP_IF if you have a strong preference.
But AT_CHECK did seem a bit better to me - except the '77' bit.

...

On 2/23/23 16:33, Simon Horman wrote:
> On Thu, Feb 23, 2023 at 04:09:10PM +0100, Ilya Maximets wrote:
>> On 2/23/23 15:19, Ilya Maximets wrote:
>>> On 2/23/23 14:02, Simon Horman wrote:
> 
> ...
> 
>>>> +    AT_CHECK([
>>>> +        if ! tc qdisc add dev veth0 handle ffff: ingress; then
>>>> +            ip link del veth0
>>>
>>> Instead of deleting in every branch, we might add on_exit call somewhere
>>> at the beginning of this function.  The port will stick around till the
>>> end of the test, but I'm not sure if that's a problem if the name is
>>> special enough.  Or we may keep one extra del at the very end.
>>>
>>> With on_exit, can probably be transformed into something like (untested):
>>>
>>>   AT_CHECK([tc qdisc add dev veth0 handle ffff: ingress || exit 77])
>>
>> Just AT_SKIP_IF([! tc qdisc add dev veth0 handle ffff: ingress]) should
>> work, I guess.
> 
> The reason I used AT_CHECK rather than AT_SKIP_IF was to
> capture stderr/stdout. Which serves two purposes:
> 
> 1. It avoids mess on the console - although that could also
>    be avoided with something like >& /dev/null
> 2. The capture output is useful in debugging
>    e.g. when developing this script.
> 
> I can move to AT_SKIP_IF if you have a strong preference.
> But AT_CHECK did seem a bit better to me - except the '77' bit.

Makes sense.  AT_CHECK is fine then.

Best regards, Ilya Maximets.

On Thu, Feb 23, 2023 at 04:36:25PM +0100, Ilya Maximets wrote:
> On 2/23/23 16:33, Simon Horman wrote:
> > On Thu, Feb 23, 2023 at 04:09:10PM +0100, Ilya Maximets wrote:
> >> On 2/23/23 15:19, Ilya Maximets wrote:
> >>> On 2/23/23 14:02, Simon Horman wrote:
> > 
> > ...
> > 
> >>>> +    AT_CHECK([
> >>>> +        if ! tc qdisc add dev veth0 handle ffff: ingress; then
> >>>> +            ip link del veth0
> >>>
> >>> Instead of deleting in every branch, we might add on_exit call somewhere
> >>> at the beginning of this function.  The port will stick around till the
> >>> end of the test, but I'm not sure if that's a problem if the name is
> >>> special enough.  Or we may keep one extra del at the very end.
> >>>
> >>> With on_exit, can probably be transformed into something like (untested):
> >>>
> >>>   AT_CHECK([tc qdisc add dev veth0 handle ffff: ingress || exit 77])
> >>
> >> Just AT_SKIP_IF([! tc qdisc add dev veth0 handle ffff: ingress]) should
> >> work, I guess.
> > 
> > The reason I used AT_CHECK rather than AT_SKIP_IF was to
> > capture stderr/stdout. Which serves two purposes:
> > 
> > 1. It avoids mess on the console - although that could also
> >    be avoided with something like >& /dev/null
> > 2. The capture output is useful in debugging
> >    e.g. when developing this script.
> > 
> > I can move to AT_SKIP_IF if you have a strong preference.
> > But AT_CHECK did seem a bit better to me - except the '77' bit.
> 
> Makes sense.  AT_CHECK is fine then.

Thanks, I'll prepare v2 with the other changes you suggested.

On Thu, Feb 23, 2023 at 03:19:37PM +0100, Ilya Maximets wrote:
> On 2/23/23 14:02, Simon Horman wrote:

...

> > +            exit 77
> > +        fi
> > +    ])
> > +    AT_CHECK([
> > +        if ! tc filter add dev veth0 parent ffff: \
> 
> 'dnl' is prefered for the line break.  autotest refuses to print out
> commands with '\' in them.

Thanks. I didn't know that.
And the code above causes the check to fail - i.e. the test
is always skipped. Oops.