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

[Breno Leitao]

Hello Maksym,

On Wed, Aug 28, 2024 at 02:33:49PM -0700, Maksym Kutsevol wrote:
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 9c09293b5258..e14b13a8e0d2 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -36,6 +36,7 @@
>  #include <linux/inet.h>
>  #include <linux/configfs.h>
>  #include <linux/etherdevice.h>
> +#include <linux/u64_stats_sync.h>
>  #include <linux/utsname.h>
>  
>  MODULE_AUTHOR("Maintainer: Matt Mackall <mpm@xxxxxxxxxxx>");

I am afraid that you are not build the patch against net-next, since
this line was changed a while ago.

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=10a6545f0bdc

Please develop on top of net-next, otherwise the patch might not apply
on top of net-next.

> +/**
> + * netpoll_send_udp_count_errs - Wrapper for netpoll_send_udp that counts errors
> + * @nt: target to send message to
> + * @msg: message to send
> + * @len: length of message
> + *
> + * Calls netpoll_send_udp and classifies the return value. If an error
> + * occurred it increments statistics in nt->stats accordingly.
> + * Noop if CONFIG_NETCONSOLE_DYNAMIC is disabled.
> + */
> +// static void netpoll_send_udp_count_errs(struct netpoll *np, const char *msg, int len)

Have you forgot to remove the line above?

> +static void netpoll_send_udp_count_errs(struct netconsole_target *nt, const char *msg, int len)
> +{
> +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> +	int result = netpoll_send_udp(&nt->np, msg, len);
> +	result = NET_XMIT_DROP;

Could you please clarify why do you want to overwrite `result` here with
NET_XMIT_DROP? It seems wrong.

> +	if (result == NET_XMIT_DROP) {
> +		u64_stats_update_begin(&nt->stats.syncp);
> +		u64_stats_inc(&nt->stats.xmit_drop_count);
> +		u64_stats_update_end(&nt->stats.syncp);
> +	} else if (result == -ENOMEM) {
> +		u64_stats_update_begin(&nt->stats.syncp);
> +		u64_stats_inc(&nt->stats.enomem_count);
> +		u64_stats_update_end(&nt->stats.syncp);
> +	};
> +#else
> +	netpoll_send_udp(&nt->np, msg, len);
> +#endif

I am not sure this if/else/endif is the best way. I am wondering if
something like this would make the code simpler (uncompiled/untested):

static void netpoll_send_udp_count_errs(struct netconsole_target *nt, const char *msg, int len)
{
        int __maybe_unused result;

        result = netpoll_send_udp(&nt->np, msg, len);
#ifdef CONFIG_NETCONSOLE_DYNAMIC
	switch (result) {
	case NET_XMIT_DROP:
                u64_stats_update_begin(&nt->stats.syncp);
                u64_stats_inc(&nt->stats.xmit_drop_count);
                u64_stats_update_end(&nt->stats.syncp);
		breadk;
        case ENOMEM:
                u64_stats_update_begin(&nt->stats.syncp);
                u64_stats_inc(&nt->stats.enomem_count);
                u64_stats_update_end(&nt->stats.syncp);
		break;
        };
#endif

Thanks for working on it.
--breno