Re: [PATCH v2 2/2] netcons: Add udp send fail statistics to netconsole
From: Breno Leitao <leitao@debian.org>
Date: 2024-08-30 08:45:32
Also in:
lkml
Hello Maksym, On Wed, Aug 28, 2024 at 02:33:49PM -0700, Maksym Kutsevol wrote:
quoted hunk ↗ jump to hunk
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. 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