pr_cat() + CATSTR(name, size)?

Hey Joe,

what do you think of this?

It would make composing continuation lines at the caller side entirely race-free, and it might fit into the usual pattern.

The more interesting thing, this would allow us to completely race-free use the dev_printk() calls with continuation content, which we should avoid otherwise for integrity reasons.

Hey Joe,

what do you think this?

It would make composing continuation lines at the caller side entirely race-free, and it might fit into the usual pattern.

The more interesting thing, this would allow us to completely race-free use the dev_printk() calls with continuation content, which we should avoid otherwise for integrity reasons.

The patch below is just hacked it into the printk.c file to illustrate the idea. It prints:

  [    0.000000] Kernel command line: root=/dev/sda2 ...
  [    0.000000] 12 34 56 78
  [    0.000000] PID hash table entries: 2048 (order: 2, 16384 bytes)

5,96,0;Kernel command line: root=/dev/sda2 ... 4,97,0;12 34 56 78
6,98,0;PID hash table entries: 2048 (order: 2, 16384 bytes)

Thanks,
Kay

kernel/printk.c +26 -0

index dba1821..1fd00b0 100644
@@ -48,6 +48,32 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/printk.h>
 
+#define CATSTR(name, max)		\
+	char name[max];			\
+	size_t _##name_len = 0;		\
+	size_t _##name_max = max;
+
+#define pr_cat(name, fmt, ...)		\
+	_catstr(name, &_##name_len, _##name_max, fmt, ##__VA_ARGS__)
+
+ssize_t _catstr(char *s, size_t *len, size_t size, const char *fmt, ...)
+{
+	va_list args;
+	size_t l = *len;
+	size_t r;
+
+	va_start(args, fmt);
+	r = vsnprintf(s + l, size - l, fmt, args);
+	va_end(args);
+
+	if (r >= size - l)
+		return -EINVAL;
+
+	*len += r;
+	s[*len] = '\0';
+	return r;
+}
+
 /*
  * Architectures can override it:
  */

@@ -668,6 +694,12 @@ void __init setup_log_buf(int early)
 	char *new_log_buf;

On Wed, 2012-07-11 at 12:33 +0200, Kay Sievers wrote:
Interesting idea, perhaps workable, but it has a few defects I can see.

It works for most uses, but it doesn't work for when there are multiple function sites like

void dump_info(struct foo *bar)

{
        if (bar->baz)
                pr_cont("baz...");
}

---

pr_info("Some initiator: ")
dump_info(&descriptor);

Another negative is that is uses a local stack variable for the entire line which increases stack pressure.

It also fails to immediately output after some defect unlike your change to output directly to console.

It would require all sites with continuation lines be modified. Because it requires in-situ code modifications, I'd prefer a cookie based approach.

I think it's more flexible, allows the cookie to be passed into extending functions and doesn't demand (much) extra stack.

Something like:
https://lkml.org/lkml/2012/4/3/231
https://lkml.org/lkml/2011/11/14/349

On Wed, Jul 11, 2012 at 5:01 PM, Joe Perches <[email protected]> wrote:

There are _many_ cases the console lock is held, and we don't print stuff immediately out to the console, and we never ensured that in the past. There was never a guarantee that stuff ended up on the console, kmsg was always and needs to be a store+forward model.

Quoted 3 lines

It would require all sites with continuation lines be modified. Because it requires in-situ code modifications, I'd prefer a cookie based approach.

Well, it would be mostly for the dev_printk() stuff, which should ideally never be merged with stuff that could go wrong.

Quoted 7 lines

I think it's more flexible, allows the cookie to be passed into extending functions and doesn't demand (much) extra stack.

Something like:
https://lkml.org/lkml/2012/4/3/231
https://lkml.org/lkml/2011/11/14/349

Hmm, how do we manage memory allocations here? We can get around that somehow? It's something the common printk() must really avoid.

Quoted 1 line · Kay Sievers

On Wed, Jul 11, 2012 at 5:01 PM, Joe Perches <[email protected]> wrote:

[]

Quoted 4 lines

There are _many_ cases the console lock is held, and we don't print stuff immediately out to the console, and we never ensured that in the past. There was never a guarantee that stuff ended up on the console, kmsg was always and needs to be a store+forward model.

I'm less concerned with kmsg than you. I think it's more a nicety than anything.

Quoted 6 lines

Quoted 3 lines

It would require all sites with continuation lines be modified. Because it requires in-situ code modifications, I'd prefer a cookie based approach.

Well, it would be mostly for the dev_printk() stuff, which should ideally never be merged with stuff that could go wrong.

Perhaps that's ideal, but not practical. printk continuations are more prevalent.

Quoted 8 lines

Quoted 7 lines

I think it's more flexible, allows the cookie to be passed into extending functions and doesn't demand (much) extra stack.

Something like:
https://lkml.org/lkml/2012/4/3/231
https://lkml.org/lkml/2011/11/14/349

Quoted 2 lines

Hmm, how do we manage memory allocations here? We can get around that somehow? It's something the common printk() must really avoid.

Well, I think the malloc costs are pretty low and could devolve pretty easily when OOM.

Anyway, interesting idea, keep at it, see what comes out of it.

Quoted 2 lines · Joe Perches

Well, I think the malloc costs are pretty low
and could devolve pretty easily when OOM.

We need to avoid allocating memory in situations where we want to printk(), it's just not possible.

That's why all the kmsg/printk can
not really do any plain malloc. All printk memory needs to be static, on the stack or somehow pre-allocated.

Quoted 2 lines

Anyway, interesting idea, keep at it, see what
comes out of it.

Just depends on us, I guess. :)

Quoted 6 lines · Kay Sievers

Quoted 2 lines · Joe Perches

Well, I think the malloc costs are pretty low
and could devolve pretty easily when OOM.

We need to avoid allocating memory in situations where we want to printk(), it's just not possible.

"it's just not possible???" Kay, them's fightin' words. :)

Quoted 3 lines

That's why all the kmsg/printk can
not really do any plain malloc. All printk memory needs to be static, on the stack or somehow pre-allocated.

Maybe, I was planning to play with it after refactoring printk in the next couple releases.

Quoted 4 lines

Quoted 2 lines

Anyway, interesting idea, keep at it, see what
comes out of it.

Just depends on us, I guess. :)

If your solution is just for the dev_<level> messages (ie: with vprintk_emit descriptors), then it's not too ugly.

Did you look at the remaining dev_<level> and printk continuations grep pattern? There really aren't too many to fix up.

Maybe in 3.6. None of them appear particularly urgent.

One trivial style note:

Maybe CATSTR could use a struct and a DECLARE_ macro?

struct printk_continuation_buffer {
        size_t length;
        size_t pos;
        char buf[];
}

It's a pity gcc doesn't allow non-static declarations like:

#define DECLARE_PRINTK_BUF(name, size)          \
struct printk_continuation_buffer name = {      \
        .length = size;                         \
        .pos = 0;                               \
        .buf[size] = {0};                       \

}

So maybe a DECLARE/DESTROY thing could work with the appropriate malloc/free.

Quoted 8 lines · Joe Perches

Quoted 6 lines · Kay Sievers

Quoted 2 lines · Joe Perches

Well, I think the malloc costs are pretty low
and could devolve pretty easily when OOM.

We need to avoid allocating memory in situations where we want to printk(), it's just not possible.

Quoted 1 line

"it's just not possible???" Kay, them's fightin' words. :)

Nah, I meant it. :) It limits the usefulness of these functions. We can not safely allocate memory, or do not get any memory in some situations where we want to use printk(). Hey, it might be used to say printk("out of memory\n").

Quoted 6 lines

Quoted 3 lines

That's why all the kmsg/printk can
not really do any plain malloc. All printk memory needs to be static, on the stack or somehow pre-allocated.

Maybe, I was planning to play with it after refactoring printk in the next couple releases.

Sounds good.

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

Anyway, interesting idea, keep at it, see what
comes out of it.

Just depends on us, I guess. :)

Quoted 3 lines

If your solution is just for the dev_<level> messages (ie: with vprintk_emit descriptors), then it's not too ugly.

Yeah, I thought only about these. But there might be more users where it makes sense to do that in a more reliable manner, don't know. It was surely no meant to replace the remaining 99.9% of the other cont users. :)

Quoted 3 lines

Did you look at the remaining dev_<level> and printk continuations grep pattern? There really aren't too many to fix up.

Yeah, it looks fine to fix these few.

Quoted 1 line

Maybe in 3.6. None of them appear particularly urgent.

Right.

Quoted 9 lines

One trivial style note:

Maybe CATSTR could use a struct and a DECLARE_ macro?

struct printk_continuation_buffer {
        size_t length;
        size_t pos;
        char buf[];
}

Yeah, but then we lose the simplicity of passing the normal string around, and we need accessor macros to get to the string when we pass it around later. Maybe it's still OK, but it's surely not so intuitive anymore.

Quoted 8 lines

It's a pity gcc doesn't allow non-static declarations like:

#define DECLARE_PRINTK_BUF(name, size)          \
struct printk_continuation_buffer name = {      \
        .length = size;                         \
        .pos = 0;                               \
        .buf[size] = {0};                       \

}

Yeah, when the size changes, we have different type of struct. So we can not name them all "printk_continuation_buffer", every different size would conflict with each other.

Quoted 2 lines

So maybe a DECLARE/DESTROY thing could work with the appropriate malloc/free.

Hmm, I really don't think we can teach the people, or expect them to know, that these printk() functions are fragile if used in some critical code paths.

It would at least need the GFP flags and in many
cases GFP_ATOMIC which can easily fail, and we would also need to do error checking then, and printk() should just never fail, because it is used to tell that something went wrong. We have the entire kmsg buffer pre-allocated at bootup for that reason.

I think the only really sane option here is to use the (usually ~50-100 bytes) stack. Or did you have another idea here which I missed?

Quoted 4 lines · Kay Sievers

Quoted 3 lines

If your solution is just for the dev_<level> messages (ie: with vprintk_emit descriptors), then it's not too ugly.

Quoted 4 lines

Yeah, I thought only about these. But there might be more users where it makes sense to do that in a more reliable manner, don't know. It was surely no meant to replace the remaining 99.9% of the other cont users. :)

I believe your current reassembly code only works on a maximum of 2 interleaved threads. Did that change?

Quoted 3 lines

Yeah, when the size changes, we have different type of struct. So we can not name them all "printk_continuation_buffer", every different size would conflict with each other.

It doesn't work so it doesn't matter no?

Quoted 3 lines

Hmm, I really don't think we can teach the people, or expect them to know, that these printk() functions are fragile if used in some critical code paths.

Vigilance. (and maybe a checkpatch test :).
There just aren't many critical code paths.

Quoted 3 lines

printk() should just never fail, because it
is used to tell that something went wrong. We have the entire kmsg buffer pre-allocated at bootup for that reason.

I still think devolving to direct printks when OOM works as a fallback just fine.