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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 25 Jun 2024 16:43:55 +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 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.