Re: [PATCH] printk: inject caller information into the body of message

[Sergey Senozhatsky]

On (10/01/18 20:21), Tetsuo Handa wrote:
> >> Because there is no guarantee that memory information is dumped under the
> >> oom_lock mutex. The oom_lock is held when calling out_of_memory(), and it
> >> cannot be held when reporting GFP_ATOMIC memory allocation failures.
> > 
> > IOW, static pr_line buffer needs additional synchronization for OOM. Correct?
> 
> Yes (assuming that your OOM refer to both out_of_memory() and warn_alloc()).

Yes, both out_of_memory() and warn_alloc().

> By the way, only up to two threads (the active printer thread and a thread
> which is marked as console_waiter) can stall inside printk(), doesn't it?

Correct.

> Then, can you imagine a situation where 1024 (NR_CPUS) threads are stalling
> inside printk() waiting for flush?

No, not really. Both console_sem owner and waiter should spin outside of
logbuf_lock, so other CPUs can flush/log_store() in the meantime.

> Maybe "struct printk_buffer" after all becomes identical to "struct cont". But
> I guess that even 16 printk_buffer-s is practically sufficient for 1024 CPUs
> system, and allocating NR_CPUS printk_buffer-s will be too wasteful.

NR_CPUS buffers is quite a lot, indeed. Maybe we can do something like
NR_CPUS/4 + 1, etc. Kconfig option will be super hard to get right for
distributions. If people who wrote the code didn't agree on the correct
number of buffers and passed it to the distributions, then it's a good
sign than distributions will have problems picking up the good number as
well.

I'm not experienced enough, and need more opinions here.


I have sketched a very silly, quick-and-dirty implementation using
struct cont. It derives all the good features of the existing pr_cont.
I didn't spend enough time on this. It's just a sketch... which compiles
and that's it.

> I'm saying that I don't like discarding overflowed part because you are
> using seq_buf_vprintf() which just marks "overflowed" rather than
> "flush incomplete line" and "store the new data".
[..]
> Your DEFINE_PR_LINE() is limiting to far smaller than LOG_LINE_MAX.

Yes, you are right, and I was wrong about it (like I said in my email
elsewhere).

The existing cont support is "special", apparently. And it does automatic
flushing and split cont line in separate records when the cont buffer is
about of overflow. So the following loop can pr_cont() more than 1024 bytes
in total:

	for (......)
		pr_cont(.....);
	pr_cont("\n");

Thus new API should have exactly the same characteristics/guarantees. Agreed.

	-ss