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

[Maksym Kutsevol]

Hello Breno,


On Fri, Aug 30, 2024 at 4:45 AM Breno Leitao <leitao@xxxxxxxxxx> wrote:
>
> 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.
Yes, that's true. Jacub sent me the link to the net-tree specific
contribution doc, I also found
that. Will fix it in the next set.

> 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?
Yes, thank you.

> > +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.
Unfortunately I sent this patch with my debugging addons, this is plainly wrong.
Will remove.

> > +     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):
Two calls in two different places to netpoll_send_udp bothers you or
the way it has to distinct cases for enabled/disabled and you prefer to
have it as added steps for the case when it's enabled?


> 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