Re: [PATCH net-next v2 4/4] selftests: drv-net: rss_ctx: add tests for RSS configuration and contexts

[Petr Machata <petrm@nvidia.com>]

Jakub Kicinski <kuba@kernel.org> writes:

> On Tue, 25 Jun 2024 12:42:22 +0200 Petr Machata wrote:
>> > +def test_rss_key_indir(cfg):
>> > +    """
>> > +    Test basics like updating the main RSS key and indirection table.
>> > +    """
>> > +    if len(_get_rx_cnts(cfg)) < 2:
>> > +        KsftSkipEx("Device has only one queue (or doesn't support queue stats)")  
>> 
>> I'm not sure, is this admin-correctible configuration issue? It looks
>> like this and some others should rather be XFAIL.
>
> TBH I don't have a good compass on what should be XFAIL and what should
> be SKIP in HW tests. Once vendors start running these we'll get more
> experience (there's only one test using Xfail in HW now).

Sure, me neither.

>> > +    # Try to allocate more queues when necessary
>> > +    qcnt = len(_get_rx_cnts(cfg))
>> > +    if qcnt >= 2 + 2 * ctx_cnt:
>> > +        qcnt = None
>> > +    else:
>> > +        try:
>> > +            ksft_pr(f"Increasing queue count {qcnt} -> {2 + 2 * ctx_cnt}")
>> > +            ethtool(f"-L {cfg.ifname} combined {2 + 2 * ctx_cnt}")
>> > +        except:
>> > +            raise KsftSkipEx("Not enough queues for the test")  
>> 
>> There are variations on this in each of the three tests. It would make
>> sense to extract to a helper, or perhaps even write as a context
>> manager. Untested:
>> 
>> class require_contexts:
>>     def __init__(self, cfg, count):
>>         self._cfg = cfg
>>         self._count = count
>>         self._qcnt = None
>> 
>>     def __enter__(self):
>>         qcnt = len(_get_rx_cnts(self._cfg))
>>         if qcnt >= self._count:
>>             return
>>         try:
>>             ksft_pr(f"Increasing queue count {qcnt} -> {self._count}")
>>             ethtool(f"-L {self._cfg.ifname} combined {self._count}")
>>             self._qcnt = qcnt
>>         except:
>>             raise KsftSkipEx("Not enough queues for the test")
>> 
>>     def __exit__(self, exc_type, exc_value, traceback):
>>         if self._qcnt is not None:
>>             ethtool(f"-L {self._cfg.ifname} combined {self._qcnt}")
>> 
>> Then:
>> 
>>     with require_contexts(cfg, 2 + 2 * ctx_cnt):
>>         ...
>
> 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.

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)

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

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.