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

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: <20240820181757.02d83f15@kernel.org>
Date: Tue, 20 Aug 2024 18:17:57 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Tony Nguyen <anthony.l.nguyen@...el.com>
Cc: davem@...emloft.net, pabeni@...hat.com, edumazet@...gle.com,
 netdev@...r.kernel.org, Alexander Lobakin <aleksander.lobakin@...el.com>,
 przemyslaw.kitszel@...el.com, joshua.a.hay@...el.com,
 michal.kubiak@...el.com, nex.sw.ncis.osdt.itp.upstreaming@...el.com
Subject: Re: [PATCH net-next v2 2/9] libeth: add common queue stats

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.