| Message ID | 20230703154155.3460313-1-leitao@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netconsole: Append kernel version to message | expand |
> > Signed-off-by: Breno Leitao <leitao@debian.org> > Cc: Dave Jones <davej@codemonkey.org.uk> 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 leitao@debian.org 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 leitao@debian.org 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.
Hello Stephen, On Mon, Jul 03, 2023 at 11:34:10AM -0700, Stephen Hemminger wrote: > On Mon, 3 Jul 2023 08:41:54 -0700 > leitao@debian.org 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 leitao@debian.org 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 <mpm@selenic.com>"); 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,
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 <leitao@debian.org> 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 > > leitao@debian.org 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 02:18:03 -0700 Breno Leitao <leitao@debian.org> 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
On Wed, 5 Jul 2023 08:26:04 -0700 Stephen Hemminger wrote: > On Wed, 5 Jul 2023 02:18:03 -0700 > Breno Leitao <leitao@debian.org> 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 <mpm@selenic.com>"); > 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.
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 <mpm@selenic.com>"); 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); }