Message-ID: <87y16s9ibl.fsf@nvidia.com>
Date: Wed, 26 Jun 2024 10:42:48 +0200
From: Petr Machata <petrm@...dia.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: Petr Machata <petrm@...dia.com>, <davem@...emloft.net>,
	<netdev@...r.kernel.org>, <edumazet@...gle.com>, <pabeni@...hat.com>,
	<willemdebruijn.kernel@...il.com>, <ecree.xilinx@...il.com>,
	<dw@...idwei.uk>, <przemyslaw.kitszel@...el.com>,
	<michael.chan@...adcom.com>, <andrew.gospodarek@...adcom.com>
Subject: Re: [PATCH net-next v2 4/4] selftests: drv-net: rss_ctx: add tests
 for RSS configuration and contexts


Jakub Kicinski <kuba@...nel.org> writes:

> On Tue, 25 Jun 2024 16:43:55 +0200 Petr Machata wrote:
>> > There are 4 things to clean up, with doesn't cover all of them
>> > naturally and complicates the code.  
>> 
>> Yeah, you can't use it everywhere, but you can use it for the ethtool
>> config here.
>> 
>> Re complexity, how about this?
>> 
>> import contextlib
>> 
>> @contextlib.contextmanager
>> def require_contexts(cfg, count):
>>     qcnt = len(_get_rx_cnts(cfg))
>>     if qcnt >= count:
>>         qcnt = None
>>     else:
>>         try:
>>             ksft_pr(f"Increasing queue count {qcnt} -> {count}")
>>             ethtool(f"-L {cfg.ifname} combined {count}")
>>         except:
>>             raise KsftSkipEx("Not enough queues for the test")
>> 
>>     try:
>>         yield
>>     finally:
>>         if qcnt is not None:
>>             ethtool(f"-L {cfg.ifname} combined {qcnt}")
>> 
>> This is mostly just business logic, hardly any boilerplate, and still
>> just uses standard Python. You get the setup and cleanup next to each
>> other, which is important for cross-comparing the two.
>
> TBH I don't really understand of how the above works.

The decorator transforms the function into a context manager. yield
marks the point where the with: body runs, whetever is before is
initialization, whatever is after is cleanup. The try: finally: wrapper
is there in case the with body ends with an exception.

>> Anyway, if I don't persuade you for The Right Path, something like this
>> would at least get rid of the duplication:
>> 
>>     qcnt = contexts_setup(cfg, 2 + 2 * ctx_cnt)
>>     try:
>>         ...
>>     finally:
>>         if qcnt:
>>             contexts_teardown(cfg, qcnt)
>
> Are we discussing this exact test script or general guidance?
>
> If the general guidance, my principle is to make the test look like
> a list of bash commands as much as possible. Having to wrap
> every single command you need to undo with a context manager
> will take us pretty far from a linear script.
>
> That's why I'd prefer if we provided a mechanism which makes
> it easy to defer execution, rather than focus on particular cases.

I meant it as a general principle. Python has tools to solve the cleanup
problem very elegantly. I asked around in the meantime, people don't
generally find it hard to understand. It's a simple concept. It might be
_foreign_, I'll grant you that, but so will whatever new API you cook
up. And you can google the former easily enough.

OK, look, I believe I made my point. I don't want to split this
particular hair too much. I see you already sent something for defer,
so I'll take a look at that.

>> > Once again, I'm thinking about adding some form of deferred execution.
>> > 	
>> > 	ethtool(f"-L {self._cfg.ifname} combined {self._qcnt}")
>> > 	undo(ethtool, f"-L {self._cfg.ifname} combined {old_qcnt}")
>> >
>> > Where cleanups will be executed in reverse order by ksft_run() after
>> > the test, with the option to delete them.
>> >
>> > 	nid = ethtool_create(cfg, "-N", flow)
>> > 	ntuple = undo(ethtool, f"-N {cfg.ifname} delete {nid}")
>> > 	# .. code using ntuple ...
>> > 	ntuple.exec()
>> > 	# .. now ntuple is gone
>> >
>> > or/and:
>> >
>> > 	nid = ethtool_create(cfg, "-N", flow)
>> > 	with undo(ethtool, f"-N {cfg.ifname} delete {nid}"):
>> > 		# .. code using ntuple ...
>> > 	# .. now ntuple is gone
>> >
>> > Thoughts?  
>> 
>> Sure, this can be done, but you are introducing a new mechanism to solve
>> something that the language has had support for for 15 years or so.
>
> Well, I can't make the try: yield work for me :(
>
> #!/bin/python3
>
> import contextlib
>
> @contextlib.contextmanager
> def bla():
>     try:
>         yield
>     except:
>         print("deferred thing")
>
> bla()
> print("done")
>
>
> Gives me:
> $ ./test.py 
> done
>
> I don't know enough Python, IDK if we can assume much Python expertise
> from others.
>
> What we basically want is a form of atexit:
>
> https://docs.python.org/3/library/atexit.html
>
> The fact atexit module exists makes me wonder whether the problem is
> really solved by the language itself. But maybe there's a deeper reason
> for atexit.

I think it's just incremental. atexit was introduced in Python 2.0, with
statements in Python 2.6, some eight years later.

>> Like, it's not terrible. I like it better than the try/finally aprroach,
>> because at least the setup and cleanup are localized.
>> 
>> Call it defer though? It doesn't "undo" there and then, but at some
>> later point.
>
> I like "defer". We're enqueuing for deferred exec. So another option
> would be "enqueue". But I think "defer" is indeed better.
>
> rm = defer(cmd, "rm example.txt")
> rm.exec()   # run now, remove from internal queue
> rm.cancel() # remove from queue, don't run

Looks good.

Powered by blists - more mailing lists