Re: [PATCH net-next v2 2/9] libeth: add common queue stats

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 19 Aug 2024 15:34:34 -0700 Tony Nguyen wrote:
> + * Return: new &net_device on success, %NULL on error.
> + */
> +struct net_device *__libeth_netdev_alloc(u32 priv, u32 rqs, u32 sqs,
> +					 u32 xdpsqs)

The netdev alloc / free / set num queues can be a separate patch

> +MODULE_DESCRIPTION("Common Ethernet library");

BTW for Intel? Or you want this to be part of the core?
I thought Intel, but you should tell us if you have broader plans.

> +	const struct libeth_netdev_priv *priv = netdev_priv(dev);	      \
> +									      \
> +	memset(stats, 0, sizeof(*stats));				      \
> +	u64_stats_init(&stats->syncp);					      \
> +									      \
> +	mutex_init(&priv->base_##pfx##s[qid].lock);			      \

the mutex is only for writes or for reads of base too?
mutex is a bad idea for rtnl stats

> +/* Netlink stats. Exported fields have the same names as in the NL structs */
> +
> +struct libeth_stats_export {
> +	u16	li;
> +	u16	gi;
> +};
> +
> +#define LIBETH_STATS_EXPORT(lpfx, gpfx, field) {			      \
> +	.li = (offsetof(struct libeth_##lpfx##_stats, field) -		      \
> +	       offsetof(struct libeth_##lpfx##_stats, raw)) /		      \
> +	      sizeof_field(struct libeth_##lpfx##_stats, field),	      \
> +	.gi = offsetof(struct netdev_queue_stats_##gpfx, field) /	      \
> +	      sizeof_field(struct netdev_queue_stats_##gpfx, field)	      \
> +}

humpf

> +#define LIBETH_STATS_DEFINE_EXPORT(pfx, gpfx)				      \
> +static void								      \
> +libeth_get_queue_stats_##gpfx(struct net_device *dev, int idx,		      \
> +			      struct netdev_queue_stats_##gpfx *stats)	      \
> +{									      \
> +	const struct libeth_netdev_priv *priv = netdev_priv(dev);	      \
> +	const struct libeth_##pfx##_stats *qs;				      \
> +	u64 *raw = (u64 *)stats;					      \
> +	u32 start;							      \
> +									      \
> +	qs = READ_ONCE(priv->live_##pfx##s[idx]);			      \
> +	if (!qs)							      \
> +		return;							      \
> +									      \
> +	do {								      \
> +		start = u64_stats_fetch_begin(&qs->syncp);		      \
> +									      \
> +		libeth_stats_foreach_export(pfx, exp)			      \
> +			raw[exp->gi] = u64_stats_read(&qs->raw[exp->li]);     \
> +	} while (u64_stats_fetch_retry(&qs->syncp, start));		      \
> +}									      \

ugh. Please no

> +									      \
> +static void								      \
> +libeth_get_##pfx##_base_stats(const struct net_device *dev,		      \
> +			      struct netdev_queue_stats_##gpfx *stats)	      \
> +{									      \
> +	const struct libeth_netdev_priv *priv = netdev_priv(dev);	      \
> +	u64 *raw = (u64 *)stats;					      \
> +									      \
> +	memset(stats, 0, sizeof(*(stats)));				      \

Have you read the docs for any of the recent stats APIs?

Nack. Just implement the APIs in the driver, this does not seem like 
a sane starting point _at all_. You're going to waste more time coming
up with such abstraction than you'd save implementing it for 10 drivers.