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

Hello Breno,


On Fri, Aug 30, 2024 at 4:45 AM Breno Leitao [off-list ref] wrote:

Hello Maksym,

On Wed, Aug 28, 2024 at 02:33:49PM -0700, Maksym Kutsevol wrote:

quoted

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@selenic.com>");

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.

quoted

+/**
+ * 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.

quoted

+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.

quoted

+     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