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

[Alexander Lobakin <aleksander.lobakin@intel.com>]

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

> From: Maciej Fijalkowski <maciej.fijalkowski@intel.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