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:

quoted

quoted

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.

quoted

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.