[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240826143546.77669b47@kernel.org>
Date: Mon, 26 Aug 2024 14:35:46 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Maksym Kutsevol <max@...sevol.com>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
<edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Breno Leitao
<leitao@...ian.org>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] netcons: Add udp send fail statistics to netconsole
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
Powered by blists - more mailing lists