netdev - Re: [PATCH net-next 03/16] libeth: add a couple of XDP helpers (libeth

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: <69d55e46-74a1-4e74-afcd-f724810b40c3@intel.com>
Date: Tue, 8 Apr 2025 15:51:48 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, Michal Kubiak
	<michal.kubiak@...el.com>, Tony Nguyen <anthony.l.nguyen@...el.com>, "Przemek
 Kitszel" <przemyslaw.kitszel@...el.com>, Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, "Alexei
 Starovoitov" <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
	"Jesper Dangaard Brouer" <hawk@...nel.org>, John Fastabend
	<john.fastabend@...il.com>, Simon Horman <horms@...nel.org>,
	<bpf@...r.kernel.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 03/16] libeth: add a couple of XDP helpers
 (libeth_xdp)

From: Alexander Lobakin <aleksander.lobakin@...el.com>
Date: Tue, 8 Apr 2025 15:22:48 +0200

> From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
> Date: Tue, 11 Mar 2025 15:05:38 +0100
> 
>> On Wed, Mar 05, 2025 at 05:21:19PM +0100, Alexander Lobakin wrote:
>>> "Couple" is a bit humbly... Add the following functionality to libeth:

[...]

>>> +/**
>>> + * libeth_xdp_xmit_do_bulk - implement full .ndo_xdp_xmit() in driver
>>> + * @dev: target &net_device
>>> + * @n: number of frames to send
>>> + * @fr: XDP frames to send
>>> + * @f: flags passed by the stack
>>> + * @xqs: array of XDPSQs driver structs
>>> + * @nqs: number of active XDPSQs, the above array length
>>> + * @fl: driver callback to flush an XDP xmit bulk
>>> + * @fin: driver cabback to finalize the queue
>>> + *
>>> + * If the driver has active XDPSQs, perform common checks and send the frames.
>>> + * Finalize the queue, if requested.
>>> + *
>>> + * Return: number of frames sent or -errno on error.
>>> + */
>>> +#define libeth_xdp_xmit_do_bulk(dev, n, fr, f, xqs, nqs, fl, fin)	      \
>>> +	_libeth_xdp_xmit_do_bulk(dev, n, fr, f, xqs, nqs, fl, fin,	      \
>>> +				 __UNIQUE_ID(bq_), __UNIQUE_ID(ret_),	      \
>>> +				 __UNIQUE_ID(nqs_))
>>
>> why __UNIQUE_ID() is needed?
> 
> As above, variable shadowing.
> 
>>
>>> +
>>> +#define _libeth_xdp_xmit_do_bulk(d, n, fr, f, xqs, nqs, fl, fin, ub, ur, un)  \
>>
>> why single underscore? usually we do __ for internal funcs as you did
>> somewhere above.
> 
> Double-underscored is defined above already :D
> So it would be either like this or __ + ___
> 
>>
>> also, why define and not inlined func?
> 
> I'll double check, but if you look at its usage in idpf/xdp.c, you'll
> see that some arguments are non-trivial to obtain, IOW they cost some
> cycles. Macro ensures they won't be fetched prior to
> `likely(number_of_xdpsqs)`.
> I'll convert to an inline and check if the compiler handles this itself.
> It didn't behave in {,__}libeth_xdp_tx_fill_stats() unfortunately, hence
> macro there as well =\

UPD: it can't be an inline func since it's meant to be called like that
from the driver:

	return libeth_xdp_xmit_do_bulk(dev, n, frames, flags,
				       &vport->txqs[vport->xdp_txq_offset],
				       vport->num_xdp_txq,
				       idpf_xdp_xmit_flush_bulk,
				       idpf_xdp_tx_finalize);

The type of `&vport->txqs[vport->xdp_txq_offset]` is undefined from
libeth's perspective. libeth_xdp_xmit_init_bulk() embedded into it picks
the appropriate queue right away in the driver and it's a macro itself.

> 
>>
>>> +({									      \
>>> +	u32 un = (nqs);							      \
>>> +	int ur;								      \
>>> +									      \
>>> +	if (likely(un)) {						      \
>>> +		struct libeth_xdp_tx_bulk ub;				      \
>>> +									      \
>>> +		libeth_xdp_xmit_init_bulk(&ub, d, xqs, un);		      \
>>> +		ur = __libeth_xdp_xmit_do_bulk(&ub, fr, n, f, fl, fin);	      \
>>> +	} else {							      \
>>> +		ur = -ENXIO;						      \
>>> +	}								      \
>>> +									      \
>>> +	ur;								      \
>>> +})

Thanks,
Olek