netdev - Re: [RFC net-next 22/22] selftests: drv-net: add test for rx-buf-len

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]

Message-ID: <20250425165243.27421a0b@kernel.org>
Date: Fri, 25 Apr 2025 16:52:43 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Stanislav Fomichev <stfomichev@...il.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
 pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
 donald.hunter@...il.com, sdf@...ichev.me, almasrymina@...gle.com,
 dw@...idwei.uk, asml.silence@...il.com, ap420073@...il.com,
 jdamato@...tly.com, dtatulea@...dia.com, michael.chan@...adcom.com
Subject: Re: [RFC net-next 22/22] selftests: drv-net: add test for
 rx-buf-len

On Tue, 22 Apr 2025 10:06:48 -0700 Stanislav Fomichev wrote:
> > +def _do_bpftrace(cfg, mul, base_size, tgt_queue=None):
> > +    queue_filter = ''
> > +    if tgt_queue is not None:
> > +        queue_filter = 'if ($skb->queue_mapping != %d) {return;}' % (tgt_queue + 1, )  
> 
> if tgt_queue: should work as well?

Ha! I'm glad I'm not the only one who would fall into this trap.
tgt_queue = 0 is legit and actually used by the test.
So we'd collect the data for the whole system if test asked
for filtering just to queue 0. This breaks the test when queue 1
has large buffers, queue 0 small and we want to prove queue 0
is not getting large ones..

> > +
> > +    t = ('tracepoint:net:netif_receive_skb { ' +
> > +         '$skb = (struct sk_buff *)args->skbaddr; '+
> > +         '$sh = (struct skb_shared_info *)($skb->head + $skb->end); ' +
> > +         'if ($skb->dev->ifindex != ' + str(cfg.ifindex) + ') {return;} ' +
> > +         queue_filter +
> > +         '@[$skb->len - $skb->data_len] = count(); ' +
> > +         '@h[$skb->len - $skb->data_len] = count(); ' +
> > +         'if ($sh->nr_frags > 0) { @[$sh->frags[0].len] = count(); @d[$sh->frags[0].len] = count();} }'
> > +        )  
> 
> Why do we have @h and @d? We seem to check only the 'sizes'/@?

:o sorry leftover debug, thought i deleted it

> > +    maps = bpftrace(t, json=True, timeout=2)
> > +    # We expect one-dim array with something like:
> > +    # {"type": "map", "data": {"@": {"1500": 1, "719": 1,
> > +    sizes = maps["@"]
> > +    h = maps["@h"]
> > +    d = maps["@d"]
> > +    good = 0
> > +    bad = 0  

> > +def nic_wide(cfg, check_geometry=False) -> None:
> > +    """
> > +    Apply NIC wide rx-buf-len change. Run some traffic to make sure there
> > +    are no crashes. Test that setting 0 restores driver default.
> > +    Assume we start with the default.
> > +    """
> > +    try:
> > +        rings = cfg.ethnl.rings_get({'header': {'dev-index': cfg.ifindex}})
> > +    except NlError as e:
> > +        rings = {}
> > +    if "rx-buf-len" not in rings:
> > +        raise KsftSkipEx('rx-buf-len configuration not supported by device')
> > +
> > +    if rings['rx-buf-len'] * 2 <= rings['rx-buf-len-max']:
> > +        mul = 2
> > +    else:
> > +        mul = 1/2  
> 
> And similarly here? (and elsewhere)
> 
> def pick_buf_size(rings):
> 	""" pick new rx-buf-len depending on current and max settings """
> 	buf_len = rings['rx-buf-len']
> 	if buf_len * 2 <= <= rings['rx-buf-len-max']:
>  	  # if can grow, try to grow
> 	  return buf_len, buf_len * 2
> 	else:
> 	  # otherwise shrink
> 	  return buf_len, buf_len / 2
> 
> old_buf_len, new_buf_len = pick_buf_size(ring)
> ...
> 
> (or maybe its just me, idk, easier to think in old>new comparisons vs
> doing mul*base_size math)

I'd still keep old_buf as base_buf, because for per-queue 
it's not really old as it's still active on remaining queues.
Otherwise SGTM.