Re: [RFC net-next 1/2] selftests: drv-net: add ability to schedule cleanup with defer()

[Petr Machata <petrm@nvidia.com>]

Jakub Kicinski <kuba@kernel.org> writes:

> On Wed, 26 Jun 2024 12:18:58 +0200 Petr Machata wrote:
>> Jakub Kicinski <kuba@kernel.org> writes:
>> > +def ksft_flush_defer():
>> > +    global KSFT_RESULT
>> > +
>> > +    while global_defer_queue:
>> > +        entry = global_defer_queue[-1]
>> > +        try:
>> > +            entry.exec()  
>> 
>> I wonder if you added _exec() to invoke it here. Because then you could
>> just do entry = global_defer_queue.pop() and entry._exec(), and in the
>> except branch you would just have the test-related business, without the
>> queue management.
>
> Initially I had both _exec, and _dequeue as separate helpers, but then
> _dequeue was identical to cancel, so I removed that one, but _exec
> stayed.
>
> As you point out _exec() would do nicely during "flush".. but linter was
> angry at me for calling private functions. I couldn't quickly think of
> a clean scheme of naming things. Or rather, I should say, I like that
> the only non-private functions in class defer right now are
> test-author-facing. At some point I considered renaming _exec() to
> __call__() or run() but I was worried people will incorrectly
> call it, instead of calling exec().
>
> So I decided to stick to a bit of awkward handling in the internals for
> the benefit of more obvious test-facing API. But no strong preference,
> LMK if calling _exec() here is fine or I should rename it..

Maybe call it something like exec_only()? There's a list that you just
need to go through, it looks a shame not to just .pop() everything out
one by one and instead have this management business in the error path.

>> > +        except Exception:  
>> 
>> I think this should be either an unqualified except: or except
>> BaseException:.
>
> SG
>
>
>> >      print(
>> >          f"# Totals: pass:{totals['pass']} fail:{totals['fail']} xfail:{totals['xfail']} xpass:0 skip:{totals['skip']} error:0"  
>> 
>> Majority of this hunk is just preparatory and should be in a patch of
>> its own. Then in this patch it should just introduce the flush.
>
> True, will split.
>
>> > +    def cancel(self):  
>> 
>> This shouldn't dequeue if not self.queued.
>
> I was wondering if we're better off throwing the exception from
> remove() or silently ignoring (what is probably an error in the 
> test code). I went with the former intentionally, but happy to
> change.

Hmm, right, it would throw. Therefore second exec() would as well. Good.
But that means that exec() should first cancel, then exec, otherwise
second exec invocation would actually exec the cleanup a second time
before bailing out.

>
>> > +        self._queue.remove(self)
>> > +        self.queued = False
>> > +
>> > +    def exec(self):  
>> 
>> This shouldn't exec if self.executed.
>> 
>> But I actually wonder if we need two flags at all. Whether the defer
>> entry is resolved through exec(), cancel() or __exit__(), it's "done".
>> It could be left in the queue, in which case the "done" flag is going to
>> disable future exec requests. Or it can just be dropped from the queue
>> when done, in which case we don't even need the "done" flag as such.
>
> If you recall there's a rss_ctx test case which removes contexts out of
> order. The flags are basically for that test. We run the .exec() to
> remove a context, and then we can check 
>
> 	if thing.queued:
> 		.. code for context that's alive ..
> 	else:
> 		.. code for dead context ..

That test already has its own flags to track which was removed, can't it
use those? My preference is always to keep an API as minimal as possible
and the flags, if any, would ideally be private. I don't think defer
objects should keep track of whether the user has already invoked them
or not, that's their user's business to know.