[RFC PATCH net-next 0/5] selftests: forwarding: Introduce deferred commands

[RFC PATCH net-next 0/5] selftests: forwarding: Introduce deferred commands

[Petr Machata]

Recently, a defer helper was added to Python selftests. The idea is to keep
cleanup commands close to their dirtying counterparts, thereby making it
more transparent what is cleaning up what, making it harder to miss a
cleanup, and make the whole cleanup business exception safe. All these
benefits are applicable to bash as well, exception safety can be
interpreted in terms of safety vs. a SIGINT.

This patchset therefore introduces a framework of several helpers that
serve to schedule cleanups in bash selftests.


As a personal remark. More than once was I bit by stop_traffic not getting
invoked because I C-c'd a traffic scheduler selftest at the wrong time.
This would leave behind a running mausezahn that would break follow-up runs
of the script that I was just debugging, making me question my sanity.
("How did this one extra debug print break the full script? And when I
remove it again, _why is it still broken_?") This is an attempt at
squashing this whole class of problems.


Patch #1 has more details about the primitives being introduced.

Patches #2 to #5 the convert several selftests to give an idea of how it
looks in practice.

Petr Machata (5):
  selftests: forwarding: Introduce deferred commands
  selftests: mlxsw: sch_red_core: Use defer for test cleanup
  selftests: mlxsw: sch_red_core: Use defer for stopping traffic
  selftests: mlxsw: sch_red_*: Use defer for qdisc management
  selftests: sch_tbf_core: Use defer for stopping traffic

 .../drivers/net/mlxsw/sch_red_core.sh         | 131 +++++++-----------
 .../drivers/net/mlxsw/sch_red_ets.sh          |  32 ++---
 .../drivers/net/mlxsw/sch_red_root.sh         |  24 +++-
 tools/testing/selftests/net/forwarding/lib.sh |  83 +++++++++++
 .../selftests/net/forwarding/sch_tbf_core.sh  |   3 +-
 5 files changed, 170 insertions(+), 103 deletions(-)

-- 
2.45.0

[RFC PATCH net-next 1/5] selftests: forwarding: Introduce deferred commands

[Petr Machata]

In commit 8510801a9dbd ("selftests: drv-net: add ability to schedule
cleanup with defer()"), a defer helper was added to Python selftests.
The idea is to keep cleanup commands close to their dirtying counterparts,
thereby making it more transparent what is cleaning up what, making it
harder to miss a cleanup, and make the whole cleanup business exception
safe. All these benefits are applicable to bash as well, exception safety
can be interpreted in terms of safety vs. a SIGINT.

This patch therefore introduces a framework of several helpers that serve
to schedule cleanups in bash selftests:

- defer_scope_push(), defer_scope_pop(): Deferred statements can be batched
  together in scopes. When a scope is popped, the deferred commands
  schoduled in that scope are executed in the order opposite to order of
  their scheduling.

- defer(): Schedules a defer to the most recently pushed scope (or the
  default scope if none was pushed.)

- defer_scopes_cleanup(): Pops any unpopped scopes, including the default
  one. The selftests that use defer should run this in their cleanup
  function. This is important to get cleanups of interrupted scripts.

  Consistent use of defers however obviates the need for a separate cleanup
  function -- everything is just taken care of in defers. So this patch
  actually introduces a cleanup() helper in the forwarding lib.sh, which
  calls just pre_cleanup() and defer_scopes_cleanup(). Selftests are
  obviously still free to override the function.

- defer_scoped_fn(): Sometimes a function would like to introduce a new
  defer scope, then run whatever it is that it wants to run, and then pop
  the scope to run the deferred cleanups. The helper defer_scoped_fn() can
  be used to derive from one function its wrapper that pushes a defer scope
  before the function is called, and pops it after it returns.

The following patches will convert several selftests to this new framework.

Signed-off-by: Petr Machata <petrm@nvidia.com>
---
 tools/testing/selftests/net/forwarding/lib.sh | 83 +++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 67f38dd1f36b..21cd6a2e3344 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -1369,6 +1369,12 @@ tests_run()
 	done
 }
 
+cleanup()
+{
+	pre_cleanup
+	defer_scopes_cleanup
+}
+
 multipath_eval()
 {
 	local desc="$1"
@@ -1423,6 +1429,83 @@ in_ns()
 	EOF
 }
 
+# map[(defer_scope,cleanup_id) -> cleanup_command]
+declare -A DEFERS
+# map[defer_scope -> # cleanup_commands]
+declare -a NDEFERS=(0)
+DEFER_SCOPE=0
+
+defer_scope_push()
+{
+	((DEFER_SCOPE++))
+	NDEFERS[${DEFER_SCOPE}]=0
+}
+
+defer_scope_pop()
+{
+	local defer_key
+	local defer_ix
+
+	for ((defer_ix=${NDEFERS[${DEFER_SCOPE}]}; defer_ix-->0; )); do
+		defer_key=${DEFER_SCOPE},$defer_ix
+		${DEFERS[$defer_key]}
+		unset DEFERS[$defer_key]
+	done
+
+	NDEFERS[${DEFER_SCOPE}]=0
+	((DEFER_SCOPE--))
+}
+
+defer()
+{
+	local defer_key=${DEFER_SCOPE},${NDEFERS[${DEFER_SCOPE}]}
+	local defer="$@"
+
+	DEFERS[$defer_key]="$defer"
+	NDEFERS[${DEFER_SCOPE}]=$((${NDEFERS[${DEFER_SCOPE}]} + 1))
+}
+
+defer_scopes_cleanup()
+{
+	while ((DEFER_SCOPE >= 0)); do
+		defer_scope_pop
+	done
+}
+
+defer_scoped_fn()
+{
+	local name=$1; shift;
+	local mangle=__defer_scoped__
+
+	declare -f $name >/dev/null
+	if (($?)); then
+		echo "Cannot make non-existent function '$name' defer-scoped" \
+			> /dev/stderr
+		exit 1
+	fi
+
+	declare -f $mangle$name
+	if ((! $?)); then
+		echo "The function '$name' appears to already be defer-scoped" \
+			> /dev/stderr
+		exit 1
+	fi
+
+	eval "$mangle$(declare -f $name)"
+	local body="
+		$name() {
+			local ret;
+			defer_scope_push;
+			$mangle$name \"\$@\";
+			ret=\$?;
+			defer_scope_pop;
+			return \$ret;
+		}
+	"
+	unset $name
+	eval "$body"
+}
+
 ##############################################################################
 # Tests
 
-- 
2.45.0

Re: [RFC PATCH net-next 1/5] selftests: forwarding: Introduce deferred commands

[Ido Schimmel]

On Thu, Aug 22, 2024 at 03:49:40PM +0200, Petr Machata wrote:
> In commit 8510801a9dbd ("selftests: drv-net: add ability to schedule
> cleanup with defer()"), a defer helper was added to Python selftests.
> The idea is to keep cleanup commands close to their dirtying counterparts,
> thereby making it more transparent what is cleaning up what, making it
> harder to miss a cleanup, and make the whole cleanup business exception
> safe. All these benefits are applicable to bash as well, exception safety
> can be interpreted in terms of safety vs. a SIGINT.
> 
> This patch therefore introduces a framework of several helpers that serve
> to schedule cleanups in bash selftests:
> 
> - defer_scope_push(), defer_scope_pop(): Deferred statements can be batched
>   together in scopes. When a scope is popped, the deferred commands
>   schoduled in that scope are executed in the order opposite to order of

s/schoduled/scheduled/

>   their scheduling.
> 
> - defer(): Schedules a defer to the most recently pushed scope (or the
>   default scope if none was pushed.)
> 
> - defer_scopes_cleanup(): Pops any unpopped scopes, including the default
>   one. The selftests that use defer should run this in their cleanup
>   function. This is important to get cleanups of interrupted scripts.
> 
>   Consistent use of defers however obviates the need for a separate cleanup
>   function -- everything is just taken care of in defers. So this patch
>   actually introduces a cleanup() helper in the forwarding lib.sh, which
>   calls just pre_cleanup() and defer_scopes_cleanup(). Selftests are
>   obviously still free to override the function.
> 
> - defer_scoped_fn(): Sometimes a function would like to introduce a new
>   defer scope, then run whatever it is that it wants to run, and then pop
>   the scope to run the deferred cleanups. The helper defer_scoped_fn() can
>   be used to derive from one function its wrapper that pushes a defer scope
>   before the function is called, and pops it after it returns.
> 
> The following patches will convert several selftests to this new framework.

The intention is to make sure new tests are using these helpers?

> 
> Signed-off-by: Petr Machata <petrm@nvidia.com>
> ---
>  tools/testing/selftests/net/forwarding/lib.sh | 83 +++++++++++++++++++

Does it make sense to place these helpers in net/lib.sh?

Re: [RFC PATCH net-next 1/5] selftests: forwarding: Introduce deferred commands

[Petr Machata]

Ido Schimmel <idosch@nvidia.com> writes:

> On Thu, Aug 22, 2024 at 03:49:40PM +0200, Petr Machata wrote:
>> The following patches will convert several selftests to this new framework.
>
> The intention is to make sure new tests are using these helpers?

Well, I sent this as RFC because I'm not sure how far to push it. I
think it would be ideal if this were adopted, because then cleanups
either always work, or are always broken, and we don't get partial or
forgotten cleanups. That's for new tests, I do not foresee converting
the existing selftests beyond a couple examples.

>> 
>> Signed-off-by: Petr Machata <petrm@nvidia.com>
>> ---
>>  tools/testing/selftests/net/forwarding/lib.sh | 83 +++++++++++++++++++
>
> Does it make sense to place these helpers in net/lib.sh?

Yeah, it does.

Re: [RFC PATCH net-next 1/5] selftests: forwarding: Introduce deferred commands

[Jakub Kicinski]

On Mon, 26 Aug 2024 16:25:47 +0200 Petr Machata wrote:
> >>  tools/testing/selftests/net/forwarding/lib.sh | 83 +++++++++++++++++++  
> >
> > Does it make sense to place these helpers in net/lib.sh?  
> 
> Yeah, it does.

Would it further make sense to split them to their own file
(net/lib_defer.sh?) and source that in net/lib.sh?

Should be pretty self-contained.

Re: [RFC PATCH net-next 1/5] selftests: forwarding: Introduce deferred commands

[Jakub Kicinski]

On Mon, 26 Aug 2024 13:03:43 -0700 Jakub Kicinski wrote:
> > > Does it make sense to place these helpers in net/lib.sh?    
> > 
> > Yeah, it does.  
> 
> Would it further make sense to split them to their own file
> (net/lib_defer.sh?) and source that in net/lib.sh?
> 
> Should be pretty self-contained.

Just saw your reply to Przemek, makes sense.

Re: [RFC PATCH net-next 1/5] selftests: forwarding: Introduce deferred commands

[Przemek Kitszel]

On 8/22/24 15:49, Petr Machata wrote:
> In commit 8510801a9dbd ("selftests: drv-net: add ability to schedule
> cleanup with defer()"), a defer helper was added to Python selftests.
> The idea is to keep cleanup commands close to their dirtying counterparts,
> thereby making it more transparent what is cleaning up what, making it
> harder to miss a cleanup, and make the whole cleanup business exception
> safe. All these benefits are applicable to bash as well, exception safety
> can be interpreted in terms of safety vs. a SIGINT.
> 
> This patch therefore introduces a framework of several helpers that serve
> to schedule cleanups in bash selftests:

Thank you for working on that, it would be great to have such
improvement for bash scripts in general, not limited to kselftests!

>   tools/testing/selftests/net/forwarding/lib.sh | 83 +++++++++++++++++++

Make it a new file in more generic location, add a comment section with
some examples and write down any assumptions there, perhaps defer.sh?

> - defer_scope_push(), defer_scope_pop(): Deferred statements can be batched >    together in scopes. When a scope is popped, the deferred commands
>    schoduled in that scope are executed in the order opposite to order of
>    their scheduling.

tldr of this sub-comment at the end

such API could be used in two variants:

1)
function test_executor1() {
	for t in tests; do
		defer_scope_push()
		exec_test1 $t
		defer_scope_pop()
	done
}

2)
function test_executor2() {
	for t in tests; do
		exec_test2 $t
	done
}
function exec_test2() {
	defer_scope_push()
	do_stuff "$@"
	defer_scope_pop()
}

That fractals down in the same way for "subtests", or some special stuff
like "make a zip" sub/task that will be used. And it could be misused as
a mix of the two variants.
I believe that the 1) is the better way, rationale: you write normal
code that does what needs to be done, using defer(), and caller (that
knows better) decides whether to sub-scope.
As this defer is very similar to golang's in intention, I would give
yet another analogy from golang's world. It's similar to concurrency, 
you write normal code that could be parallelized via "go" keyword,
instead of writing async code that needs to be awaited for.

Going back to the use case variants, there is no much sense to have
push() and pop() dispersed by much from each other, thus I would like
to introduce an API that just combines the two instead:

new_scope exec_test1 $t
(name discussion below)

> 
> - defer(): Schedules a defer to the most recently pushed scope (or the
>    default scope if none was pushed. >
> - defer_scopes_cleanup(): Pops any unpopped scopes, including the default
>    one. The selftests that use defer should run this in their cleanup
>    function. This is important to get cleanups of interrupted scripts.

this should be *the* trap(1)

with that said, it should be internal to the defer.sh script and it
should be obvious that developers must not introduce their own trap
(as of now we have ~330 in kselftests, ~270 of which in networking)

> 
>    Consistent use of defers however obviates the need for a separate cleanup
>    function -- everything is just taken care of in defers. So this patch
>    actually introduces a cleanup() helper in the forwarding lib.sh, which
>    calls just pre_cleanup() and defer_scopes_cleanup(). Selftests are
>    obviously still free to override the function.
> 
> - defer_scoped_fn(): Sometimes a function would like to introduce a new
>    defer scope, then run whatever it is that it wants to run, and then pop
>    the scope to run the deferred cleanups. The helper defer_scoped_fn() can
>    be used to derive from one function its wrapper that pushes a defer scope
>    before the function is called, and pops it after it returns.

It is basically a helper I would like to see as new_scope() mentioned
above, but it takes it upside down - it should really be the caller that
sub-scopes.


I think that the name of the new_scope() would be better, still concise,
but more precise as:
subscope_defer(),
trapped(), or
sub_trap().

I have no idea how to make a sub-trapped, SIGSEGV isolated scope of bash
execution that has ability to still edit outer scope variables. Perhaps
we could relax the need for edit to have easier implementation? It is
"all ok or failure/rollback" mode of operation anyway most of the time.

After the above parts will be discussed out I will look more into the
details of the code more deeply.

Re: [RFC PATCH net-next 1/5] selftests: forwarding: Introduce deferred commands

[Petr Machata]

Przemek Kitszel <przemyslaw.kitszel@intel.com> writes:

> On 8/22/24 15:49, Petr Machata wrote:
>> In commit 8510801a9dbd ("selftests: drv-net: add ability to schedule
>> cleanup with defer()"), a defer helper was added to Python selftests.
>> The idea is to keep cleanup commands close to their dirtying counterparts,
>> thereby making it more transparent what is cleaning up what, making it
>> harder to miss a cleanup, and make the whole cleanup business exception
>> safe. All these benefits are applicable to bash as well, exception safety
>> can be interpreted in terms of safety vs. a SIGINT.
>> This patch therefore introduces a framework of several helpers that serve
>> to schedule cleanups in bash selftests:
>
> Thank you for working on that, it would be great to have such
> improvement for bash scripts in general, not limited to kselftests!
>
>>   tools/testing/selftests/net/forwarding/lib.sh | 83 +++++++++++++++++++
>
> Make it a new file in more generic location, add a comment section with
> some examples and write down any assumptions there, perhaps defer.sh?

I can do it, but it's gonna be more pain in setting up those
TEST_INCLUDES. People will forget. It will be a nuisance.

I'm thinking of just moving it to net/lib.sh, from forwarding.

>> - defer_scope_push(), defer_scope_pop(): Deferred statements can be batched > together in scopes.
>> When a scope is popped, the deferred commands
>>    schoduled in that scope are executed in the order opposite to order of
>>    their scheduling.
>
> tldr of this sub-comment at the end
>
> such API could be used in two variants:
>
> 1)
> function test_executor1() {
> 	for t in tests; do
> 		defer_scope_push()
> 		exec_test1 $t
> 		defer_scope_pop()
> 	done
> }
>
> 2)
> function test_executor2() {
> 	for t in tests; do
> 		exec_test2 $t
> 	done
> }
> function exec_test2() {
> 	defer_scope_push()
> 	do_stuff "$@"
> 	defer_scope_pop()
> }
>
> That fractals down in the same way for "subtests", or some special stuff
> like "make a zip" sub/task that will be used. And it could be misused as
> a mix of the two variants.
> I believe that the 1) is the better way, rationale: you write normal
> code that does what needs to be done, using defer(), and caller (that
> knows better) decides whether to sub-scope.

But the caller does not know better. The cleanups can't be done
"sometime", but at a predictable place, so that they don't end up
interfering with other work. The callee knows where it needs the
cleanups to happen. The caller shouldn't have to know.

> As this defer is very similar to golang's in intention, I would give
> yet another analogy from golang's world. It's similar to concurrency, you write normal code that
> could be parallelized via "go" keyword,
> instead of writing async code that needs to be awaited for.

Notice how in go, defer also runs at function exit. Similarly with C++
destructors, run on scope exit. There's no caller-defined "collection
point".

Putting off until "sometime" works for memory. Things like garbage
collection, obstacks, autorelease pools, etc. work, because there's
plenty of memory and we don't mind keeping stuff around until later. But
that doesn't work for the sort of cleanups that selftests typically need
to do.

> Going back to the use case variants, there is no much sense to have
> push() and pop() dispersed by much from each other, thus I would like
> to introduce an API that just combines the two instead:
>
> new_scope exec_test1 $t
> (name discussion below)
>
>> - defer(): Schedules a defer to the most recently pushed scope (or the
>>    default scope if none was pushed. >
>> - defer_scopes_cleanup(): Pops any unpopped scopes, including the default
>>    one. The selftests that use defer should run this in their cleanup
>>    function. This is important to get cleanups of interrupted scripts.
>
> this should be *the* trap(1)
>
> with that said, it should be internal to the defer.sh script and it
> should be obvious that developers must not introduce their own trap
> (as of now we have ~330 in kselftests, ~270 of which in networking)

Yeah, we have 100+ tests that use their own traps in forwarding alone.
That ship has sailed.

I agree that the defer module probably has the "right" to own the exit
trap. Any other cleanups can be expressed in terms of defer, and I don't
know if there are legitimate uses of exit trap with that taken out. But
that's for sometime.

>>    Consistent use of defers however obviates the need for a separate cleanup
>>    function -- everything is just taken care of in defers. So this patch
>>    actually introduces a cleanup() helper in the forwarding lib.sh, which
>>    calls just pre_cleanup() and defer_scopes_cleanup(). Selftests are
>>    obviously still free to override the function.
>> - defer_scoped_fn(): Sometimes a function would like to introduce a new
>>    defer scope, then run whatever it is that it wants to run, and then pop
>>    the scope to run the deferred cleanups. The helper defer_scoped_fn() can
>>    be used to derive from one function its wrapper that pushes a defer scope
>>    before the function is called, and pops it after it returns.
>
> It is basically a helper I would like to see as new_scope() mentioned
> above, but it takes it upside down - it should really be the caller that
> sub-scopes.
>
> I think that the name of the new_scope() would be better, still concise,
> but more precise as:
> subscope_defer(),
> trapped(), or
> sub_trap().
>
> I have no idea how to make a sub-trapped, SIGSEGV isolated scope of bash
> execution that has ability to still edit outer scope variables. Perhaps
> we could relax the need for edit to have easier implementation? It is
> "all ok or failure/rollback" mode of operation anyway most of the time.

I'm not sure what you have in mind.

> After the above parts will be discussed out I will look more into the
> details of the code more deeply.

Re: [RFC PATCH net-next 1/5] selftests: forwarding: Introduce deferred commands

[Przemek Kitszel]

On 8/26/24 17:20, Petr Machata wrote:
> 
> Przemek Kitszel <przemyslaw.kitszel@intel.com> writes:
> 
>> On 8/22/24 15:49, Petr Machata wrote:
>>> In commit 8510801a9dbd ("selftests: drv-net: add ability to schedule
>>> cleanup with defer()"), a defer helper was added to Python selftests.
>>> The idea is to keep cleanup commands close to their dirtying counterparts,
>>> thereby making it more transparent what is cleaning up what, making it
>>> harder to miss a cleanup, and make the whole cleanup business exception
>>> safe. All these benefits are applicable to bash as well, exception safety
>>> can be interpreted in terms of safety vs. a SIGINT.
>>> This patch therefore introduces a framework of several helpers that serve
>>> to schedule cleanups in bash selftests:
>>
>> Thank you for working on that, it would be great to have such
>> improvement for bash scripts in general, not limited to kselftests!
>>
>>>    tools/testing/selftests/net/forwarding/lib.sh | 83 +++++++++++++++++++
>>
>> Make it a new file in more generic location, add a comment section with
>> some examples and write down any assumptions there, perhaps defer.sh?
> 
> I can do it, but it's gonna be more pain in setting up those
> TEST_INCLUDES. People will forget. It will be a nuisance.
> 
> I'm thinking of just moving it to net/lib.sh, from forwarding.

what about separate file, but included from net/lib.sh?

> 
>>> - defer_scope_push(), defer_scope_pop(): Deferred statements can be batched > together in scopes.
>>> When a scope is popped, the deferred commands
>>>     schoduled in that scope are executed in the order opposite to order of
>>>     their scheduling.
>>
>> tldr of this sub-comment at the end
>>
>> such API could be used in two variants:
>>
>> 1)
>> function test_executor1() {
>> 	for t in tests; do
>> 		defer_scope_push()
>> 		exec_test1 $t
>> 		defer_scope_pop()
>> 	done
>> }
>>
>> 2)
>> function test_executor2() {
>> 	for t in tests; do
>> 		exec_test2 $t
>> 	done
>> }
>> function exec_test2() {
>> 	defer_scope_push()
>> 	do_stuff "$@"
>> 	defer_scope_pop()
>> }
>>
>> That fractals down in the same way for "subtests", or some special stuff
>> like "make a zip" sub/task that will be used. And it could be misused as
>> a mix of the two variants.
>> I believe that the 1) is the better way, rationale: you write normal
>> code that does what needs to be done, using defer(), and caller (that
>> knows better) decides whether to sub-scope.
> 
> But the caller does not know better. The cleanups can't be done
> "sometime", but at a predictable place, so that they don't end up
> interfering with other work. The callee knows where it needs the
> cleanups to happen. The caller shouldn't have to know.

The caller should not have to know what will be cleaned, but knows that
they are done with callee.
OTOH, callee has no idea about the "other work".

> 
>> As this defer is very similar to golang's in intention, I would give
>> yet another analogy from golang's world. It's similar to concurrency, you write normal code that
>> could be parallelized via "go" keyword,
>> instead of writing async code that needs to be awaited for.
> 
> Notice how in go, defer also runs at function exit. Similarly with C++
> destructors, run on scope exit. There's no caller-defined "collection
> point".
> 
> Putting off until "sometime" works for memory. Things like garbage
> collection, obstacks, autorelease pools, etc. work, because there's
> plenty of memory and we don't mind keeping stuff around until later. But
> that doesn't work for the sort of cleanups that selftests typically need
> to do.

That's true. But I still believe that it's the caller (or better, "glue
code") responsibility to take care of cleanup schedule.

> 
>> Going back to the use case variants, there is no much sense to have
>> push() and pop() dispersed by much from each other, thus I would like
>> to introduce an API that just combines the two instead:
>>
>> new_scope exec_test1 $t
>> (name discussion below)
>>
>>> - defer(): Schedules a defer to the most recently pushed scope (or the
>>>     default scope if none was pushed. >
>>> - defer_scopes_cleanup(): Pops any unpopped scopes, including the default
>>>     one. The selftests that use defer should run this in their cleanup
>>>     function. This is important to get cleanups of interrupted scripts.
>>
>> this should be *the* trap(1)
>>
>> with that said, it should be internal to the defer.sh script and it
>> should be obvious that developers must not introduce their own trap
>> (as of now we have ~330 in kselftests, ~270 of which in networking)
> 
> Yeah, we have 100+ tests that use their own traps in forwarding alone.
> That ship has sailed.
> 
> I agree that the defer module probably has the "right" to own the exit
> trap. Any other cleanups can be expressed in terms of defer, and I don't
> know if there are legitimate uses of exit trap with that taken out. But
> that's for sometime.

There could be multiple traps for ERR/EXIT/etc conditions, but for
simplicity it's best to rely on just EXIT trap.
So we should convert current scripts one by one to use your new API.

> 
>>>     Consistent use of defers however obviates the need for a separate cleanup
>>>     function -- everything is just taken care of in defers. So this patch
>>>     actually introduces a cleanup() helper in the forwarding lib.sh, which
>>>     calls just pre_cleanup() and defer_scopes_cleanup(). Selftests are
>>>     obviously still free to override the function.
>>> - defer_scoped_fn(): Sometimes a function would like to introduce a new
>>>     defer scope, then run whatever it is that it wants to run, and then pop
>>>     the scope to run the deferred cleanups. The helper defer_scoped_fn() can
>>>     be used to derive from one function its wrapper that pushes a defer scope
>>>     before the function is called, and pops it after it returns.
>>
>> It is basically a helper I would like to see as new_scope() mentioned
>> above, but it takes it upside down - it should really be the caller that
>> sub-scopes.
>>
>> I think that the name of the new_scope() would be better, still concise,
>> but more precise as:
>> subscope_defer(),
>> trapped(), or
>> sub_trap().

here I mean that "scope" is too broad without the word "trap" or "defer"
in name

>>
>> I have no idea how to make a sub-trapped, SIGSEGV isolated scope of bash
>> execution that has ability to still edit outer scope variables. Perhaps
>> we could relax the need for edit to have easier implementation? It is
>> "all ok or failure/rollback" mode of operation anyway most of the time.
> 
> I'm not sure what you have in mind.

	foo=1
	function bumpfoo {
		maybe-crash
		foo=2
	}
	new-defer-scope bumpfoo
	echo $foo

do you want this to print 2 or 1?


> 
>> After the above parts will be discussed out I will look more into the
>> details of the code more deeply.
> 

Re: [RFC PATCH net-next 1/5] selftests: forwarding: Introduce deferred commands

[Petr Machata]

Przemek Kitszel <przemyslaw.kitszel@intel.com> writes:

> On 8/26/24 17:20, Petr Machata wrote:
>> Przemek Kitszel <przemyslaw.kitszel@intel.com> writes:
>> 
>>> On 8/22/24 15:49, Petr Machata wrote:
>>>> In commit 8510801a9dbd ("selftests: drv-net: add ability to schedule
>>>> cleanup with defer()"), a defer helper was added to Python selftests.
>>>> The idea is to keep cleanup commands close to their dirtying counterparts,
>>>> thereby making it more transparent what is cleaning up what, making it
>>>> harder to miss a cleanup, and make the whole cleanup business exception
>>>> safe. All these benefits are applicable to bash as well, exception safety
>>>> can be interpreted in terms of safety vs. a SIGINT.
>>>> This patch therefore introduces a framework of several helpers that serve
>>>> to schedule cleanups in bash selftests:
>>>
>>> Thank you for working on that, it would be great to have such
>>> improvement for bash scripts in general, not limited to kselftests!
>>>
>>>>    tools/testing/selftests/net/forwarding/lib.sh | 83 +++++++++++++++++++
>>>
>>> Make it a new file in more generic location, add a comment section with
>>> some examples and write down any assumptions there, perhaps defer.sh?
>> I can do it, but it's gonna be more pain in setting up those
>> TEST_INCLUDES. People will forget. It will be a nuisance.
>> I'm thinking of just moving it to net/lib.sh, from forwarding.
>
> what about separate file, but included from net/lib.sh?

Unfortunately that would be even worse. Then you need to remember to put
the file into TEST_INCLUDES despite seemingly not using it.

Like ideally we'd have automation for this. But I don't know how to do that
without actually parsing the bash files, and that's just asking for
trouble. Maybe after the defer stuff we also need a module system :-/

>>>> - defer_scope_push(), defer_scope_pop(): Deferred statements can be batched > together in scopes.
>>>> When a scope is popped, the deferred commands
>>>>     schoduled in that scope are executed in the order opposite to order of
>>>>     their scheduling.
>>>
>>> tldr of this sub-comment at the end
>>>
>>> such API could be used in two variants:
>>>
>>> 1)
>>> function test_executor1() {
>>> 	for t in tests; do
>>> 		defer_scope_push()
>>> 		exec_test1 $t
>>> 		defer_scope_pop()
>>> 	done
>>> }
>>>
>>> 2)
>>> function test_executor2() {
>>> 	for t in tests; do
>>> 		exec_test2 $t
>>> 	done
>>> }
>>> function exec_test2() {
>>> 	defer_scope_push()
>>> 	do_stuff "$@"
>>> 	defer_scope_pop()
>>> }
>>>
>>> That fractals down in the same way for "subtests", or some special stuff
>>> like "make a zip" sub/task that will be used. And it could be misused as
>>> a mix of the two variants.
>>> I believe that the 1) is the better way, rationale: you write normal
>>> code that does what needs to be done, using defer(), and caller (that
>>> knows better) decides whether to sub-scope.
>> But the caller does not know better. The cleanups can't be done
>> "sometime", but at a predictable place, so that they don't end up
>> interfering with other work. The callee knows where it needs the
>> cleanups to happen. The caller shouldn't have to know.
>
> The caller should not have to know what will be cleaned, but knows that
> they are done with callee.
>
> OTOH, callee has no idea about the "other work".

Nor should it have to. It just needs to dispose of all responsibilities it
has acquired (read: clean up what it has dirtied, or what others have
dirtied for it). That's done by closing the defer scope.

But let me take a step back. I've been going back and forth on this
basically since yesterday.

In practice, the caller-defined scopes lead to nicer code.

If run_tests creates an implicit scope per test, most of the tests can just
issue their defers without thinking about it too much.

For cases where the implicit scope is not enough, the caller has to know
that a certain function needs to be run in a dedicated scope or else it
will interfere with something else that it's running. That's not great, it
complicates the caller-callee contract in a way that's not captured
anywhere in the syntax. But I suspect it's going to be just fine, these
scripts are not exactly complex, and if there's an interference, I figure
it will be easy to notice.

The major upside is that we avoid the need to pepper the code with
defer_scoped_fn.

So I'll drop defer_scoped_fn and add in_defer_scope:

in_defer_scope()
{
	local ret

	defer_scope_push
	"$@"
	ret=$?
	defer_scope_pop

	return ret
}

>>> Going back to the use case variants, there is no much sense to have
>>> push() and pop() dispersed by much from each other, thus I would like
>>> to introduce an API that just combines the two instead:
>>>
>>> new_scope exec_test1 $t
>>> (name discussion below)
>>>
>>>> - defer(): Schedules a defer to the most recently pushed scope (or the
>>>>     default scope if none was pushed. >
>>>> - defer_scopes_cleanup(): Pops any unpopped scopes, including the default
>>>>     one. The selftests that use defer should run this in their cleanup
>>>>     function. This is important to get cleanups of interrupted scripts.
>>>
>>> this should be *the* trap(1)
>>>
>>> with that said, it should be internal to the defer.sh script and it
>>> should be obvious that developers must not introduce their own trap
>>> (as of now we have ~330 in kselftests, ~270 of which in networking)
>> Yeah, we have 100+ tests that use their own traps in forwarding alone.
>> That ship has sailed.
>> I agree that the defer module probably has the "right" to own the exit
>> trap. Any other cleanups can be expressed in terms of defer, and I don't
>> know if there are legitimate uses of exit trap with that taken out. But
>> that's for sometime.
>
> There could be multiple traps for ERR/EXIT/etc conditions, but for
> simplicity it's best to rely on just EXIT trap.
> So we should convert current scripts one by one to use your new API.

I'd just grandfather those in, but having this stuff consolidated would
obviously be nice.

I think in practice we just need to add the trap registration to
forwarding.sh, and per bash script do something like:

-trap cleanup EXIT
 setup_prepare
+defer cleanup
 setup_wait

It should be fairly mechanical most of the time. But the defer stuff works
without it as well, so we can take care of that later on.

>>>>     Consistent use of defers however obviates the need for a separate cleanup
>>>>     function -- everything is just taken care of in defers. So this patch
>>>>     actually introduces a cleanup() helper in the forwarding lib.sh, which
>>>>     calls just pre_cleanup() and defer_scopes_cleanup(). Selftests are
>>>>     obviously still free to override the function.
>>>> - defer_scoped_fn(): Sometimes a function would like to introduce a new
>>>>     defer scope, then run whatever it is that it wants to run, and then pop
>>>>     the scope to run the deferred cleanups. The helper defer_scoped_fn() can
>>>>     be used to derive from one function its wrapper that pushes a defer scope
>>>>     before the function is called, and pops it after it returns.
>>>
>>> It is basically a helper I would like to see as new_scope() mentioned
>>> above, but it takes it upside down - it should really be the caller that
>>> sub-scopes.
>>>
>>> I think that the name of the new_scope() would be better, still concise,
>>> but more precise as:
>>> subscope_defer(),
>>> trapped(), or
>>> sub_trap().
>
> here I mean that "scope" is too broad without the word "trap" or "defer"
> in name
>
>>>
>>> I have no idea how to make a sub-trapped, SIGSEGV isolated scope of bash
>>> execution that has ability to still edit outer scope variables. Perhaps
>>> we could relax the need for edit to have easier implementation? It is
>>> "all ok or failure/rollback" mode of operation anyway most of the time.
>> I'm not sure what you have in mind.
>
> 	foo=1
> 	function bumpfoo {
> 		maybe-crash
> 		foo=2
> 	}
> 	new-defer-scope bumpfoo
> 	echo $foo
>
> do you want this to print 2 or 1?

Oh, that's what you mean by relaxing the edits. Yeah, I think I'd want that
to print 2 if at all possible. I think in_ns() is the only helper that
violates this.

Re: [RFC PATCH net-next 1/5] selftests: forwarding: Introduce deferred commands

[Jakub Kicinski]

On Tue, 27 Aug 2024 10:53:53 +0200 Petr Machata wrote:
> >> I can do it, but it's gonna be more pain in setting up those
> >> TEST_INCLUDES. People will forget. It will be a nuisance.
> >> I'm thinking of just moving it to net/lib.sh, from forwarding.  
> >
> > what about separate file, but included from net/lib.sh?  
> 
> Unfortunately that would be even worse. Then you need to remember to put
> the file into TEST_INCLUDES despite seemingly not using it.
> 
> Like ideally we'd have automation for this. But I don't know how to do that
> without actually parsing the bash files, and that's just asking for
> trouble. Maybe after the defer stuff we also need a module system :-/

FWIW we could throw it into net/lib, which has a fake target, see:

b86761ff6374 ("selftests: net: add scaffolding for Netlink tests in Python")

Re: [RFC PATCH net-next 1/5] selftests: forwarding: Introduce deferred commands

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> On Tue, 27 Aug 2024 10:53:53 +0200 Petr Machata wrote:
>> >> I can do it, but it's gonna be more pain in setting up those
>> >> TEST_INCLUDES. People will forget. It will be a nuisance.
>> >> I'm thinking of just moving it to net/lib.sh, from forwarding.  
>> >
>> > what about separate file, but included from net/lib.sh?  
>> 
>> Unfortunately that would be even worse. Then you need to remember to put
>> the file into TEST_INCLUDES despite seemingly not using it.
>> 
>> Like ideally we'd have automation for this. But I don't know how to do that
>> without actually parsing the bash files, and that's just asking for
>> trouble. Maybe after the defer stuff we also need a module system :-/
>
> FWIW we could throw it into net/lib, which has a fake target, see:
>
> b86761ff6374 ("selftests: net: add scaffolding for Netlink tests in Python")

Oh, I see net/lib is the default dependency of everything net.
This could work. I'll check it out.

[RFC PATCH net-next 2/5] selftests: mlxsw: sch_red_core: Use defer for test cleanup

[Petr Machata]

Instead of having a suite of dedicated cleanup functions, use the defer
framework to schedule cleanups right as their setup functions are run.

This makes a dedicated cleanup() moot, instead fall back to the
lib.sh-provided one, which invokes the necessary defer cleanups as well.

Signed-off-by: Petr Machata <petrm@nvidia.com>
---
 .../drivers/net/mlxsw/sch_red_core.sh         | 109 ++++++------------
 1 file changed, 37 insertions(+), 72 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_red_core.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_red_core.sh
index 7151b0c7bf8d..117a99fdbba5 100644
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_red_core.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_red_core.sh
@@ -89,39 +89,31 @@ host_create()
 	local host=$1; shift
 
 	simple_if_init $dev
+	defer simple_if_fini $dev
+
 	mtu_set $dev 10000
+	defer mtu_restore $dev
 
 	vlan_create $dev 10 v$dev $(ipaddr $host 10)/28
+	defer vlan_destroy $dev 10
 	ip link set dev $dev.10 type vlan egress 0:0
 
 	vlan_create $dev 11 v$dev $(ipaddr $host 11)/28
+	defer vlan_destroy $dev 11
 	ip link set dev $dev.11 type vlan egress 0:1
 }
 
-host_destroy()
-{
-	local dev=$1; shift
-
-	vlan_destroy $dev 11
-	vlan_destroy $dev 10
-	mtu_restore $dev
-	simple_if_fini $dev
-}
-
 h1_create()
 {
 	host_create $h1 1
 }
 
-h1_destroy()
-{
-	host_destroy $h1
-}
-
 h2_create()
 {
 	host_create $h2 2
+
 	tc qdisc add dev $h2 clsact
+	defer tc qdisc del dev $h2 clsact
 
 	# Some of the tests in this suite use multicast traffic. As this traffic
 	# enters BR2_10 resp. BR2_11, it is flooded to all other ports. Thus
@@ -139,13 +131,7 @@ h2_create()
 
 	tc qdisc replace dev $h2 root handle 10: tbf rate 1gbit \
 		burst 128K limit 1G
-}
-
-h2_destroy()
-{
-	tc qdisc del dev $h2 root handle 10:
-	tc qdisc del dev $h2 clsact
-	host_destroy $h2
+	defer tc qdisc del dev $h2 root handle 10:
 }
 
 h3_create()
@@ -153,40 +139,54 @@ h3_create()
 	host_create $h3 3
 }
 
-h3_destroy()
-{
-	host_destroy $h3
-}
-
 switch_create()
 {
 	local intf
 	local vlan
 
 	ip link add dev br1_10 type bridge
+	defer ip link del dev br1_10
+
 	ip link add dev br1_11 type bridge
+	defer ip link del dev br1_11
 
 	ip link add dev br2_10 type bridge
+	defer ip link del dev br2_10
+
 	ip link add dev br2_11 type bridge
+	defer ip link del dev br2_11
 
 	for intf in $swp1 $swp2 $swp3 $swp4 $swp5; do
 		ip link set dev $intf up
+		defer ip link set dev $intf down
+
 		mtu_set $intf 10000
+		defer mtu_restore $intf
 	done
 
 	for intf in $swp1 $swp4; do
 		for vlan in 10 11; do
 			vlan_create $intf $vlan
+			defer vlan_destroy $intf $vlan
+
 			ip link set dev $intf.$vlan master br1_$vlan
+			defer ip link set dev $intf.$vlan nomaster
+
 			ip link set dev $intf.$vlan up
+			defer ip link set dev $intf.$vlan up
 		done
 	done
 
 	for intf in $swp2 $swp3 $swp5; do
 		for vlan in 10 11; do
 			vlan_create $intf $vlan
+			defer vlan_destroy $intf $vlan
+
 			ip link set dev $intf.$vlan master br2_$vlan
+			defer ip link set dev $intf.$vlan nomaster
+
 			ip link set dev $intf.$vlan up
+			defer ip link set dev $intf.$vlan up
 		done
 	done
 
@@ -201,49 +201,25 @@ switch_create()
 	for intf in $swp3 $swp4; do
 		tc qdisc replace dev $intf root handle 1: tbf rate 1gbit \
 			burst 128K limit 1G
+		defer tc qdisc del dev $intf root handle 1:
 	done
 
 	ip link set dev br1_10 up
+	defer ip link set dev br1_10 down
+
 	ip link set dev br1_11 up
+	defer ip link set dev br1_11 down
+
 	ip link set dev br2_10 up
+	defer ip link set dev br2_10 down
+
 	ip link set dev br2_11 up
+	defer ip link set dev br2_11 down
 
 	local size=$(devlink_pool_size_thtype 0 | cut -d' ' -f 1)
 	devlink_port_pool_th_save $swp3 8
 	devlink_port_pool_th_set $swp3 8 $size
-}
-
-switch_destroy()
-{
-	local intf
-	local vlan
-
-	devlink_port_pool_th_restore $swp3 8
-
-	ip link set dev br2_11 down
-	ip link set dev br2_10 down
-	ip link set dev br1_11 down
-	ip link set dev br1_10 down
-
-	for intf in $swp4 $swp3; do
-		tc qdisc del dev $intf root handle 1:
-	done
-
-	for intf in $swp5 $swp3 $swp2 $swp4 $swp1; do
-		for vlan in 11 10; do
-			ip link set dev $intf.$vlan down
-			ip link set dev $intf.$vlan nomaster
-			vlan_destroy $intf $vlan
-		done
-
-		mtu_restore $intf
-		ip link set dev $intf down
-	done
-
-	ip link del dev br2_11
-	ip link del dev br2_10
-	ip link del dev br1_11
-	ip link del dev br1_10
+	defer devlink_port_pool_th_restore $swp3 8
 }
 
 setup_prepare()
@@ -263,6 +239,7 @@ setup_prepare()
 	h3_mac=$(mac_get $h3)
 
 	vrf_prepare
+	defer vrf_cleanup
 
 	h1_create
 	h2_create
@@ -270,18 +247,6 @@ setup_prepare()
 	switch_create
 }
 
-cleanup()
-{
-	pre_cleanup
-
-	switch_destroy
-	h3_destroy
-	h2_destroy
-	h1_destroy
-
-	vrf_cleanup
-}
-
 ping_ipv4()
 {
 	ping_test $h1.10 $(ipaddr 3 10) " from host 1, vlan 10"
-- 
2.45.0

Re: [RFC PATCH net-next 2/5] selftests: mlxsw: sch_red_core: Use defer for test cleanup

[Ido Schimmel]

On Thu, Aug 22, 2024 at 03:49:41PM +0200, Petr Machata wrote:
> Instead of having a suite of dedicated cleanup functions, use the defer
> framework to schedule cleanups right as their setup functions are run.
> 
> This makes a dedicated cleanup() moot, instead fall back to the
> lib.sh-provided one, which invokes the necessary defer cleanups as well.
> 
> Signed-off-by: Petr Machata <petrm@nvidia.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

[RFC PATCH net-next 3/5] selftests: mlxsw: sch_red_core: Use defer for stopping traffic

[Petr Machata]

Tests interrupted part-way through leave behind a running mausezahn. Use
defer to schedule a traffic stop after traffic is started. Mark the
functions that run traffic as defer-scoped, such that the traffic is
stopped right after the function returns.

Signed-off-by: Petr Machata <petrm@nvidia.com>
---
 .../drivers/net/mlxsw/sch_red_core.sh         | 22 ++++++++++++-------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_red_core.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_red_core.sh
index 117a99fdbba5..c917b88d300e 100644
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_red_core.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_red_core.sh
@@ -415,6 +415,7 @@ __do_ecn_test()
 
 	start_tcp_traffic $h1.$vlan $(ipaddr 1 $vlan) $(ipaddr 3 $vlan) \
 			  $h3_mac tos=0x01
+	defer stop_traffic
 	sleep 1
 
 	ecn_test_common "$name" "$get_nmarked" $vlan $limit
@@ -427,9 +428,9 @@ __do_ecn_test()
 	check_fail $? "UDP traffic went into backlog instead of being early-dropped"
 	log_test "TC $((vlan - 10)): $name backlog > limit: UDP early-dropped"
 
-	stop_traffic
 	sleep 1
 }
+defer_scoped_fn __do_ecn_test
 
 do_ecn_test()
 {
@@ -456,6 +457,7 @@ do_ecn_nodrop_test()
 
 	start_tcp_traffic $h1.$vlan $(ipaddr 1 $vlan) $(ipaddr 3 $vlan) \
 			  $h3_mac tos=0x01
+	defer stop_traffic
 	sleep 1
 
 	ecn_test_common "$name" get_nmarked $vlan $limit
@@ -468,9 +470,9 @@ do_ecn_nodrop_test()
 	check_err $? "UDP traffic was early-dropped instead of getting into backlog"
 	log_test "TC $((vlan - 10)): $name backlog > limit: UDP not dropped"
 
-	stop_traffic
 	sleep 1
 }
+defer_scoped_fn do_ecn_nodrop_test
 
 do_red_test()
 {
@@ -483,6 +485,7 @@ do_red_test()
 	# is above limit.
 	start_tcp_traffic $h1.$vlan $(ipaddr 1 $vlan) $(ipaddr 3 $vlan) \
 			  $h3_mac tos=0x01
+	defer stop_traffic
 
 	# Pushing below the queue limit should work.
 	RET=0
@@ -506,9 +509,9 @@ do_red_test()
 	check_err $? "backlog $backlog / $limit expected <= 10% distance"
 	log_test "TC $((vlan - 10)): RED backlog > limit"
 
-	stop_traffic
 	sleep 1
 }
+defer_scoped_fn do_red_test
 
 do_mc_backlog_test()
 {
@@ -520,7 +523,10 @@ do_mc_backlog_test()
 	RET=0
 
 	start_tcp_traffic $h1.$vlan $(ipaddr 1 $vlan) $(ipaddr 3 $vlan) bc
+	defer stop_traffic
+
 	start_tcp_traffic $h2.$vlan $(ipaddr 2 $vlan) $(ipaddr 3 $vlan) bc
+	defer stop_traffic
 
 	qbl=$(busywait 5000 until_counter_is ">= 500000" \
 		       get_qdisc_backlog $vlan)
@@ -533,11 +539,9 @@ do_mc_backlog_test()
 		       get_mc_transmit_queue $vlan)
 	check_err $? "MC backlog reported by qdisc not visible in ethtool"
 
-	stop_traffic
-	stop_traffic
-
 	log_test "TC $((vlan - 10)): Qdisc reports MC backlog"
 }
+defer_scoped_fn do_mc_backlog_test
 
 do_mark_test()
 {
@@ -554,6 +558,7 @@ do_mark_test()
 
 	start_tcp_traffic $h1.$vlan $(ipaddr 1 $vlan) $(ipaddr 3 $vlan) \
 			  $h3_mac tos=0x01
+	defer stop_traffic
 
 	# Create a bit of a backlog and observe no mirroring due to marks.
 	qevent_rule_install_$subtest
@@ -584,9 +589,9 @@ do_mark_test()
 		log_test "TC $((vlan - 10)): marked packets $subtest'd"
 	fi
 
-	stop_traffic
 	sleep 1
 }
+defer_scoped_fn do_mark_test
 
 do_drop_test()
 {
@@ -603,6 +608,7 @@ do_drop_test()
 	RET=0
 
 	start_traffic $h1.$vlan $(ipaddr 1 $vlan) $(ipaddr 3 $vlan) $h3_mac
+	defer stop_traffic
 
 	# Create a bit of a backlog and observe no mirroring due to drops.
 	qevent_rule_install_$subtest
@@ -636,9 +642,9 @@ do_drop_test()
 
 	log_test "TC $((vlan - 10)): ${trigger}ped packets $subtest'd"
 
-	stop_traffic
 	sleep 1
 }
+defer_scoped_fn do_drop_test
 
 qevent_rule_install_mirror()
 {
-- 
2.45.0

Re: [RFC PATCH net-next 3/5] selftests: mlxsw: sch_red_core: Use defer for stopping traffic

[Ido Schimmel]

On Thu, Aug 22, 2024 at 03:49:42PM +0200, Petr Machata wrote:
> Tests interrupted part-way through leave behind a running mausezahn. Use
> defer to schedule a traffic stop after traffic is started. Mark the
> functions that run traffic as defer-scoped, such that the traffic is
> stopped right after the function returns.
> 
> Signed-off-by: Petr Machata <petrm@nvidia.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

[RFC PATCH net-next 4/5] selftests: mlxsw: sch_red_*: Use defer for qdisc management

[Petr Machata]

Use defer scopes to manage qdisc lifetime.

Signed-off-by: Petr Machata <petrm@nvidia.com>
---
 .../drivers/net/mlxsw/sch_red_ets.sh          | 32 +++++++++----------
 .../drivers/net/mlxsw/sch_red_root.sh         | 24 ++++++++++----
 2 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh
index 576067b207a8..87b4dc509896 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_red_ets.sh
@@ -80,36 +80,37 @@ uninstall_qdisc()
 ecn_test()
 {
 	install_qdisc ecn
+	defer uninstall_qdisc
 
 	do_ecn_test 10 $BACKLOG1
 	do_ecn_test 11 $BACKLOG2
-
-	uninstall_qdisc
 }
+defer_scoped_fn ecn_test
 
 ecn_test_perband()
 {
 	install_qdisc ecn
+	defer uninstall_qdisc
 
 	do_ecn_test_perband 10 $BACKLOG1
 	do_ecn_test_perband 11 $BACKLOG2
-
-	uninstall_qdisc
 }
+defer_scoped_fn ecn_test_perband
 
 ecn_nodrop_test()
 {
 	install_qdisc ecn nodrop
+	defer uninstall_qdisc
 
 	do_ecn_nodrop_test 10 $BACKLOG1
 	do_ecn_nodrop_test 11 $BACKLOG2
-
-	uninstall_qdisc
 }
+defer_scoped_fn ecn_nodrop_test
 
 red_test()
 {
 	install_qdisc
+	defer uninstall_qdisc
 
 	# Make sure that we get the non-zero value if there is any.
 	local cur=$(busywait 1100 until_counter_is "> 0" \
@@ -120,51 +121,50 @@ red_test()
 
 	do_red_test 10 $BACKLOG1
 	do_red_test 11 $BACKLOG2
-
-	uninstall_qdisc
 }
+defer_scoped_fn red_test
 
 mc_backlog_test()
 {
 	install_qdisc
+	defer uninstall_qdisc
 
 	# Note that the backlog numbers here do not correspond to RED
 	# configuration, but are arbitrary.
 	do_mc_backlog_test 10 $BACKLOG1
 	do_mc_backlog_test 11 $BACKLOG2
-
-	uninstall_qdisc
 }
+defer_scoped_fn mc_backlog_test
 
 red_mirror_test()
 {
 	install_qdisc qevent early_drop block 10
+	defer uninstall_qdisc
 
 	do_drop_mirror_test 10 $BACKLOG1 early_drop
 	do_drop_mirror_test 11 $BACKLOG2 early_drop
-
-	uninstall_qdisc
 }
+defer_scoped_fn red_mirror_test
 
 red_trap_test()
 {
 	install_qdisc qevent early_drop block 10
+	defer uninstall_qdisc
 
 	do_drop_trap_test 10 $BACKLOG1 early_drop
 	do_drop_trap_test 11 $BACKLOG2 early_drop
-
-	uninstall_qdisc
 }
+defer_scoped_fn red_trap_test
 
 ecn_mirror_test()
 {
 	install_qdisc ecn qevent mark block 10
+	defer uninstall_qdisc
 
 	do_mark_mirror_test 10 $BACKLOG1
 	do_mark_mirror_test 11 $BACKLOG2
-
-	uninstall_qdisc
 }
+defer_scoped_fn ecn_mirror_test
 
 bail_on_lldpad "configure DCB" "configure Qdiscs"
 
diff --git a/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh b/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh
index 159108d02895..1777b79b1190 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/sch_red_root.sh
@@ -32,46 +32,58 @@ uninstall_qdisc()
 ecn_test()
 {
 	install_qdisc ecn
+	defer uninstall_qdisc
+
 	do_ecn_test 10 $BACKLOG
-	uninstall_qdisc
 }
+defer_scoped_fn ecn_test
 
 ecn_test_perband()
 {
 	install_qdisc ecn
+	defer uninstall_qdisc
+
 	do_ecn_test_perband 10 $BACKLOG
-	uninstall_qdisc
 }
+defer_scoped_fn ecn_test_perband
 
 ecn_nodrop_test()
 {
 	install_qdisc ecn nodrop
+	defer uninstall_qdisc
+
 	do_ecn_nodrop_test 10 $BACKLOG
-	uninstall_qdisc
 }
+defer_scoped_fn ecn_nodrop_test
 
 red_test()
 {
 	install_qdisc
+	defer uninstall_qdisc
+
 	do_red_test 10 $BACKLOG
-	uninstall_qdisc
 }
+defer_scoped_fn red_test
 
 mc_backlog_test()
 {
 	install_qdisc
+	defer uninstall_qdisc
+
 	# Note that the backlog value here does not correspond to RED
 	# configuration, but is arbitrary.
 	do_mc_backlog_test 10 $BACKLOG
-	uninstall_qdisc
 }
+defer_scoped_fn mc_backlog_test
 
 red_mirror_test()
 {
 	install_qdisc qevent early_drop block 10
+	defer uninstall_qdisc
+
 	do_drop_mirror_test 10 $BACKLOG
-	uninstall_qdisc
 }
+defer_scoped_fn red_mirror_test
 
 bail_on_lldpad "configure DCB" "configure Qdiscs"
 
-- 
2.45.0

Re: [RFC PATCH net-next 4/5] selftests: mlxsw: sch_red_*: Use defer for qdisc management

[Ido Schimmel]

On Thu, Aug 22, 2024 at 03:49:43PM +0200, Petr Machata wrote:
> Use defer scopes to manage qdisc lifetime.
> 
> Signed-off-by: Petr Machata <petrm@nvidia.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

[RFC PATCH net-next 5/5] selftests: sch_tbf_core: Use defer for stopping traffic

[Petr Machata]

Tests interrupted part-way through leave behind a running mausezahn. Use
defer to schedule a traffic stop after traffic is started.

Signed-off-by: Petr Machata <petrm@nvidia.com>
---
 tools/testing/selftests/net/forwarding/sch_tbf_core.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/forwarding/sch_tbf_core.sh b/tools/testing/selftests/net/forwarding/sch_tbf_core.sh
index 9cd884d4a5de..5d58c04e055c 100644
--- a/tools/testing/selftests/net/forwarding/sch_tbf_core.sh
+++ b/tools/testing/selftests/net/forwarding/sch_tbf_core.sh
@@ -213,12 +213,12 @@ do_tbf_test()
 	local mbit=$1; shift
 
 	start_traffic $h1.$vlan $(ipaddr 1 $vlan) $(ipaddr 2 $vlan) $h2_mac
+	defer stop_traffic
 	sleep 5 # Wait for the burst to dwindle
 
 	local t2=$(busywait_for_counter 1000 +1 tbf_get_counter $vlan)
 	sleep 10
 	local t3=$(tbf_get_counter $vlan)
-	stop_traffic
 
 	RET=0
 
@@ -231,3 +231,4 @@ do_tbf_test()
 
 	log_test "TC $((vlan - 10)): TBF rate ${mbit}Mbit"
 }
+defer_scoped_fn do_tbf_test
-- 
2.45.0

Re: [RFC PATCH net-next 5/5] selftests: sch_tbf_core: Use defer for stopping traffic

[Ido Schimmel]

On Thu, Aug 22, 2024 at 03:49:44PM +0200, Petr Machata wrote:
> Tests interrupted part-way through leave behind a running mausezahn. Use
> defer to schedule a traffic stop after traffic is started.

Any reason not to convert the setup part to the defer mechanism as well?

> 
> Signed-off-by: Petr Machata <petrm@nvidia.com>
> ---
>  tools/testing/selftests/net/forwarding/sch_tbf_core.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/forwarding/sch_tbf_core.sh b/tools/testing/selftests/net/forwarding/sch_tbf_core.sh
> index 9cd884d4a5de..5d58c04e055c 100644
> --- a/tools/testing/selftests/net/forwarding/sch_tbf_core.sh
> +++ b/tools/testing/selftests/net/forwarding/sch_tbf_core.sh
> @@ -213,12 +213,12 @@ do_tbf_test()
>  	local mbit=$1; shift
>  
>  	start_traffic $h1.$vlan $(ipaddr 1 $vlan) $(ipaddr 2 $vlan) $h2_mac
> +	defer stop_traffic
>  	sleep 5 # Wait for the burst to dwindle
>  
>  	local t2=$(busywait_for_counter 1000 +1 tbf_get_counter $vlan)
>  	sleep 10
>  	local t3=$(tbf_get_counter $vlan)
> -	stop_traffic
>  
>  	RET=0
>  
> @@ -231,3 +231,4 @@ do_tbf_test()
>  
>  	log_test "TC $((vlan - 10)): TBF rate ${mbit}Mbit"
>  }
> +defer_scoped_fn do_tbf_test
> -- 
> 2.45.0
> 

Re: [RFC PATCH net-next 5/5] selftests: sch_tbf_core: Use defer for stopping traffic

[Petr Machata]

Ido Schimmel <idosch@nvidia.com> writes:

> On Thu, Aug 22, 2024 at 03:49:44PM +0200, Petr Machata wrote:
>> Tests interrupted part-way through leave behind a running mausezahn. Use
>> defer to schedule a traffic stop after traffic is started.
>
> Any reason not to convert the setup part to the defer mechanism as well?

No. I wanted to see if it makes sense to have an entire test use the
defer mechanism, in addition to cleanups specific to individual tests.
But I wasn't sure what people would think, so just converted the RED
tests as a sort of a demo.