Message-ID: <20250120110653.693fd5ec@kernel.org>
Date: Mon, 20 Jan 2025 11:06:53 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Breno Leitao <leitao@...ian.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
 <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, Jonathan Corbet
 <corbet@....net>, Shuah Khan <shuah@...nel.org>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-kselftest@...r.kernel.org, kernel-team@...a.com, max@...sevol.com,
 thepacketgeek@...il.com
Subject: Re: [PATCH net-next v2 3/5] netconsole: add support for sysdata and
 CPU population

On Mon, 20 Jan 2025 09:30:48 -0800 Breno Leitao wrote:
> > > Not sure I followed. The data ({userdata,extradata}_complete) was always
> > > inside nt field, which belongs to target_list.  
> > 
> > I mean the buffer we use for formatting. Today it's this:
> > 
> > 	static char buf[MAX_PRINT_CHUNK]; /* protected by target_list_lock */
> > 	int header_len, msgbody_len;
> > 	const char *msgbody;
> > 
> > right? I missed that "static" actually so it's not on the stack, 
> > it's in the .bss section.  
> 
> Since you raised this topic, I don't think buf needs to be static
> for a functional perspective, since `buf` is completely overwritten
> every time send_msg functions are called.

It may be because it's relatively big and stack space used to be 
very limited.

> > My thinking was to handle it like the release.
> > Print it at the send_msg_no_fragmentation() stage directly 
> > into the static buffer. Does that get hairy coding-wise?  
> 
> I suppose the advantage of doing this approach is to reduce a
> memcpy/strcpy, right?

Not really, my main motivation is to try to find a common way
of how various pieces of the output are protected and handled.

> If this is what your motivation, I think we cannot remove it from the
> fragmented case. Let me share my thought process:
> 
> 1) sysdata needs to be appended to both send_msg_fragmented() and
> send_msg_no_fragmentation(). The fragmented case is the problem.
> 
> 2) It is trivially done in send_msg_fragmented() case.
> 
> 3) For the send_msg_no_fragmentation() case, there is no trivial way to
> get it done without using a secondary buffer and then memcpy to `buf`.
> 
> Let's suppose sysdata has "cpu=42", and original `buf` has only 5 available
> chars, thus it needs to have 2 msgs to accommodate the full message.
> 
> Then the it needs to track that `cpu=4` will be sent in a msg and create
> another message with the missing `2`.
> 
> The only way to do it properly is having a extra buffer where we
> have `cpu=42` and copy 5 bytes from there, and then copy the last one in
> the next iteration. I am not sure we can do it in one shot.

FWIW to simplify reasoning about the length I thought we could take the
worst case, assume we'll need len(cpu=) + log10(nr_cpu_ids) of space.

> On top of that, I am planning to increase other features in sysdata
> (such as current task name, modules and even consolidate the release as
> sysdata), which has two implications:
> 
> 1) Average messages size will become bigger. Thus, memcpy will be needed
> one way or another.
> 
> 2) Unless we can come up with a smart solution, this solution will be
> harder to reason about.
> 
> If you want to invest more time in this direction, I am more than happy
> to create a PoC, so we can discuss more concretely. 

I don't feel super strongly about this. But hacking around is always
good to get a sense of how hairy the implementation ends up being.

To rephrase my concern is that we have some data as static on the
stack, some dynamically appended at the send_*() stage, now we're
adding a third way of handling things. Perhaps the simplest way to
make me happy would be to move the bufs which are currently static 
into nt.

Powered by blists - more mailing lists