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

[Jakub Kicinski <kuba@kernel.org>]

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.