[PATCH 1/2] netpoll: Make netpoll_send_udp return status instead of void

[PATCH 1/2] netpoll: Make netpoll_send_udp return status instead of void

[Maksym Kutsevol]

netpoll_send_udp can return if send was successful.
It will allow client code to be aware of the send status.

Possible return values are the result of __netpoll_send_skb (cast to int)
and -ENOMEM. This doesn't cover the case when TX was not successful
instantaneously and was scheduled for later, __netpoll__send_skb returns
success in that case.

Signed-off-by: Maksym Kutsevol <max@kutsevol.com>
---
Used in the next patch to expose send failure stats in netconsole

 include/linux/netpoll.h | 2 +-
 net/core/netpoll.c      | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index bd19c4b91e31..10ceef618e40 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -56,7 +56,7 @@ static inline void netpoll_poll_disable(struct net_device *dev) { return; }
 static inline void netpoll_poll_enable(struct net_device *dev) { return; }
 #endif
 
-void netpoll_send_udp(struct netpoll *np, const char *msg, int len);
+int netpoll_send_udp(struct netpoll *np, const char *msg, int len);
 void netpoll_print_options(struct netpoll *np);
 int netpoll_parse_options(struct netpoll *np, char *opt);
 int __netpoll_setup(struct netpoll *np, struct net_device *ndev);
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index d657b042d5a0..664343e3b688 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -395,7 +395,7 @@ netdev_tx_t netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
 }
 EXPORT_SYMBOL(netpoll_send_skb);
 
-void netpoll_send_udp(struct netpoll *np, const char *msg, int len)
+int netpoll_send_udp(struct netpoll *np, const char *msg, int len)
 {
 	int total_len, ip_len, udp_len;
 	struct sk_buff *skb;
@@ -419,7 +419,7 @@ void netpoll_send_udp(struct netpoll *np, const char *msg, int len)
 	skb = find_skb(np, total_len + np->dev->needed_tailroom,
 		       total_len - len);
 	if (!skb)
-		return;
+		return -ENOMEM;
 
 	skb_copy_to_linear_data(skb, msg, len);
 	skb_put(skb, len);
@@ -495,7 +495,7 @@ void netpoll_send_udp(struct netpoll *np, const char *msg, int len)
 
 	skb->dev = np->dev;
 
-	netpoll_send_skb(np, skb);
+	return (int)netpoll_send_skb(np, skb);
 }
 EXPORT_SYMBOL(netpoll_send_udp);
 

base-commit: 8af174ea863c72f25ce31cee3baad8a301c0cf0f
-- 
2.43.5

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

[Maksym Kutsevol]

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@kutsevol.com>
---
 Documentation/networking/netconsole.rst |  1 +
 drivers/net/netconsole.c                | 54 +++++++++++++++++++++++--
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/netconsole.rst b/Documentation/networking/netconsole.rst
index d55c2a22ec7a..733d4a93878e 100644
--- a/Documentation/networking/netconsole.rst
+++ b/Documentation/networking/netconsole.rst
@@ -135,6 +135,7 @@ The interface exposes these parameters of a netconsole target to userspace:
 	remote_ip	Remote agent's IP address		(read-write)
 	local_mac	Local interface's MAC address		(read-only)
 	remote_mac	Remote agent's MAC address		(read-write)
+	stats		Send error stats			(read-only)
 	==============  =================================       ============
 
 The "enabled" attribute is also used to control whether the parameters of
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;
+};
+#endif
+
 /**
  * struct netconsole_target - Represents a configured netconsole target.
  * @list:	Links this target into the target_list.
@@ -89,6 +96,7 @@ static struct console netconsole_ext;
  * @userdata_group:	Links to the userdata configfs hierarchy
  * @userdata_complete:	Cached, formatted string of append
  * @userdata_length:	String length of userdata_complete
+ * @stats:	UDP send stats for the target. Used for debugging.
  * @enabled:	On / off knob to enable / disable target.
  *		Visible from userspace (read-write).
  *		We maintain a strict 1:1 correspondence between this and
@@ -115,6 +123,7 @@ struct netconsole_target {
 	struct config_group	userdata_group;
 	char userdata_complete[MAX_USERDATA_ENTRY_LENGTH * MAX_USERDATA_ITEMS];
 	size_t			userdata_length;
+	struct netconsole_target_stats stats;
 #endif
 	bool			enabled;
 	bool			extended;
@@ -227,6 +236,7 @@ static struct netconsole_target *alloc_and_init(void)
  *				|	remote_ip
  *				|	local_mac
  *				|	remote_mac
+ *				|	stats
  *				|	userdata/
  *				|		<key>/
  *				|			value
@@ -323,6 +333,14 @@ static ssize_t remote_mac_show(struct config_item *item, char *buf)
 	return sysfs_emit(buf, "%pM\n", to_target(item)->np.remote_mac);
 }
 
+static ssize_t stats_show(struct config_item *item, char *buf)
+{
+	struct netconsole_target *nt = to_target(item);
+
+	return sysfs_emit(buf, "xmit_drop: %lu enomem: %lu\n",
+		nt->stats.xmit_drop_count, nt->stats.enomem_count);
+}
+
 /*
  * This one is special -- targets created through the configfs interface
  * are not enabled (and the corresponding netpoll activated) by default.
@@ -795,6 +813,7 @@ CONFIGFS_ATTR(, remote_ip);
 CONFIGFS_ATTR_RO(, local_mac);
 CONFIGFS_ATTR(, remote_mac);
 CONFIGFS_ATTR(, release);
+CONFIGFS_ATTR_RO(, stats);
 
 static struct configfs_attribute *netconsole_target_attrs[] = {
 	&attr_enabled,
@@ -807,6 +826,7 @@ static struct configfs_attribute *netconsole_target_attrs[] = {
 	&attr_remote_ip,
 	&attr_local_mac,
 	&attr_remote_mac,
+	&attr_stats,
 	NULL,
 };
 
@@ -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++;
+	} else if (result == -ENOMEM) {
+		nt->stats.enomem_count++;
+	};
+#endif
+}
+
 /**
  * send_ext_msg_udp - send extended log message to target
  * @nt: target to send message to
@@ -1063,7 +1102,9 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
 					     "%s", userdata);
 
 		msg_ready = buf;
-		netpoll_send_udp(&nt->np, msg_ready, msg_len);
+		count_udp_send_stats(nt, netpoll_send_udp(&nt->np,
+							  msg_ready,
+							  msg_len));
 		return;
 	}
 
@@ -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)
+		);
 		offset += this_offset;
 	}
 }
@@ -1172,7 +1217,10 @@ static void write_msg(struct console *con, const char *msg, unsigned int len)
 			tmp = msg;
 			for (left = len; left;) {
 				frag = min(left, MAX_PRINT_CHUNK);
-				netpoll_send_udp(&nt->np, tmp, frag);
+				int send_result = netpoll_send_udp(&nt->np,
+								   tmp,
+								   frag);
+				count_udp_send_stats(nt, send_result);
 				tmp += frag;
 				left -= frag;
 			}
-- 
2.43.5

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

[Jakub Kicinski]

On Sat, 24 Aug 2024 14:50:24 -0700 Maksym Kutsevol wrote:
> Enhance observability of netconsole. UDP sends can fail. Start tracking at

nit: "UDP sends" sounds a bit too much like it's using sockets
maybe "packet sends"?

> least two failure possibilities: ENOMEM and NET_XMIT_DROP for every target.
> Stats are exposed via an additional attribute in CONFIGFS.

Please provide a reference to another configfs user in the kernel which
exposes stats. To help reviewers validate it's a legit use case.

> +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> +struct netconsole_target_stats  {
> +	size_t xmit_drop_count;
> +	size_t enomem_count;
> +};
> +#endif

Don't hide types under ifdefs
In fact I'm not sure if hiding stats if DYNAMIC isn't enabled makes
sense. They don't take up much space.

> +static ssize_t stats_show(struct config_item *item, char *buf)
> +{
> +	struct netconsole_target *nt = to_target(item);
> +
> +	return sysfs_emit(buf, "xmit_drop: %lu enomem: %lu\n",
> +		nt->stats.xmit_drop_count, nt->stats.enomem_count);

does configfs require value per file like sysfs or this is okay?

>  /**
>   * send_ext_msg_udp - send extended log message to target
>   * @nt: target to send message to
> @@ -1063,7 +1102,9 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
>  					     "%s", userdata);
>  
>  		msg_ready = buf;
> -		netpoll_send_udp(&nt->np, msg_ready, msg_len);
> +		count_udp_send_stats(nt, netpoll_send_udp(&nt->np,
> +							  msg_ready,
> +							  msg_len));

Please add a wrapper which calls netpoll_send_udp() and counts the
stats. This sort of nested function calls are unlikely to meet kernel
coding requirements.
-- 
pw-bot: cr

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

[Maksym Kutsevol]

Hey Jakub,
thank you for your time looking into this.

PS. Sorry for the html message, noob mistake.

On Mon, Aug 26, 2024 at 5:35 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sat, 24 Aug 2024 14:50:24 -0700 Maksym Kutsevol wrote:
> > Enhance observability of netconsole. UDP sends can fail. Start tracking at
>
> nit: "UDP sends" sounds a bit too much like it's using sockets
> maybe "packet sends"?


Sure, it makes sense, I will update it.

>
> > least two failure possibilities: ENOMEM and NET_XMIT_DROP for every target.
> > Stats are exposed via an additional attribute in CONFIGFS.
>
> Please provide a reference to another configfs user in the kernel which
> exposes stats. To help reviewers validate it's a legit use case.
>
doc/Documentation/block/stat.txt describes what stats block devices expose.
The idea is the same there - a single read gives a coherent snapshot of stats.
Did you mean that the commit message needs updating or info provided
here is enough?

> > +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> > +struct netconsole_target_stats  {
> > +     size_t xmit_drop_count;
> > +     size_t enomem_count;
> > +};
> > +#endif
>
> Don't hide types under ifdefs
> In fact I'm not sure if hiding stats if DYNAMIC isn't enabled makes
> sense. They don't take up much space.
>
I'll remove the ifdef.

Without _DYNAMIC it will not create the sysfs config group, so there
will be no place to expose the stats from,
hence the attachment to dynamic config.
The reason to hide the type comes from the same idea. It's not about
saving space, but about the fact that
it can't exist without it the way it's currently implemented. There's
no way to expose those metrics if _DYNAMIC
is not enabled.

> > +static ssize_t stats_show(struct config_item *item, char *buf)
> > +{
> > +     struct netconsole_target *nt = to_target(item);
> > +
> > +     return
> > +             nt->stats.xmit_drop_count, nt->stats.enomem_count);
>
> does configfs require value per file like sysfs or this is okay?


Docs say (Documentation/filesystems/sysfs.txt):

Attributes should be ASCII text files, preferably with only one value
per file. It is noted that it may not be efficient to contain only one
value per file, so it is socially acceptable to express an array of
values of the same type.

Given those are of the same type, I thought it's ok. To make it less
"fancy" maybe move to
just values separated by whitespace + a block in
Documentation/networking/netconsole.rst describing the format?
E.g. sysfs_emit(buf, "%lu %lu\n", .....) ? I really don't want to have
multiple files for it.
What do you think?

>
>
> >  /**
> >   * send_ext_msg_udp - send extended log message to target
> >   * @nt: target to send message to
> > @@ -1063,7 +1102,9 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
> >                                            "%s", userdata);
> >
> >               msg_ready = buf;
> > -             netpoll_send_udp(&nt->np, msg_ready, msg_len);
> > +             count_udp_send_stats(nt, netpoll_send_udp(&nt->np,
> > +                                                       msg_ready,
> > +                                                       msg_len));
>
> Please add a wrapper which calls netpoll_send_udp() and counts the
> stats. This sort of nested function calls are unlikely to meet kernel
> coding requirements.


I see, will do. Noob question -  I couldn't find any written guidance
regarding this, if you have it handy - I'd appreciate
a link to some guidance regarding passing the result of a function to
a classifier vs wrapper function.

> --
> pw-bot: cr

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

[kernel test robot]

Hi Maksym,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 8af174ea863c72f25ce31cee3baad8a301c0cf0f]

url:    https://github.com/intel-lab-lkp/linux/commits/Maksym-Kutsevol/netcons-Add-udp-send-fail-statistics-to-netconsole/20240826-163850
base:   8af174ea863c72f25ce31cee3baad8a301c0cf0f
patch link:    https://lore.kernel.org/r/20240824215130.2134153-2-max%40kutsevol.com
patch subject: [PATCH 2/2] netcons: Add udp send fail statistics to netconsole
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240827/202408271419.3JJwwxE7-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 08e5a1de8227512d4774a534b91cb2353cef6284)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240827/202408271419.3JJwwxE7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408271419.3JJwwxE7-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/net/netconsole.c:27:
   In file included from include/linux/mm.h:2228:
   include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     517 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   In file included from drivers/net/netconsole.c:35:
   In file included from include/linux/netpoll.h:11:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/net/netconsole.c:35:
   In file included from include/linux/netpoll.h:11:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/net/netconsole.c:35:
   In file included from include/linux/netpoll.h:11:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
>> drivers/net/netconsole.c:341:3: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
     340 |         return sysfs_emit(buf, "xmit_drop: %lu enomem: %lu\n",
         |                                            ~~~
         |                                            %zu
     341 |                 nt->stats.xmit_drop_count, nt->stats.enomem_count);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/netconsole.c:341:30: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
     340 |         return sysfs_emit(buf, "xmit_drop: %lu enomem: %lu\n",
         |                                                        ~~~
         |                                                        %zu
     341 |                 nt->stats.xmit_drop_count, nt->stats.enomem_count);
         |                                            ^~~~~~~~~~~~~~~~~~~~~~
   9 warnings generated.


vim +341 drivers/net/netconsole.c

   335	
   336	static ssize_t stats_show(struct config_item *item, char *buf)
   337	{
   338		struct netconsole_target *nt = to_target(item);
   339	
   340		return sysfs_emit(buf, "xmit_drop: %lu enomem: %lu\n",
 > 341			nt->stats.xmit_drop_count, nt->stats.enomem_count);
   342	}
   343	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Maksym,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 8af174ea863c72f25ce31cee3baad8a301c0cf0f]

url:    https://github.com/intel-lab-lkp/linux/commits/Maksym-Kutsevol/netcons-Add-udp-send-fail-statistics-to-netconsole/20240826-163850
base:   8af174ea863c72f25ce31cee3baad8a301c0cf0f
patch link:    https://lore.kernel.org/r/20240824215130.2134153-2-max%40kutsevol.com
patch subject: [PATCH 2/2] netcons: Add udp send fail statistics to netconsole
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20240827/202408271711.RhzKTDRD-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240827/202408271711.RhzKTDRD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408271711.RhzKTDRD-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/netconsole.c: In function 'stats_show':
>> drivers/net/netconsole.c:340:46: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
     340 |         return sysfs_emit(buf, "xmit_drop: %lu enomem: %lu\n",
         |                                            ~~^
         |                                              |
         |                                              long unsigned int
         |                                            %u
     341 |                 nt->stats.xmit_drop_count, nt->stats.enomem_count);
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~     
         |                          |
         |                          size_t {aka unsigned int}
   drivers/net/netconsole.c:340:58: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
     340 |         return sysfs_emit(buf, "xmit_drop: %lu enomem: %lu\n",
         |                                                        ~~^
         |                                                          |
         |                                                          long unsigned int
         |                                                        %u
     341 |                 nt->stats.xmit_drop_count, nt->stats.enomem_count);
         |                                            ~~~~~~~~~~~~~~~~~~~~~~
         |                                                     |
         |                                                     size_t {aka unsigned int}


vim +340 drivers/net/netconsole.c

   335	
   336	static ssize_t stats_show(struct config_item *item, char *buf)
   337	{
   338		struct netconsole_target *nt = to_target(item);
   339	
 > 340		return sysfs_emit(buf, "xmit_drop: %lu enomem: %lu\n",
   341			nt->stats.xmit_drop_count, nt->stats.enomem_count);
   342	}
   343	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Breno Leitao]

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@kutsevol.com>

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

https://lore.kernel.org/all/ZsWoUzyK5du9Ffl+@gmail.com/

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

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

[Jakub Kicinski]

On Mon, 26 Aug 2024 19:55:36 -0400 Maksym Kutsevol wrote:
> > > +static ssize_t stats_show(struct config_item *item, char *buf)
> > > +{
> > > +     struct netconsole_target *nt = to_target(item);
> > > +
> > > +     return
> > > +             nt->stats.xmit_drop_count, nt->stats.enomem_count);  
> >
> > does configfs require value per file like sysfs or this is okay?  
> 
> Docs say (Documentation/filesystems/sysfs.txt):
> 
> Attributes should be ASCII text files, preferably with only one value
> per file. It is noted that it may not be efficient to contain only one
> value per file, so it is socially acceptable to express an array of
> values of the same type.

Right, but this is for sysfs, main question is whether configfs has 
the same expectations.

> Given those are of the same type, I thought it's ok. To make it less
> "fancy" maybe move to
> just values separated by whitespace + a block in
> Documentation/networking/netconsole.rst describing the format?
> E.g. sysfs_emit(buf, "%lu %lu\n", .....) ? I really don't want to have
> multiple files for it.
> What do you think?

Stats as an array are quite hard to read / understand

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

[Maksym Kutsevol]

Hey Jakub,
thanks for looking into this.

PS. A couple more email send mistakes and I'll go install mutt, sorry
for the noise :)

On Tue, Aug 27, 2024 at 9:59 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 26 Aug 2024 19:55:36 -0400 Maksym Kutsevol wrote:
> > > > +static ssize_t stats_show(struct config_item *item, char *buf)
> > > > +{
> > > > +     struct netconsole_target *nt = to_target(item);
> > > > +
> > > > +     return
> > > > +             nt->stats.xmit_drop_count, nt->stats.enomem_count);
> > >
> > > does configfs require value per file like sysfs or this is okay?
> >
> > Docs say (Documentation/filesystems/sysfs.txt):
> >
> > Attributes should be ASCII text files, preferably with only one value
> > per file. It is noted that it may not be efficient to contain only one
> > value per file, so it is socially acceptable to express an array of
> > values of the same type.
>
> Right, but this is for sysfs, main question is whether configfs has
> the same expectations.
Eh, my bad, thank you :)

Docs on configfs (Documentation/filesystems/configfs.rst) say approximately
the same, quote:
* Normal attributes, which similar to sysfs attributes, are small ASCII text
  files, with a maximum size of one page (PAGE_SIZE, 4096 on i386).  Preferably
  only one value per file should be used, and the same caveats from sysfs apply.
  Configfs expects write(2) to store the entire buffer at once.  When writing to
  normal configfs attributes, userspace processes should first read the entire
  file, modify the portions they wish to change, and then write the entire
  buffer back.

so based on sysfs+configfs docs it looks ok to do so. What do you think?

Regarding the overall idea of exposing stats via configfs I found this:
https://github.com/torvalds/linux/blob/master/drivers/target/iscsi/iscsi_target_stat.c#L82-L87
as an example of another place doing it, which exposes the number of
active sessions.

> > Given those are of the same type, I thought it's ok. To make it less
> > "fancy" maybe move to
> > just values separated by whitespace + a block in
> > Documentation/networking/netconsole.rst describing the format?
> > E.g. sysfs_emit(buf, "%lu %lu\n", .....) ? I really don't want to have
> > multiple files for it.
> > What do you think?
>
> Stats as an array are quite hard to read / understand
I agree with that.
I couldn't find examples of multiple values exported as stats from
configfs. Only from sysfs,
e.g. https://www.kernel.org/doc/Documentation/block/stat.txt, which
describes a whitespace
separated file with stats.

I want to lean on the opinion of someone more experienced in kernel
dev on how to proceed here.
- as is
- whitespace separated like blockdev stats
- multiple files and stop talking about it? :)

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

[Maksym Kutsevol]

Hey Breno,
thanks for looking at it again :)


On Tue, Aug 27, 2024 at 8:18 AM Breno Leitao <leitao@debian.org> wrote:
>
> 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@kutsevol.com>
>
> Would you mind adding a "Reported-by" me in this case?
>
> https://lore.kernel.org/all/ZsWoUzyK5du9Ffl+@gmail.com/

Absolutely!

> > 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!?
>
Do you think that these counters really need more than an int?
Switching them to unsigned int instead might be better?
I'd argue that at the point when an external system collection
interval is not short enough
to see the unsigned counter going to a lesser value (counters are
unsigned, they go to 0 at UINT_MAX+1).
I need advice/pointer on locking - I'm looking at it and it looks to
me as if there's no locking needed when
updating a member of nt there. Tell me if I'm wrong.

> > @@ -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.
ok

> > @@ -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.
ok

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

[Breno Leitao]

Hello Maksym,

On Wed, Aug 28, 2024 at 11:03:09AM -0400, Maksym Kutsevol wrote:

> > Stats as an array are quite hard to read / understand

> I agree with that.
> I couldn't find examples of multiple values exported as stats from
> configfs. Only from sysfs,
> e.g. https://www.kernel.org/doc/Documentation/block/stat.txt, which
> describes a whitespace
> separated file with stats.
> 
> I want to lean on the opinion of someone more experienced in kernel
> dev on how to proceed here.
> - as is
> - whitespace separated like blockdev stats
> - multiple files and stop talking about it? :)

Do we really need both stats/numbers here? Would it be easier if we just have
a "dropped_packet" counter that count packets that netconsole dropped
for one reason or another?

I don't see statistics for lack of memory in regular netdev interfaces.
We probably don't need it for netconsole.

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

[Maksym Kutsevol]

Hey Breno,
thanks for looking at it.


On Wed, Aug 28, 2024 at 11:19 AM Breno Leitao <leitao@debian.org> wrote:
>
> Hello Maksym,
>
> On Wed, Aug 28, 2024 at 10:26:20AM -0400, Maksym Kutsevol wrote:
> > > > 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!?
> > >
> > Do you think that these counters really need more than an int?
>
> An int can overflow and become negative, so, you will see negative
> values, right?
It's an unsigned int (maybe not an int on every platform, but still unsigned),
it will become 0, not negative. E.g.
https://www.programiz.com/online-compiler/3qbkayylX5Cmf

> > Switching them to unsigned int instead might be better?
>
> Why not `u64_stats_sync` ?
>
Only because it's smaller and does the job. Unless locking is needed.

> > I'd argue that at the point when an external system collection
> > interval is not short enough
> > to see the unsigned counter going to a lesser value (counters are
> > unsigned, they go to 0 at UINT_MAX+1).
> > I need advice/pointer on locking - I'm looking at it and it looks to
> > me as if there's no locking needed when
> > updating a member of nt there. Tell me if I'm wrong.
>
> well, they are updated while holding `target_list_lock` right? But I
> would not rely on it.
Then I'll update to use them instead.

> If you just convert the values to u64_stats_sync, you get the
> synchronization for free, basically doing the following:
>
>         u64_stats_update_begin()
>         u64_stats_inc()
>         u64_stats_update_end()
>
> Thanks for working on it,
> --breno
Absolutely, my pleasure :)

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

[Maksym Kutsevol]

Hey Breno,


On Wed, Aug 28, 2024 at 11:25 AM Breno Leitao <leitao@debian.org> wrote:
>
> Hello Maksym,
>
> On Wed, Aug 28, 2024 at 11:03:09AM -0400, Maksym Kutsevol wrote:
>
> > > Stats as an array are quite hard to read / understand
>
> > I agree with that.
> > I couldn't find examples of multiple values exported as stats from
> > configfs. Only from sysfs,
> > e.g. https://www.kernel.org/doc/Documentation/block/stat.txt, which
> > describes a whitespace
> > separated file with stats.
> >
> > I want to lean on the opinion of someone more experienced in kernel
> > dev on how to proceed here.
> > - as is
> > - whitespace separated like blockdev stats
> > - multiple files and stop talking about it? :)
>
> Do we really need both stats/numbers here? Would it be easier if we just have
> a "dropped_packet" counter that count packets that netconsole dropped
> for one reason or another?
>
> I don't see statistics for lack of memory in regular netdev interfaces.
> We probably don't need it for netconsole.
I like this idea! I'll send a V2 with only one attribute.
Thanks, Breno!

[PATCH v2 1/2] netpoll: Make netpoll_send_udp return status instead of void

[Maksym Kutsevol]

netpoll_send_udp can return if send was successful.
It will allow client code to be aware of the send status.

Possible return values are the result of __netpoll_send_skb (cast to int)
and -ENOMEM. This doesn't cover the case when TX was not successful
instantaneously and was scheduled for later, __netpoll__send_skb returns
success in that case.

Signed-off-by: Maksym Kutsevol <max@kutsevol.com>
---
Used in the next patch to expose send failure stats in netconsole

Changelog:

v2: No changes, resend.

v1:
 * https://lore.kernel.org/netdev/20240824215130.2134153-1-max@kutsevol.com/

 include/linux/netpoll.h | 2 +-
 net/core/netpoll.c      | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index bd19c4b91e31..10ceef618e40 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -56,7 +56,7 @@ static inline void netpoll_poll_disable(struct net_device *dev) { return; }
 static inline void netpoll_poll_enable(struct net_device *dev) { return; }
 #endif
 
-void netpoll_send_udp(struct netpoll *np, const char *msg, int len);
+int netpoll_send_udp(struct netpoll *np, const char *msg, int len);
 void netpoll_print_options(struct netpoll *np);
 int netpoll_parse_options(struct netpoll *np, char *opt);
 int __netpoll_setup(struct netpoll *np, struct net_device *ndev);
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index d657b042d5a0..664343e3b688 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -395,7 +395,7 @@ netdev_tx_t netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
 }
 EXPORT_SYMBOL(netpoll_send_skb);
 
-void netpoll_send_udp(struct netpoll *np, const char *msg, int len)
+int netpoll_send_udp(struct netpoll *np, const char *msg, int len)
 {
 	int total_len, ip_len, udp_len;
 	struct sk_buff *skb;
@@ -419,7 +419,7 @@ void netpoll_send_udp(struct netpoll *np, const char *msg, int len)
 	skb = find_skb(np, total_len + np->dev->needed_tailroom,
 		       total_len - len);
 	if (!skb)
-		return;
+		return -ENOMEM;
 
 	skb_copy_to_linear_data(skb, msg, len);
 	skb_put(skb, len);
@@ -495,7 +495,7 @@ void netpoll_send_udp(struct netpoll *np, const char *msg, int len)
 
 	skb->dev = np->dev;
 
-	netpoll_send_skb(np, skb);
+	return (int)netpoll_send_skb(np, skb);
 }
 EXPORT_SYMBOL(netpoll_send_udp);
 

base-commit: 3a0504d54b3b57f0d7bf3d9184a00c9f8887f6d7
-- 
2.43.5

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

[Maksym Kutsevol]

Enhance observability of netconsole. Packet 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.

Reported-by: Breno Leitao <leitao@debian.org>
Closes: https://lore.kernel.org/all/ZsWoUzyK5du9Ffl+@gmail.com/
Signed-off-by: Maksym Kutsevol <max@kutsevol.com>
---
Changelog:

v2:
 * fixed commit message wording and reported-by reference.
 * not hiding netconsole_target_stats when CONFIG_NETCONSOLE_DYNAMIC
   is not enabled.
 * rename stats attribute in configfs to transmit_errors and make it
   a single u64 value, which is a sum of errors that occured.
 * make a wrapper function to count errors instead of a return result
   classifier one.
 * use u64_stats_sync.h to manage stats.

v1:
 * https://lore.kernel.org/netdev/20240824215130.2134153-2-max@kutsevol.com/
 Documentation/networking/netconsole.rst |  5 +-
 drivers/net/netconsole.c                | 63 +++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/Documentation/networking/netconsole.rst b/Documentation/networking/netconsole.rst
index d55c2a22ec7a..94c4680fdf3e 100644
--- a/Documentation/networking/netconsole.rst
+++ b/Documentation/networking/netconsole.rst
@@ -124,7 +124,7 @@ To remove a target::
 
 The interface exposes these parameters of a netconsole target to userspace:
 
-	==============  =================================       ============
+	=============== =================================       ============
 	enabled		Is this target currently enabled?	(read-write)
 	extended	Extended mode enabled			(read-write)
 	release		Prepend kernel release to message	(read-write)
@@ -135,7 +135,8 @@ The interface exposes these parameters of a netconsole target to userspace:
 	remote_ip	Remote agent's IP address		(read-write)
 	local_mac	Local interface's MAC address		(read-only)
 	remote_mac	Remote agent's MAC address		(read-write)
-	==============  =================================       ============
+	transmit_errors	Number of packet send errors		(read-only)
+	=============== =================================       ============
 
 The "enabled" attribute is also used to control whether the parameters of
 a target can be updated or not -- you can modify the parameters of only
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>");
@@ -82,6 +83,12 @@ static DEFINE_SPINLOCK(target_list_lock);
  */
 static struct console netconsole_ext;
 
+struct netconsole_target_stats  {
+	u64_stats_t xmit_drop_count;
+	u64_stats_t enomem_count;
+	struct u64_stats_sync syncp;
+} __aligned(2 * sizeof(u64));
+
 /**
  * struct netconsole_target - Represents a configured netconsole target.
  * @list:	Links this target into the target_list.
@@ -89,6 +96,7 @@ static struct console netconsole_ext;
  * @userdata_group:	Links to the userdata configfs hierarchy
  * @userdata_complete:	Cached, formatted string of append
  * @userdata_length:	String length of userdata_complete
+ * @stats:	Packet send stats for the target. Used for debugging.
  * @enabled:	On / off knob to enable / disable target.
  *		Visible from userspace (read-write).
  *		We maintain a strict 1:1 correspondence between this and
@@ -115,6 +123,7 @@ struct netconsole_target {
 	struct config_group	userdata_group;
 	char userdata_complete[MAX_USERDATA_ENTRY_LENGTH * MAX_USERDATA_ITEMS];
 	size_t			userdata_length;
+	struct netconsole_target_stats stats;
 #endif
 	bool			enabled;
 	bool			extended;
@@ -227,6 +236,7 @@ static struct netconsole_target *alloc_and_init(void)
  *				|	remote_ip
  *				|	local_mac
  *				|	remote_mac
+ *				|	transmit_errors
  *				|	userdata/
  *				|		<key>/
  *				|			value
@@ -323,6 +333,21 @@ static ssize_t remote_mac_show(struct config_item *item, char *buf)
 	return sysfs_emit(buf, "%pM\n", to_target(item)->np.remote_mac);
 }
 
+static ssize_t transmit_errors_show(struct config_item *item, char *buf)
+{
+	struct netconsole_target *nt = to_target(item);
+	u64 xmit_drop_count, enomem_count;
+	unsigned int start;
+
+	do {
+		start = u64_stats_fetch_begin(&nt->stats.syncp);
+		xmit_drop_count = u64_stats_read(&nt->stats.xmit_drop_count);
+		enomem_count = u64_stats_read(&nt->stats.enomem_count);
+	} while (u64_stats_fetch_retry(&nt->stats.syncp, start));
+
+	return sysfs_emit(buf, "%llu\n", xmit_drop_count + enomem_count);
+}
+
 /*
  * This one is special -- targets created through the configfs interface
  * are not enabled (and the corresponding netpoll activated) by default.
@@ -795,6 +820,7 @@ CONFIGFS_ATTR(, remote_ip);
 CONFIGFS_ATTR_RO(, local_mac);
 CONFIGFS_ATTR(, remote_mac);
 CONFIGFS_ATTR(, release);
+CONFIGFS_ATTR_RO(, transmit_errors);
 
 static struct configfs_attribute *netconsole_target_attrs[] = {
 	&attr_enabled,
@@ -807,6 +833,7 @@ static struct configfs_attribute *netconsole_target_attrs[] = {
 	&attr_remote_ip,
 	&attr_local_mac,
 	&attr_remote_mac,
+	&attr_transmit_errors,
 	NULL,
 };
 
@@ -1015,6 +1042,36 @@ static struct notifier_block netconsole_netdev_notifier = {
 	.notifier_call  = netconsole_netdev_event,
 };
 
+/**
+ * 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)
+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;
+	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
+}
+
 /**
  * send_ext_msg_udp - send extended log message to target
  * @nt: target to send message to
@@ -1063,7 +1120,7 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
 					     "%s", userdata);
 
 		msg_ready = buf;
-		netpoll_send_udp(&nt->np, msg_ready, msg_len);
+		netpoll_send_udp_count_errs(nt, msg_ready, msg_len);
 		return;
 	}
 
@@ -1126,7 +1183,7 @@ 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);
+		netpoll_send_udp_count_errs(nt, buf, this_header + this_offset);
 		offset += this_offset;
 	}
 }
@@ -1172,7 +1229,7 @@ static void write_msg(struct console *con, const char *msg, unsigned int len)
 			tmp = msg;
 			for (left = len; left;) {
 				frag = min(left, MAX_PRINT_CHUNK);
-				netpoll_send_udp(&nt->np, tmp, frag);
+				netpoll_send_udp_count_errs(nt, tmp, frag);
 				tmp += frag;
 				left -= frag;
 			}
-- 
2.43.5

Re: [PATCH v2 1/2] netpoll: Make netpoll_send_udp return status instead of void

[Jakub Kicinski]

On Wed, 28 Aug 2024 14:33:48 -0700 Maksym Kutsevol wrote:
> netpoll_send_udp can return if send was successful.
> It will allow client code to be aware of the send status.
> 
> Possible return values are the result of __netpoll_send_skb (cast to int)
> and -ENOMEM. This doesn't cover the case when TX was not successful
> instantaneously and was scheduled for later, __netpoll__send_skb returns
> success in that case.

no need to repost but, quoting documentation:

  Resending after review
  ~~~~~~~~~~~~~~~~~~~~~~
  
  Allow at least 24 hours to pass between postings. This will ensure reviewers
  from all geographical locations have a chance to chime in. Do not wait
  too long (weeks) between postings either as it will make it harder for reviewers
  to recall all the context.
  
  Make sure you address all the feedback in your new posting. Do not post a new
  version of the code if the discussion about the previous version is still
  ongoing, unless directly instructed by a reviewer.
  
  The new version of patches should be posted as a separate thread,
  not as a reply to the previous posting. Change log should include a link
  to the previous posting (see :ref:`Changes requested`).
  
See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#resending-after-review

Re: [PATCH v2 1/2] netpoll: Make netpoll_send_udp return status instead of void

[Maksym Kutsevol]

Hey Jakub,

On Wed, Aug 28, 2024 at 6:58 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 28 Aug 2024 14:33:48 -0700 Maksym Kutsevol wrote:
> > netpoll_send_udp can return if send was successful.
> > It will allow client code to be aware of the send status.
> >
> > Possible return values are the result of __netpoll_send_skb (cast to int)
> > and -ENOMEM. This doesn't cover the case when TX was not successful
> > instantaneously and was scheduled for later, __netpoll__send_skb returns
> > success in that case.
>
> no need to repost but, quoting documentation:
I definitely didn't find this doc, thanks for the link, looked at it,
and I see at least another error in
this submission - there's no designation which tree it's for, it
should be net-next. Will follow
the doc for in the future.

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@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

Re: [PATCH v2 1/2] netpoll: Make netpoll_send_udp return status instead of void

[Breno Leitao]

On Wed, Aug 28, 2024 at 02:33:48PM -0700, Maksym Kutsevol wrote:
> netpoll_send_udp can return if send was successful.
> It will allow client code to be aware of the send status.
> 
> Possible return values are the result of __netpoll_send_skb (cast to int)
> and -ENOMEM. This doesn't cover the case when TX was not successful
> instantaneously and was scheduled for later, __netpoll__send_skb returns
> success in that case.
> 
> Signed-off-by: Maksym Kutsevol <max@kutsevol.com>

Reviewed-by: Breno Leitao <leitao@debian.org>

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

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

[Breno Leitao]

Hello Maksym,

On Fri, Aug 30, 2024 at 08:58:12AM -0400, Maksym Kutsevol wrote:

> > 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?

I would say both. I try to reduce as much as possible the number of
similar calls and #else(s) is the goal.

At the same time, I admit it is easier said than done, and Jakub is
usually the one that helps me to reach the last mile.

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

[Maksym Kutsevol]

Hello Breno,

On Fri, Aug 30, 2024 at 10:12 AM Breno Leitao <leitao@debian.org> wrote:
>
> Hello Maksym,
>
> On Fri, Aug 30, 2024 at 08:58:12AM -0400, Maksym Kutsevol wrote:
>
> > > 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?
>
> I would say both. I try to reduce as much as possible the number of
> similar calls and #else(s) is the goal.
>
> At the same time, I admit it is easier said than done, and Jakub is
> usually the one that helps me to reach the last mile.
I see, ok.
I was more looking for possible (maybe impossible) compiler
optimizations in this case.

but access to nt->np probably makes it impossible anyway, I don't know
compilers well
enough to say.

I'll make it an if then, e.g.
if (IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC)){

}