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

From: Breno Leitao
Date: Tue Aug 27 2024 - 08:18:32 EST


On Sat, Aug 24, 2024 at 02:50:24PM -0700, Maksym Kutsevol wrote:
> Enhance observability of netconsole. UDP sends can fail. Start tracking at
> least two failure possibilities: ENOMEM and NET_XMIT_DROP for every target.
> Stats are exposed via an additional attribute in CONFIGFS.
>
> The exposed statistics allows easier debugging of cases when netconsole
> messages were not seen by receivers, eliminating the guesswork if the
> sender thinks that messages in question were sent out.
>
> Stats are not reset on enable/disable/change remote ip/etc, they
> belong to the netcons target itself.
>
> Signed-off-by: Maksym Kutsevol <max@xxxxxxxxxxxx>

Would you mind adding a "Reported-by" me in this case?

https://lore.kernel.org/all/ZsWoUzyK5du9Ffl+@xxxxxxxxx/

> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 9c09293b5258..45c07ec7842d 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -82,6 +82,13 @@ static DEFINE_SPINLOCK(target_list_lock);
> */
> static struct console netconsole_ext;
>
> +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> +struct netconsole_target_stats {
> + size_t xmit_drop_count;
> + size_t enomem_count;

I am looking at other drivers, and they use a specific type for these
counters, u64_stats_sync.

if it is possible to use this format, then you can leverage the
`__u64_stats_update` helpers, and not worry about locking/overflow!?

> @@ -1015,6 +1035,25 @@ static struct notifier_block netconsole_netdev_notifier = {
> .notifier_call = netconsole_netdev_event,
> };
>
> +/**
> + * count_udp_send_stats - Classify netpoll_send_udp result and count errors.
> + * @nt: target that was sent to
> + * @result: result of netpoll_send_udp
> + *
> + * Takes the result of netpoll_send_udp and classifies the type of error that
> + * occurred. Increments statistics in nt->stats accordingly.
> + */
> +static void count_udp_send_stats(struct netconsole_target *nt, int result)
> +{
> +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> + if (result == NET_XMIT_DROP) {
> + nt->stats.xmit_drop_count++;

If you change the type, you can use the `u64_stats_inc` helper here.

> @@ -1126,7 +1167,11 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
> this_offset += this_chunk;
> }
>
> - netpoll_send_udp(&nt->np, buf, this_header + this_offset);
> + count_udp_send_stats(nt,
> + netpoll_send_udp(&nt->np,
> + buf,
> + this_header + this_offset)
> + );

as Jakub said, this is not a format that is common in the Linux kenrel.