From: Breno Leitao <[email protected]>
Create a new netconsole Kconfig option that prepends the kernel version in
the netconsole message. This is useful to map kernel messages to kernel
version in a simple way, i.e., without checking somewhere which kernel
version the host that sent the message is using.
If this option is selected, then the "<uname>;" is prepended before the
netconsole message. This is an example of a netcons output, with this
feature enabled:
6.4.0-01762-ga1ba2ffe946e;12,426,112883998,-;this is a test
Calvin Owens send a RFC about this problem in 2016[1], but his
approach was a bit more intrusive, changing the printk subsystem. This
approach is lighter, and just append the information in the last mile,
just before netconsole push the message to netpoll.
[1] Link: https://lore.kernel.org/all/51047c0f6e86abcb9ee13f60653b6946f8fcfc99.1463172791.git.calvinowens@fb.com/
Signed-off-by: Breno Leitao <[email protected]>
Cc: Dave Jones <[email protected]>
---
drivers/net/Kconfig | 10 ++++++++++
drivers/net/netconsole.c | 35 ++++++++++++++++++++++++++++++++++-
2 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index d0a1ed216d15..df50fdb6c794 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -332,6 +332,16 @@ config NETCONSOLE_DYNAMIC
at runtime through a userspace interface exported using configfs.
See <file:Documentation/networking/netconsole.rst> for details.
+config NETCONSOLE_UNAME
+ bool "Add the kernel version to netconsole lines"
+ depends on NETCONSOLE
+ default n
+ help
+ This option causes extended netcons messages to be prepended with
+ kernel uname version. This can be useful for monitoring a large
+ deployment of servers, so, you can easily map outputs to kernel
+ versions.
+
config NETPOLL
def_bool NETCONSOLE
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 4f4f79532c6c..7edc5b033e14 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/utsname.h>
MODULE_AUTHOR("Maintainer: Matt Mackall <[email protected]>");
MODULE_DESCRIPTION("Console driver for network interfaces");
@@ -815,6 +816,38 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
}
}
+#ifdef CONFIG_NETCONSOLE_UNAME
+static void send_ext_msg_udp_uname(struct netconsole_target *nt,
+ const char *msg, unsigned int len)
+{
+ unsigned int newlen;
+ char *newmsg;
+ char *uname;
+
+ uname = init_utsname()->release;
+
+ newmsg = kasprintf(GFP_KERNEL, "%s;%s", uname, msg);
+ if (!newmsg)
+ /* In case of ENOMEM, just ignore this entry */
+ return;
+ newlen = strlen(uname) + len + 1;
+
+ send_ext_msg_udp(nt, newmsg, newlen);
+
+ kfree(newmsg);
+}
+#endif
+
+static inline void send_msg_udp(struct netconsole_target *nt,
+ const char *msg, unsigned int len)
+{
+#ifdef CONFIG_NETCONSOLE_UNAME
+ send_ext_msg_udp_uname(nt, msg, len);
+#else
+ send_ext_msg_udp(nt, msg, len);
+#endif
+}
+
static void write_ext_msg(struct console *con, const char *msg,
unsigned int len)
{
@@ -827,7 +860,7 @@ static void write_ext_msg(struct console *con, const char *msg,
spin_lock_irqsave(&target_list_lock, flags);
list_for_each_entry(nt, &target_list, list)
if (nt->extended && nt->enabled && netif_running(nt->np.dev))
- send_ext_msg_udp(nt, msg, len);
+ send_msg_udp(nt, msg, len);
spin_unlock_irqrestore(&target_list_lock, flags);
}
--
2.34.1
>
> Signed-off-by: Breno Leitao <[email protected]>
> Cc: Dave Jones <[email protected]>
Signed-off-by should come last.
> +#ifdef CONFIG_NETCONSOLE_UNAME
> +static void send_ext_msg_udp_uname(struct netconsole_target *nt,
> + const char *msg, unsigned int len)
> +{
> + unsigned int newlen;
> + char *newmsg;
> + char *uname;
> +
> + uname = init_utsname()->release;
> +
> + newmsg = kasprintf(GFP_KERNEL, "%s;%s", uname, msg);
> + if (!newmsg)
> + /* In case of ENOMEM, just ignore this entry */
> + return;
Hi Breno
Why not just send the message without uname appended. You probably
want to see the OOM messages...
Also, what context are we in here? Should that be GFP_ATOMIC, which
net/core/netpoll.c is using to allocate the skbs?
> +static inline void send_msg_udp(struct netconsole_target *nt,
> + const char *msg, unsigned int len)
> +{
> +#ifdef CONFIG_NETCONSOLE_UNAME
> + send_ext_msg_udp_uname(nt, msg, len);
> +#else
> + send_ext_msg_udp(nt, msg, len);
> +#endif
Please use
if (IS_ENABLED(CONFIG_NETCONSOLE_UNAME)) {} else {}
so the code is compiled and then thrown away. That nakes build testing
more efficient.
Andrew
On Mon, 3 Jul 2023 08:41:54 -0700
[email protected] wrote:
> +config NETCONSOLE_UNAME
> + bool "Add the kernel version to netconsole lines"
> + depends on NETCONSOLE
> + default n
> + help
> + This option causes extended netcons messages to be prepended with
> + kernel uname version. This can be useful for monitoring a large
> + deployment of servers, so, you can easily map outputs to kernel
> + versions.
This should be runtime configured like other netconsole options.
Not enabled at compile time.
On Mon, 3 Jul 2023 08:41:54 -0700 [email protected] wrote:
> + uname = init_utsname()->release;
> +
> + newmsg = kasprintf(GFP_KERNEL, "%s;%s", uname, msg);
> + if (!newmsg)
> + /* In case of ENOMEM, just ignore this entry */
> + return;
> + newlen = strlen(uname) + len + 1;
> +
> + send_ext_msg_udp(nt, newmsg, newlen);
> +
> + kfree(newmsg);
You can avoid the memory allocation by putting this code into
send_ext_msg_udp(), I reckon? There's already a buffer there.
--
pw-bot: cr
Hello Stephen,
On Mon, Jul 03, 2023 at 11:34:10AM -0700, Stephen Hemminger wrote:
> On Mon, 3 Jul 2023 08:41:54 -0700
> [email protected] wrote:
>
> > +config NETCONSOLE_UNAME
> > + bool "Add the kernel version to netconsole lines"
> > + depends on NETCONSOLE
> > + default n
> > + help
> > + This option causes extended netcons messages to be prepended with
> > + kernel uname version. This can be useful for monitoring a large
> > + deployment of servers, so, you can easily map outputs to kernel
> > + versions.
>
> This should be runtime configured like other netconsole options.
> Not enabled at compile time.
Do you mean I should add a new option to netconsole line? This is the
current line format today:
[+][src-port]@[src-ip]/[<dev>],[tgt-port]@<tgt-ip>/[tgt-macaddr]
If that is the case, I suppose I want to add something at the beginning
of format, that specify that uname should be sent. What about something
as?
[u][+][src-port]@[src-ip]/[<dev>],[tgt-port]@<tgt-ip>/[tgt-macaddr]
Thanks!
On Mon, Jul 03, 2023 at 12:44:27PM -0700, Jakub Kicinski wrote:
> On Mon, 3 Jul 2023 08:41:54 -0700 [email protected] wrote:
> > + uname = init_utsname()->release;
> > +
> > + newmsg = kasprintf(GFP_KERNEL, "%s;%s", uname, msg);
> > + if (!newmsg)
> > + /* In case of ENOMEM, just ignore this entry */
> > + return;
> > + newlen = strlen(uname) + len + 1;
> > +
> > + send_ext_msg_udp(nt, newmsg, newlen);
> > +
> > + kfree(newmsg);
>
> You can avoid the memory allocation by putting this code into
> send_ext_msg_udp(), I reckon? There's already a buffer there.
This is doable as well, basically appending "uname" at the beginning of
the buffer, copying the rest of the message to the buffer and sending
the buffer to netpoll.
If the message needs fragmentation, basically keep "uname" as part of
the header, and the new header length (this_header) will be "header_len
+ uname_len"
This is the code that does it. How does it sound?
--
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 4f4f79532c6c..d26bd3b785c4 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/utsname.h>
MODULE_AUTHOR("Maintainer: Matt Mackall <[email protected]>");
MODULE_DESCRIPTION("Console driver for network interfaces");
@@ -772,8 +773,10 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
const char *header, *body;
int offset = 0;
int header_len, body_len;
+ int uname_len = 0;
- if (msg_len <= MAX_PRINT_CHUNK) {
+ if (msg_len <= MAX_PRINT_CHUNK &&
+ !IS_ENABLED(CONFIG_NETCONSOLE_UNAME)) {
netpoll_send_udp(&nt->np, msg, msg_len);
return;
}
@@ -788,14 +791,31 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
body_len = msg_len - header_len - 1;
body++;
+ if (IS_ENABLED(CONFIG_NETCONSOLE_UNAME)) {
+ /* Add uname at the beginning of buffer */
+ char *uname = init_utsname()->release;
+ /* uname_len contains the length of uname + ',' */
+ uname_len = strlen(uname) + 1;
+
+ if (uname_len + msg_len < MAX_PRINT_CHUNK) {
+ /* No fragmentation needed */
+ scnprintf(buf, MAX_PRINT_CHUNK, "%s,%s", uname, msg);
+ netpoll_send_udp(&nt->np, buf, uname_len + msg_len);
+ return;
+ }
+
+ /* The data will be fragment, prepending uname */
+ scnprintf(buf, MAX_PRINT_CHUNK, "%s,", uname);
+ }
+
/*
* Transfer multiple chunks with the following extra header.
* "ncfrag=<byte-offset>/<total-bytes>"
*/
- memcpy(buf, header, header_len);
+ memcpy(buf + uname_len, header, header_len);
while (offset < body_len) {
- int this_header = header_len;
+ int this_header = header_len + uname_len;
int this_chunk;
this_header += scnprintf(buf + this_header,
--
2.34.1
On Mon, Jul 03, 2023 at 06:46:25PM +0200, Andrew Lunn wrote:
> Hi Breno
Hello,
> Why not just send the message without uname appended. You probably
> want to see the OOM messages...
>
> Also, what context are we in here? Should that be GFP_ATOMIC, which
> net/core/netpoll.c is using to allocate the skbs?
Maybe this is not necessary anymore, since I might be using the buffer
already allocated.
> > +static inline void send_msg_udp(struct netconsole_target *nt,
> > + const char *msg, unsigned int len)
> > +{
> > +#ifdef CONFIG_NETCONSOLE_UNAME
> > + send_ext_msg_udp_uname(nt, msg, len);
> > +#else
> > + send_ext_msg_udp(nt, msg, len);
> > +#endif
>
> Please use
>
> if (IS_ENABLED(CONFIG_NETCONSOLE_UNAME)) {} else {}
>
> so the code is compiled and then thrown away. That nakes build testing
> more efficient.
Makes total sense, I am incorporating it into v2 now.
Thanks!
On Tue, 4 Jul 2023 08:15:47 -0700
Breno Leitao <[email protected]> wrote:
> Hello Stephen,
>
> On Mon, Jul 03, 2023 at 11:34:10AM -0700, Stephen Hemminger wrote:
> > On Mon, 3 Jul 2023 08:41:54 -0700
> > [email protected] wrote:
> >
> > > +config NETCONSOLE_UNAME
> > > + bool "Add the kernel version to netconsole lines"
> > > + depends on NETCONSOLE
> > > + default n
> > > + help
> > > + This option causes extended netcons messages to be prepended with
> > > + kernel uname version. This can be useful for monitoring a large
> > > + deployment of servers, so, you can easily map outputs to kernel
> > > + versions.
> >
> > This should be runtime configured like other netconsole options.
> > Not enabled at compile time.
>
> Do you mean I should add a new option to netconsole line? This is the
> current line format today:
>
> [+][src-port]@[src-ip]/[<dev>],[tgt-port]@<tgt-ip>/[tgt-macaddr]
>
> If that is the case, I suppose I want to add something at the beginning
> of format, that specify that uname should be sent. What about something
> as?
>
> [u][+][src-port]@[src-ip]/[<dev>],[tgt-port]@<tgt-ip>/[tgt-macaddr]
>
> Thanks!
Keep it as simple as possible.
What ever program is reading udp socket knows where it is coming from.
The uname is really not needed.
On Tue, Jul 04, 2023 at 08:58:00AM -0700, Stephen Hemminger wrote:
> > > This should be runtime configured like other netconsole options.
> > > Not enabled at compile time.
> >
> > Do you mean I should add a new option to netconsole line? This is the
> > current line format today:
> >
> > [+][src-port]@[src-ip]/[<dev>],[tgt-port]@<tgt-ip>/[tgt-macaddr]
> >
> > If that is the case, I suppose I want to add something at the beginning
> > of format, that specify that uname should be sent. What about something
> > as?
> >
> > [u][+][src-port]@[src-ip]/[<dev>],[tgt-port]@<tgt-ip>/[tgt-macaddr]
> >
> > Thanks!
>
> Keep it as simple as possible.
> What ever program is reading udp socket knows where it is coming from.
Right, the server knows from where the package is coming, so, the source
address is known at receive time, and that is good. I want to do the
same with uname.
> The uname is really not needed.
The uname is useful if the receiver side is looking (grepping) for
specific messages (warnings, oops, etc) affecting specific kernel
versions. If the uname is not available, the receiver needs to read boot
message and keep a map for source IP to kernel version. This is far from
ideal at a hyperscale level.
Things get worse when you have VMs using different kernels, and both
host and guests are sending traffic to the same receiver. In this case, you
have two different kernels versions mapped to the same IP.
Thanks!
On Wed, 5 Jul 2023 08:26:04 -0700 Stephen Hemminger wrote:
> On Wed, 5 Jul 2023 02:18:03 -0700
> Breno Leitao <[email protected]> wrote:
>
> > The uname is useful if the receiver side is looking (grepping) for
> > specific messages (warnings, oops, etc) affecting specific kernel
> > versions. If the uname is not available, the receiver needs to read boot
> > message and keep a map for source IP to kernel version. This is far from
> > ideal at a hyperscale level.
>
> At hyperscale you need a real collector (not just netcat) that can consult
> the VM database to based on IP and record the meta data there. If you allow
> random updates and versions, things get out of control real fast and this
> won't really help much
VM world is simpler because the orchestrator knows exactly what it's
launching each time. Bare metal is more complicated, especially
with modern automation designs where the DBs may contain _intended_
state, and local host agent performs actions to bring the machine
into the intended state.
Not to mention that there may be multiple kernels at play (provisioning
flow, bootloader / EFI, prod, kdump etc.)
As a kernel dev I do like the 100% certainty as to which kernel version
was running at the time of the problem.
On Tue, 4 Jul 2023 08:47:23 -0700 Breno Leitao wrote:
> This is the code that does it. How does it sound?
More or less :)
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 4f4f79532c6c..d26bd3b785c4 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/utsname.h>
>
> MODULE_AUTHOR("Maintainer: Matt Mackall <[email protected]>");
> MODULE_DESCRIPTION("Console driver for network interfaces");
> @@ -772,8 +773,10 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
> const char *header, *body;
> int offset = 0;
> int header_len, body_len;
> + int uname_len = 0;
I'd calculate the uname_len here if the option was set.
> - if (msg_len <= MAX_PRINT_CHUNK) {
> + if (msg_len <= MAX_PRINT_CHUNK &&
> + !IS_ENABLED(CONFIG_NETCONSOLE_UNAME)) {
And then try to fold the path with uname into this. So that we don't
have to separate exit points for the "message is short enough".
> netpoll_send_udp(&nt->np, msg, msg_len);
> return;
> }
> @@ -788,14 +791,31 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
> body_len = msg_len - header_len - 1;
> body++;
>
> + if (IS_ENABLED(CONFIG_NETCONSOLE_UNAME)) {
> + /* Add uname at the beginning of buffer */
> + char *uname = init_utsname()->release;
nit: const
> + /* uname_len contains the length of uname + ',' */
> + uname_len = strlen(uname) + 1;
> +
> + if (uname_len + msg_len < MAX_PRINT_CHUNK) {
> + /* No fragmentation needed */
> + scnprintf(buf, MAX_PRINT_CHUNK, "%s,%s", uname, msg);
> + netpoll_send_udp(&nt->np, buf, uname_len + msg_len);
> + return;
> + }
> +
> + /* The data will be fragment, prepending uname */
> + scnprintf(buf, MAX_PRINT_CHUNK, "%s,", uname);
> + }
> +
> /*
> * Transfer multiple chunks with the following extra header.
> * "ncfrag=<byte-offset>/<total-bytes>"
> */
> - memcpy(buf, header, header_len);
> + memcpy(buf + uname_len, header, header_len);
And once done prepping I'd add uname_len to header_len
> while (offset < body_len) {
> - int this_header = header_len;
> + int this_header = header_len + uname_len;
Last but not least, I do agree with Stephen that this can be
configurable with sysfs at runtime, no need to make it a Kconfig.