Re: [PATCH 2/2] netcons: Add udp send fail statistics to netconsole

[Jakub Kicinski]

On Sat, 24 Aug 2024 14:50:24 -0700 Maksym Kutsevol wrote:
> Enhance observability of netconsole. UDP sends can fail. Start tracking at

nit: "UDP sends" sounds a bit too much like it's using sockets
maybe "packet sends"?

> least two failure possibilities: ENOMEM and NET_XMIT_DROP for every target.
> Stats are exposed via an additional attribute in CONFIGFS.

Please provide a reference to another configfs user in the kernel which
exposes stats. To help reviewers validate it's a legit use case.

> +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> +struct netconsole_target_stats  {
> +	size_t xmit_drop_count;
> +	size_t enomem_count;
> +};
> +#endif

Don't hide types under ifdefs
In fact I'm not sure if hiding stats if DYNAMIC isn't enabled makes
sense. They don't take up much space.

> +static ssize_t stats_show(struct config_item *item, char *buf)
> +{
> +	struct netconsole_target *nt = to_target(item);
> +
> +	return sysfs_emit(buf, "xmit_drop: %lu enomem: %lu\n",
> +		nt->stats.xmit_drop_count, nt->stats.enomem_count);

does configfs require value per file like sysfs or this is okay?

>  /**
>   * send_ext_msg_udp - send extended log message to target
>   * @nt: target to send message to
> @@ -1063,7 +1102,9 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
>  					     "%s", userdata);
>  
>  		msg_ready = buf;
> -		netpoll_send_udp(&nt->np, msg_ready, msg_len);
> +		count_udp_send_stats(nt, netpoll_send_udp(&nt->np,
> +							  msg_ready,
> +							  msg_len));

Please add a wrapper which calls netpoll_send_udp() and counts the
stats. This sort of nested function calls are unlikely to meet kernel
coding requirements.
-- 
pw-bot: cr