| Message ID | 20250124-netcon_cpu-v3-1-12a0d286ba1d@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netconsole: Add support for CPU population | expand |
On Fri, Jan 24, 2025 at 07:16:40AM -0800, Breno Leitao wrote: > Move the static buffers from send_msg_no_fragmentation() and > send_msg_fragmented() into the netconsole_target structure. This > simplifies the code by: > - Eliminating redundant static buffers > - Centralizing buffer management in the target structure > - Reducing memory usage by 1KB (one buffer instead of two) > > The buffer in netconsole_target is protected by target_list_lock, > maintaining the same synchronization semantics as the original code. > > Suggested-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > drivers/net/netconsole.c | 29 +++++++++++++++-------------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c > index 86ab4a42769a49eebe5dd6f01dafafc6c86ec54f..1a78704681184673f5c1ba8ae665e46751384293 100644 > --- a/drivers/net/netconsole.c > +++ b/drivers/net/netconsole.c > @@ -137,6 +137,8 @@ struct netconsole_target { > bool extended; > bool release; > struct netpoll np; > + /* protected by target_list_lock */ > + char buf[MAX_PRINT_CHUNK]; nit: buf should also be added to the Kernel doc for this structure. ...
On Tue, Jan 28, 2025 at 04:11:28PM +0000, Simon Horman wrote: > On Fri, Jan 24, 2025 at 07:16:40AM -0800, Breno Leitao wrote: > > Move the static buffers from send_msg_no_fragmentation() and > > send_msg_fragmented() into the netconsole_target structure. This > > simplifies the code by: > > - Eliminating redundant static buffers > > - Centralizing buffer management in the target structure > > - Reducing memory usage by 1KB (one buffer instead of two) > > > > The buffer in netconsole_target is protected by target_list_lock, > > maintaining the same synchronization semantics as the original code. > > > > Suggested-by: Jakub Kicinski <kuba@kernel.org> > > Signed-off-by: Breno Leitao <leitao@debian.org> > > --- > > drivers/net/netconsole.c | 29 +++++++++++++++-------------- > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c > > index 86ab4a42769a49eebe5dd6f01dafafc6c86ec54f..1a78704681184673f5c1ba8ae665e46751384293 100644 > > --- a/drivers/net/netconsole.c > > +++ b/drivers/net/netconsole.c > > @@ -137,6 +137,8 @@ struct netconsole_target { > > bool extended; > > bool release; > > struct netpoll np; > > + /* protected by target_list_lock */ > > + char buf[MAX_PRINT_CHUNK]; > > nit: buf should also be added to the Kernel doc for this structure. > > ... Hi Breno, With that fixed feel free to add: Reviewed-by: Simon Horman <horms@kernel.org>
Hello Simon, On Thu, Jan 30, 2025 at 10:35:44AM +0000, Simon Horman wrote: > On Tue, Jan 28, 2025 at 04:11:28PM +0000, Simon Horman wrote: > > On Fri, Jan 24, 2025 at 07:16:40AM -0800, Breno Leitao wrote: > > > Move the static buffers from send_msg_no_fragmentation() and > > > send_msg_fragmented() into the netconsole_target structure. This > > > simplifies the code by: > > > - Eliminating redundant static buffers > > > - Centralizing buffer management in the target structure > > > - Reducing memory usage by 1KB (one buffer instead of two) > > > > > > The buffer in netconsole_target is protected by target_list_lock, > > > maintaining the same synchronization semantics as the original code. > > > > > > Suggested-by: Jakub Kicinski <kuba@kernel.org> > > > Signed-off-by: Breno Leitao <leitao@debian.org> > > > --- > > > drivers/net/netconsole.c | 29 +++++++++++++++-------------- > > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c > > > index 86ab4a42769a49eebe5dd6f01dafafc6c86ec54f..1a78704681184673f5c1ba8ae665e46751384293 100644 > > > --- a/drivers/net/netconsole.c > > > +++ b/drivers/net/netconsole.c > > > @@ -137,6 +137,8 @@ struct netconsole_target { > > > bool extended; > > > bool release; > > > struct netpoll np; > > > + /* protected by target_list_lock */ > > > + char buf[MAX_PRINT_CHUNK]; > > > > nit: buf should also be added to the Kernel doc for this structure. > > > > ... > > Hi Breno, > > With that fixed feel free to add: > > Reviewed-by: Simon Horman <horms@kernel.org> Thanks again for the review. I will update according to your suggestion, and send the new version after the merge window is opened. --breno
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 86ab4a42769a49eebe5dd6f01dafafc6c86ec54f..1a78704681184673f5c1ba8ae665e46751384293 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -137,6 +137,8 @@ struct netconsole_target { bool extended; bool release; struct netpoll np; + /* protected by target_list_lock */ + char buf[MAX_PRINT_CHUNK]; }; #ifdef CONFIG_NETCONSOLE_DYNAMIC @@ -1117,7 +1119,6 @@ static void send_msg_no_fragmentation(struct netconsole_target *nt, int msg_len, int release_len) { - static char buf[MAX_PRINT_CHUNK]; /* protected by target_list_lock */ const char *userdata = NULL; const char *release; @@ -1128,18 +1129,18 @@ static void send_msg_no_fragmentation(struct netconsole_target *nt, if (release_len) { release = init_utsname()->release; - scnprintf(buf, MAX_PRINT_CHUNK, "%s,%s", release, msg); + scnprintf(nt->buf, MAX_PRINT_CHUNK, "%s,%s", release, msg); msg_len += release_len; } else { - memcpy(buf, msg, msg_len); + memcpy(nt->buf, msg, msg_len); } if (userdata) - msg_len += scnprintf(&buf[msg_len], + msg_len += scnprintf(&nt->buf[msg_len], MAX_PRINT_CHUNK - msg_len, "%s", userdata); - send_udp(nt, buf, msg_len); + send_udp(nt, nt->buf, msg_len); } static void append_release(char *buf) @@ -1150,7 +1151,7 @@ static void append_release(char *buf) scnprintf(buf, MAX_PRINT_CHUNK, "%s,", release); } -static void send_fragmented_body(struct netconsole_target *nt, char *buf, +static void send_fragmented_body(struct netconsole_target *nt, const char *msgbody, int header_len, int msgbody_len) { @@ -1181,7 +1182,7 @@ static void send_fragmented_body(struct netconsole_target *nt, char *buf, int this_offset = 0; int this_chunk = 0; - this_header += scnprintf(buf + this_header, + this_header += scnprintf(nt->buf + this_header, MAX_PRINT_CHUNK - this_header, ",ncfrag=%d/%d;", offset, body_len); @@ -1192,7 +1193,8 @@ static void send_fragmented_body(struct netconsole_target *nt, char *buf, MAX_PRINT_CHUNK - this_header); if (WARN_ON_ONCE(this_chunk <= 0)) return; - memcpy(buf + this_header, msgbody + offset, this_chunk); + memcpy(nt->buf + this_header, msgbody + offset, + this_chunk); this_offset += this_chunk; } @@ -1226,13 +1228,13 @@ static void send_fragmented_body(struct netconsole_target *nt, char *buf, */ return; - memcpy(buf + this_header + this_offset, + memcpy(nt->buf + this_header + this_offset, userdata + sent_userdata, this_chunk); this_offset += this_chunk; } - send_udp(nt, buf, this_header + this_offset); + send_udp(nt, nt->buf, this_header + this_offset); offset += this_offset; } } @@ -1242,7 +1244,6 @@ static void send_msg_fragmented(struct netconsole_target *nt, int msg_len, int release_len) { - static char buf[MAX_PRINT_CHUNK]; /* protected by target_list_lock */ int header_len, msgbody_len; const char *msgbody; @@ -1260,16 +1261,16 @@ static void send_msg_fragmented(struct netconsole_target *nt, * "ncfrag=<byte-offset>/<total-bytes>" */ if (release_len) - append_release(buf); + append_release(nt->buf); /* Copy the header into the buffer */ - memcpy(buf + release_len, msg, header_len); + memcpy(nt->buf + release_len, msg, header_len); header_len += release_len; /* for now on, the header will be persisted, and the msgbody * will be replaced */ - send_fragmented_body(nt, buf, msgbody, header_len, msgbody_len); + send_fragmented_body(nt, msgbody, header_len, msgbody_len); } /**
Move the static buffers from send_msg_no_fragmentation() and send_msg_fragmented() into the netconsole_target structure. This simplifies the code by: - Eliminating redundant static buffers - Centralizing buffer management in the target structure - Reducing memory usage by 1KB (one buffer instead of two) The buffer in netconsole_target is protected by target_list_lock, maintaining the same synchronization semantics as the original code. Suggested-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Breno Leitao <leitao@debian.org> --- drivers/net/netconsole.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)