Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

[PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer

 

 

First page Previous page 1 2 3 4 Next page Last page  View All Linux kernel RSS feed   Index | Next | Previous | View Threaded


kay at vrfy

May 9, 2012, 6:29 AM

Post #26 of 100 (239 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Wed, May 9, 2012 at 9:07 AM, Ingo Molnar <mingo [at] kernel> wrote:

> Thanks for bringing sanity into printk,

Just to check all the options we have, and I have no good idea how
they would work out. It's just an idea, without giving much thought
about possible side effects:

Can we somehow store the PID of the printk() thread that has left the
un-terminated line behind in the buffer, and flush it out when the
next printk() is from a differnt PID? That could prevent the mangling
of "atomic" printk()s by continuation users, and merging unrelated
continuation users together.

Would it make sense to make the printk line buffer per CPU, so they
are not shared between threads, and continuation could work more
reliable?

Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


joe at perches

May 9, 2012, 6:50 AM

Post #27 of 100 (247 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Wed, 2012-05-09 at 11:38 +0200, Kay Sievers wrote:
> On Wed, May 9, 2012 at 5:52 AM, Linus Torvalds
> <torvalds [at] linux-foundation> wrote:
> > On Tue, May 8, 2012 at 4:14 AM, Kay Sievers <kay [at] vrfy> wrote:
> >>
> >> Yeah, we need to make sure, we never merge the (always racy)
> >> continuation printk() users with (non-racy) non-continuation users.
> >> Therefore KERN_CONT is required to suppress the newline and to merge the
> >> content with the earlier non-newline-terminated printk() line.
> >
> > Why?
>
> The idea was: Prefixes are not used that often, but not using a prefix
> should not expose the user to wrongly get appended to an earlier
> non-terminated line of another thread.
>
> The point was to limit the "risk" of wrong merges to users of
> continuation, and not to users which send ordinary "atomic" lines.

I think your premise is wrong for a couple of reasons.

1: printk content is not guaranteed to be stable.

printk content should not be guaranteed to be stable.
It'd make it difficult to simply add a new message
or extend or correct an existing one.

2: There are _thousands_ of printks without prefix levels.

KERN_CONT was always kind of a half-stupid idea.
It's only real purpose is to designate message
continuations but it never gave enough information to
correctly coalesce the messages.

I think you need to give up on the idea that printk
output can be made to be machine readable.

If you really want to have users get complete and
not-interleaved messages you need to buffer partial
messages, Whether or not printk message coalescing
needs to have a cookie or can be done well enough by
using call tree information as suggested by Andrew
Morton is tbd.

Changing printk semantics and trying to stabilize
printk message content are bad ideas though.

I think printk_emit is an OK idea. Having a
per-subsystem notification mechanism, something
like an expanded ethtool is certainly useful.

Making the new additional content of printk_emit
discoverable would be useful.

Converting log_buf from circular to record oriented
and adding another binary header/descriptor to it
is a good idea too.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


kay at vrfy

May 9, 2012, 7:37 AM

Post #28 of 100 (245 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Wed, May 9, 2012 at 3:50 PM, Joe Perches <joe [at] perches> wrote:
> On Wed, 2012-05-09 at 11:38 +0200, Kay Sievers wrote:

> I think your premise is wrong for a couple of reasons.
>
> 1: printk content is not guaranteed to be stable.

Nobody talks about the content of the strings.

>   printk content should not be guaranteed to be stable.
>   It'd make it difficult to simply add a new message
>   or extend or correct an existing one.

Again, that's only what you make out of it.

My point is reliable log _storage_ and with that properly working, the
output _format_ gets as _reliable_ as possible. Continuation printk()
today is entirely unreliable, not only for the continuation users,
also for everybody else, because we merge randomly with atomic
printk() users.

We have (stupid ) scripts that diff dmesg after
supposed-to-be-harmless kernel changes to automatically spot problems.
You can see all the broken merges of continuation lines, and they also
affect lines which do simple and proper atomic printk()s. This is not
useful, and again it's the broken _logic_ of storage not the _content_
of the message.

> 2: There are _thousands_ of printks without prefix levels.

Right, and nobody is crazy enough to think we want to touch them all.

>   KERN_CONT was always kind of a half-stupid idea.
>   It's only real purpose is to designate message
>   continuations but it never gave enough information to
>   correctly coalesce the messages.

Right, it's the annotation of an exception; not nice, but pretty
well-defined, and can be isolated from the non-exception by software
instead of a human.

> I think you need to give up on the idea that printk
> output can be made to be machine readable.

Nobody talks about that in this very context. It is all about better
_reliability_ of the facility not about _content_.

The structured data, possibly attached to the log message, which
uniquely identifies the kernel device, is surely meant to be machine
readable, and no, I'm absolutely not giving up on that approach.

> If you really want to have users get complete and
> not-interleaved messages you need to buffer partial
> messages,  Whether or not printk message coalescing
> needs to have a cookie or can be done well enough by
> using call tree information as suggested by Andrew
> Morton is tbd.

I'm convinced, that we need something simpler than a new cookie logic.
It can work sure, but I doubt we really wants to change the code flow
of all the stuff.

> Changing printk semantics and trying to stabilize
> printk message content are bad ideas though.

Again, nobody talks about content of here.

But reliable context-aware facilities are almost never a bad idea. We
can certainly make printk() better without changing the whole source
tree.

You might not oppose to the heuristics and the "throw it all in a bag
an let's find out later" strategy, I certainly don't like it and think
it's very broken.

> I think printk_emit is an OK idea.  Having a
> per-subsystem notification mechanism, something
> like an expanded ethtool is certainly useful.
>
> Making the new additional content of printk_emit
> discoverable would be useful.

It is all available through /dev/kmsg,

Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


yinghai at kernel

May 9, 2012, 4:02 PM

Post #29 of 100 (244 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Wed, May 9, 2012 at 7:37 AM, Kay Sievers <kay [at] vrfy> wrote:
>
>> Changing printk semantics and trying to stabilize
>> printk message content are bad ideas though.
>
> Again, nobody talks about content of here.

printk_time=1 does not work anymore with this patch on serial console.

but dmesg still have that time printout.

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


greg at kroah

May 9, 2012, 4:06 PM

Post #30 of 100 (243 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Wed, May 09, 2012 at 04:02:40PM -0700, Yinghai Lu wrote:
> On Wed, May 9, 2012 at 7:37 AM, Kay Sievers <kay [at] vrfy> wrote:
> >
> >> Changing printk semantics and trying to stabilize
> >> printk message content are bad ideas though.
> >
> > Again, nobody talks about content of here.
>
> printk_time=1 does not work anymore with this patch on serial console.

Known issue, I think Kay is working on the solution for this right
now...

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


kay at vrfy

May 9, 2012, 5:54 PM

Post #31 of 100 (239 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Wed, 2012-05-09 at 15:29 +0200, Kay Sievers wrote:

> Can we somehow store the PID of the printk() thread that has left the
> un-terminated line behind in the buffer, and flush it out when the
> next printk() is from a differnt PID? That could prevent the mangling
> of "atomic" printk()s by continuation users, and merging unrelated
> continuation users together.

How about this? It relaxes the need for KERN_CONT, but it limits
continuation lines to repeated calls of the same thread.

If things race against each other, the lines are separated and not
wrongly mixed with the data from other users.

I would be happy with that, as it protects the "atomic" users of
printk() from getting mixed up with continuation users.

Thanks,
Kay


From: Kay Sievers <kay [at] vrfy>
Subject: printk() - do not merge continuation lines of different threads

Signed-off-by: Kay Sievers <kay [at] vrfy>---
---

kernel/printk.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)

--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1230,12 +1230,13 @@ asmlinkage int vprintk_emit(int facility
static size_t buflen;
static int buflevel;
static char textbuf[LOG_LINE_MAX];
+ static struct task_struct *cont;
char *text = textbuf;
size_t textlen;
unsigned long flags;
int this_cpu;
bool newline = false;
- bool cont = false;
+ bool prefix = false;
int printed_len = 0;

boot_delay_msec();
@@ -1295,20 +1296,17 @@ asmlinkage int vprintk_emit(int facility
case '0' ... '7':
if (level == -1)
level = text[1] - '0';
- text += 3;
- textlen -= 3;
- break;
case 'c': /* KERN_CONT */
- cont = true;
case 'd': /* KERN_DEFAULT */
+ if (text[1] != 'c')
+ prefix = true;
text += 3;
textlen -= 3;
- break;
}
}

- if (buflen && (!cont || dict)) {
- /* no continuation; flush existing buffer */
+ if (buflen && (prefix || dict || cont != current)) {
+ /* flush existing buffer */
log_store(facility, buflevel, NULL, 0, buf, buflen);
printed_len += buflen;
buflen = 0;
@@ -1342,6 +1340,10 @@ asmlinkage int vprintk_emit(int facility
dict, dictlen, text, textlen);
printed_len += textlen;
}
+ cont = NULL;
+ } else {
+ /* remember thread which filled the buffer */
+ cont = current;
}

/*


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


torvalds at linux-foundation

May 9, 2012, 6:18 PM

Post #32 of 100 (241 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Wed, May 9, 2012 at 5:54 PM, Kay Sievers <kay [at] vrfy> wrote:
>
> How about this? It relaxes the need for KERN_CONT, but it limits
> continuation lines to repeated calls of the same thread.

Fair enough, looks reasonable.

Except your case-statement sucks. Having a "case 'c':" that basically
then immediately has an "if (it was not 'c')" is just disturbing. Just
move the code that is common to all the cases outside the case
statement entirely, and make the 'c' case not do anything at all, and
remove that crazy extraneous if-statement.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


kay at vrfy

May 9, 2012, 7:30 PM

Post #33 of 100 (241 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Wed, 2012-05-09 at 16:06 -0700, Greg Kroah-Hartmann wrote:
> On Wed, May 09, 2012 at 04:02:40PM -0700, Yinghai Lu wrote:
> > On Wed, May 9, 2012 at 7:37 AM, Kay Sievers <kay [at] vrfy> wrote:
> > >
> > >> Changing printk semantics and trying to stabilize
> > >> printk message content are bad ideas though.
> > >
> > > Again, nobody talks about content of here.
> >
> > printk_time=1 does not work anymore with this patch on serial console.
>
> Known issue, I think Kay is working on the solution for this right
> now...

Sure, and this seems to work for me.

Thanks,
Kay


From: Kay Sievers <kay [at] vrfy>
Subject: printk() - restore timestamp printing at console output

The output of the timestamps got lost with the conversion of the
kmsg buffer to records; restore the old behavior.

Document, that CONFIG_PRINTK_TIME now only controls the output of
the timestamps in the syslog() system call and on the console, and
not the recording of the timestamps.

Signed-off-by: Kay Sievers <kay [at] vrfy>
---

kernel/printk.c | 43 ++++++++++++++++++++++++++-----------------
lib/Kconfig.debug | 16 ++++++++++------
2 files changed, 36 insertions(+), 23 deletions(-)

--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -786,6 +786,22 @@ static bool printk_time;
#endif
module_param_named(time, printk_time, bool, S_IRUGO | S_IWUSR);

+static size_t prepend_timestamp(unsigned long long t, char *buf)
+{
+ unsigned long rem_ns;
+
+ if (!printk_time)
+ return 0;
+
+ if (!buf)
+ return 15;
+
+ rem_ns = do_div(t, 1000000000);
+
+ return sprintf(buf, "[%5lu.%06lu] ",
+ (unsigned long) t, rem_ns / 1000);
+}
+
static int syslog_print_line(u32 idx, char *text, size_t size)
{
struct log *msg;
@@ -800,9 +816,7 @@ static int syslog_print_line(u32 idx, ch
len++;
if (msg->level > 99)
len++;
-
- if (printk_time)
- len += 15;
+ len += prepend_timestamp(0, NULL);

len += msg->text_len;
len++;
@@ -810,15 +824,7 @@ static int syslog_print_line(u32 idx, ch
}

len = sprintf(text, "<%u>", msg->level);
-
- if (printk_time) {
- unsigned long long t = msg->ts_nsec;
- unsigned long rem_ns = do_div(t, 1000000000);
-
- len += sprintf(text + len, "[%5lu.%06lu] ",
- (unsigned long) t, rem_ns / 1000);
- }
-
+ len += prepend_timestamp(msg->ts_nsec, text + len);
if (len + msg->text_len > size)
return -EINVAL;
memcpy(text + len, log_text(msg), msg->text_len);
@@ -1741,7 +1747,7 @@ again:
for (;;) {
struct log *msg;
static char text[LOG_LINE_MAX];
- size_t len;
+ size_t len, l;
int level;

raw_spin_lock_irqsave(&logbuf_lock, flags);
@@ -1761,10 +1767,13 @@ again:

msg = log_from_idx(console_idx);
level = msg->level & 7;
- len = msg->text_len;
- if (len+1 >= sizeof(text))
- len = sizeof(text)-1;
- memcpy(text, log_text(msg), len);
+
+ len = prepend_timestamp(msg->ts_nsec, text);
+ l = msg->text_len;
+ if (len + l + 1 >= sizeof(text))
+ l = sizeof(text) - len - 1;
+ memcpy(text + len, log_text(msg), l);
+ len += l;
text[len++] = '\n';

console_idx = log_next(console_idx);
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -3,12 +3,16 @@ config PRINTK_TIME
bool "Show timing information on printks"
depends on PRINTK
help
- Selecting this option causes timing information to be
- included in printk output. This allows you to measure
- the interval between kernel operations, including bootup
- operations. This is useful for identifying long delays
- in kernel startup. Or add printk.time=1 at boot-time.
- See Documentation/kernel-parameters.txt
+ Selecting this option causes time stamps of the printk()
+ messages to be added to the output of the syslog() system
+ call and at the console.
+
+ The timestamp is always recorded internally, and exported
+ to /dev/kmsg. This flag just specifies if the timestamp should
+ be included, not that the timestamp is recorded.
+
+ The behavior is also controlled by the kernel command line
+ parameter printk.time=1. See Documentation/kernel-parameters.txt

config DEFAULT_MESSAGE_LOGLEVEL
int "Default message log level (1-7)"


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


kay at vrfy

May 9, 2012, 7:32 PM

Post #34 of 100 (241 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Wed, 2012-05-09 at 18:18 -0700, Linus Torvalds wrote:
> On Wed, May 9, 2012 at 5:54 PM, Kay Sievers <kay [at] vrfy> wrote:
> >
> > How about this? It relaxes the need for KERN_CONT, but it limits
> > continuation lines to repeated calls of the same thread.
>
> Fair enough, looks reasonable.
>
> Except your case-statement sucks. Having a "case 'c':" that basically
> then immediately has an "if (it was not 'c')" is just disturbing. Just
> move the code that is common to all the cases outside the case
> statement entirely, and make the 'c' case not do anything at all, and
> remove that crazy extraneous if-statement.


From: Kay Sievers <kay [at] vrfy>
Subject: printk() - do not merge continuation lines of different threads

This prevents the merging of printk() continuation lines of different
threads, in the case they race against each other.

It should properly isolate "atomic" single-line printk() users from
continuation users, to make sure the single-line users will never be
merged with the racy continuation ones.

Signed-off-by: Kay Sievers <kay [at] vrfy>
---

kernel/printk.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)

--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1230,12 +1230,13 @@ asmlinkage int vprintk_emit(int facility
static size_t buflen;
static int buflevel;
static char textbuf[LOG_LINE_MAX];
+ static struct task_struct *cont;
char *text = textbuf;
size_t textlen;
unsigned long flags;
int this_cpu;
bool newline = false;
- bool cont = false;
+ bool prefix = false;
int printed_len = 0;

boot_delay_msec();
@@ -1295,20 +1296,16 @@ asmlinkage int vprintk_emit(int facility
case '0' ... '7':
if (level == -1)
level = text[1] - '0';
- text += 3;
- textlen -= 3;
- break;
- case 'c': /* KERN_CONT */
- cont = true;
case 'd': /* KERN_DEFAULT */
+ prefix = true;
+ case 'c': /* KERN_CONT */
text += 3;
textlen -= 3;
- break;
}
}

- if (buflen && (!cont || dict)) {
- /* no continuation; flush existing buffer */
+ if (buflen && (prefix || dict || cont != current)) {
+ /* flush existing buffer */
log_store(facility, buflevel, NULL, 0, buf, buflen);
printed_len += buflen;
buflen = 0;
@@ -1342,6 +1339,10 @@ asmlinkage int vprintk_emit(int facility
dict, dictlen, text, textlen);
printed_len += textlen;
}
+ cont = NULL;
+ } else {
+ /* remember thread which filled the buffer */
+ cont = current;
}

/*


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


joe at perches

May 9, 2012, 7:46 PM

Post #35 of 100 (240 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Thu, 2012-05-10 at 04:32 +0200, Kay Sievers wrote:
> --- a/kernel/printk.c
[]
> @@ -1295,20 +1296,16 @@ asmlinkage int vprintk_emit(int facility
> case '0' ... '7':
> if (level == -1)
> level = text[1] - '0';
> - text += 3;
> - textlen -= 3;
> - break;
> - case 'c': /* KERN_CONT */
> - cont = true;
> case 'd': /* KERN_DEFAULT */
> + prefix = true;
> + case 'c': /* KERN_CONT */

trivia:

Please add /* fallthrough */ comment lines
so it shows the "missing" breaks are intentional.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


kay at vrfy

May 10, 2012, 9:39 AM

Post #36 of 100 (238 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Wed, 2012-05-09 at 18:18 -0700, Linus Torvalds wrote:
> On Wed, May 9, 2012 at 5:54 PM, Kay Sievers <kay [at] vrfy> wrote:
> >
> > How about this? It relaxes the need for KERN_CONT, but it limits
> > continuation lines to repeated calls of the same thread.
>
> Fair enough, looks reasonable.

Here is something which might make sense, and could become be the most
reliable and fail-tolerant version so far. It is also the least
restrictive one regarding the input format, and takes the same amount of
resources as the current implementation.

We fully isolate continuation users from non-continuation users. If a
continuation user gets interrupted by a an ordinary non-continuation
user, we will no longer touch the buffer of the continuation user, we
just emit the ordinary message.

When the same thread comes back and continues printing, we still append
to the earlier buffer we stored.

The only case where printk() still gets messed up now, is when multiple
threads use continuation at the same time, which is way less likely than
mixing with ordinary users.

We will also never merge two racing continuation users into one line;
the worst thing that can happen now, is that they end split up into more
than the intended single line.

Note: In this version, the KERN_* prefixes have all no further meaning
anymore, besides that they carry the priority, or prevent they the
content of the line to be parsed for a priority.

All the special rules are gone. KERN_CONT is the same as KERN_DEFAULT
now.

Even continuation users could use prefixes now, if they wanted to. We
should be context-aware enough now, with remembering the owner (task) of
the buffered data, that we might be able to relax all the special rules
regarding the prefixes.

Any ideas about that?

Thanks,
Kay



From: Kay Sievers <kay [at] vrfy>
Subject: printk() - fully separate continuation line users from ordinary ones
---

printk.c | 86 ++++++++++++++++++++++++++++++---------------------------------
1 file changed, 41 insertions(+), 45 deletions(-)

--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1232,17 +1232,16 @@ asmlinkage int vprintk_emit(int facility, int level,
const char *fmt, va_list args)
{
static int recursion_bug;
- static char buf[LOG_LINE_MAX];
- static size_t buflen;
- static int buflevel;
+ static char cont_buf[LOG_LINE_MAX];
+ static size_t cont_len;
+ static int cont_level;
+ static struct task_struct *cont_task;
static char textbuf[LOG_LINE_MAX];
- static struct task_struct *cont;
char *text = textbuf;
- size_t textlen;
+ size_t text_len;
unsigned long flags;
int this_cpu;
bool newline = false;
- bool prefix = false;
int printed_len = 0;

boot_delay_msec();
@@ -1288,11 +1287,11 @@ asmlinkage int vprintk_emit(int facility, int level,
* The printf needs to come first; we need the syslog
* prefix which might be passed-in as a parameter.
*/
- textlen = vscnprintf(text, sizeof(textbuf), fmt, args);
+ text_len = vscnprintf(text, sizeof(textbuf), fmt, args);

/* mark and strip a trailing newline */
- if (textlen && text[textlen-1] == '\n') {
- textlen--;
+ if (text_len && text[text_len-1] == '\n') {
+ text_len--;
newline = true;
}

@@ -1303,52 +1302,49 @@ asmlinkage int vprintk_emit(int facility, int level,
if (level == -1)
level = text[1] - '0';
case 'd': /* KERN_DEFAULT */
- prefix = true;
case 'c': /* KERN_CONT */
text += 3;
- textlen -= 3;
+ text_len -= 3;
}
}

- if (buflen && (prefix || dict || cont != current)) {
- /* flush existing buffer */
- log_store(facility, buflevel, NULL, 0, buf, buflen);
- printed_len += buflen;
- buflen = 0;
- }
+ if (level == -1)
+ level = default_message_loglevel;

- if (buflen == 0) {
- /* remember level for first message in the buffer */
- if (level == -1)
- buflevel = default_message_loglevel;
- else
- buflevel = level;
- }
+ if (!newline) {
+ if (cont_len && cont_task != current) {
+ /* flush earlier buffer from different thread */
+ log_store(facility, cont_level, NULL, 0, cont_buf, cont_len);
+ cont_len = 0;
+ }

- if (buflen || !newline) {
- /* append to existing buffer, or buffer until next message */
- if (buflen + textlen > sizeof(buf))
- textlen = sizeof(buf) - buflen;
- memcpy(buf + buflen, text, textlen);
- buflen += textlen;
- }
+ if (!cont_len) {
+ cont_level = level;
+ cont_task = current;
+ }

- if (newline) {
- /* end of line; flush buffer */
- if (buflen) {
- log_store(facility, buflevel,
- dict, dictlen, buf, buflen);
- printed_len += buflen;
- buflen = 0;
+ /* buffer, or append to earlier buffer from same thread */
+ if (cont_len + text_len > sizeof(cont_buf))
+ text_len = sizeof(cont_buf) - cont_len;
+ memcpy(cont_buf + cont_len, text, text_len);
+ cont_len += text_len;
+ } else {
+ if (cont_len && cont_task == current) {
+ /* append to earlier buffer and flush */
+ if (cont_len + text_len > sizeof(cont_buf))
+ text_len = sizeof(cont_buf) - cont_len;
+ memcpy(cont_buf + cont_len, text, text_len);
+ cont_len += text_len;
+ log_store(facility, cont_level,
+ NULL, 0, cont_buf, cont_len);
+ cont_len = 0;
+ cont_task = NULL;
+ printed_len = cont_len;
} else {
- log_store(facility, buflevel,
- dict, dictlen, text, textlen);
- printed_len += textlen;
+ log_store(facility, level,
+ dict, dictlen, text, text_len);
+ printed_len = text_len;
}
- cont = NULL;
- } else {
- /* remember thread which filled the buffer */
- cont = current;
}

/*


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


torvalds at linux-foundation

May 10, 2012, 9:47 AM

Post #37 of 100 (237 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Thu, May 10, 2012 at 9:39 AM, Kay Sievers <kay [at] vrfy> wrote:
>
> All the special rules are gone. KERN_CONT is the same as KERN_DEFAULT
> now.

That's wrong.

Key, KERN_CONT and KERN_DEFAULT really are different and have
fundamentally different semantics. The fact that you think they aren't
shows that you don't understand it.

Your "current" check doesn't change anything.

There are two main and important differences:

- people can avoid using '\n' if they know the next printk will have
a KERN_xxx marker (xxx != CONT).

This is often useful for having loops that print out individual
entries all on the same line - print all of them without the '\n'.

The printk afterwards will automatically start a new line if it has
KERN_DEFAULT.

If you make KERN_CONT and KERN_DEFAULT the same, it is a BUG. Don't do it.

- You don't seem to realize that interrupts are threaded events too,
but they will happen with the same task-struct.

An interrupt that prints out with KERN_DEFAULT had better not act
the same way as KERN_CONT.

So dammit, just stop trying to get rid of KERN_CONT or KERN_DEFAULT.
They are both real, and they are both *different*. If you think they
are the same, you are WRONG.

Don't try to change the rules because you think you are "clever".
You're only making things worse.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


tony.luck at gmail

May 10, 2012, 11:49 AM

Post #38 of 100 (236 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Thu, May 10, 2012 at 9:47 AM, Linus Torvalds
<torvalds [at] linux-foundation> wrote:
> This is often useful for having loops that print out individual
> entries all on the same line - print all of them without the '\n'.

Though it is a pain when people do this and the output from each iteration
of the loop gets interleaved with other printk() output. Perhaps it is OK to
do multiple printk() calls in initialization code where it kernel is
mostly single
threaded. But it should be avoided in "oh dear, something bad happened" bits
of code (in case the badness isn't localized and all the other cpus are spitting
out partial messages too).

-Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


kay at vrfy

May 10, 2012, 12:09 PM

Post #39 of 100 (241 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Thu, 2012-05-10 at 09:47 -0700, Linus Torvalds wrote:
> On Thu, May 10, 2012 at 9:39 AM, Kay Sievers <kay [at] vrfy> wrote:

> An interrupt that prints out with KERN_DEFAULT had better not act
> the same way as KERN_CONT.

Good point, understood, and adapted to the new logic.


Here is a new RFC version. I think it's slightly better than the
currently implemented logic in Greg's tree:

We fully isolate continuation users from non-continuation users. If a
continuation user gets interrupted by an ordinary non-continuation
user, we will not touch the continuation buffer, we just emit the
ordinary message. When the same thread comes back and continues its
printing, we still append to the earlier buffer we stored.

The only case where printk() still gets messed up now, is when multiple
threads (or interrupts) race against each other.

We will also never wrongly merge two racing continuation users into one
line.


Current intended behavior, and general rules:
printk() starting with KERN_* (not KERN_CONT) and ending with '\n' will
*always* logged as single line, *never* be merged with anything else.

printk() not starting with any KERN_* might get merged with an earlier
line buffered by the *same* thread.

printk() not ending with '\n' will be buffered.

Buffered lines will be flushed when a different thread emits a printk()
that needs to be buffered --> race.

Buffered line will be flushed, when the same thread emits a printk()
with a KERN_* (not KERN_CONT) --> newline missing.

Buffered line will be joined, when the same thread emits a printk()
without any KERN_* or with KERN_CONT.

Does that sounds correct? Anything to add or adjust?

Thanks,
Kay

---

printk.c | 97 +++++++++++++++++++++++++++++++++++----------------------------
1 file changed, 54 insertions(+), 43 deletions(-)

--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1232,13 +1232,13 @@ asmlinkage int vprintk_emit(int facility, int level,
const char *fmt, va_list args)
{
static int recursion_bug;
- static char buf[LOG_LINE_MAX];
- static size_t buflen;
- static int buflevel;
+ static char cont_buf[LOG_LINE_MAX];
+ static size_t cont_len;
+ static int cont_level;
+ static struct task_struct *cont_task;
static char textbuf[LOG_LINE_MAX];
- static struct task_struct *cont;
char *text = textbuf;
- size_t textlen;
+ size_t text_len;
unsigned long flags;
int this_cpu;
bool newline = false;
@@ -1288,11 +1288,11 @@ asmlinkage int vprintk_emit(int facility, int level,
* The printf needs to come first; we need the syslog
* prefix which might be passed-in as a parameter.
*/
- textlen = vscnprintf(text, sizeof(textbuf), fmt, args);
+ text_len = vscnprintf(text, sizeof(textbuf), fmt, args);

/* mark and strip a trailing newline */
- if (textlen && text[textlen-1] == '\n') {
- textlen--;
+ if (text_len && text[text_len-1] == '\n') {
+ text_len--;
newline = true;
}

@@ -1306,49 +1306,60 @@ asmlinkage int vprintk_emit(int facility, int level,
prefix = true;
case 'c': /* KERN_CONT */
text += 3;
- textlen -= 3;
+ text_len -= 3;
}
}

- if (buflen && (prefix || dict || cont != current)) {
- /* flush existing buffer */
- log_store(facility, buflevel, NULL, 0, buf, buflen);
- printed_len += buflen;
- buflen = 0;
- }
+ if (level == -1)
+ level = default_message_loglevel;

- if (buflen == 0) {
- /* remember level for first message in the buffer */
- if (level == -1)
- buflevel = default_message_loglevel;
- else
- buflevel = level;
- }
+ if (!newline) {
+ if (cont_len && (prefix || cont_task != current)) {
+ /*
+ * Flush earlier buffer, either from a different
+ * thread, or when we've seen a new prefix.
+ */
+ log_store(facility, cont_level, NULL, 0, cont_buf, cont_len);
+ cont_len = 0;
+ }

- if (buflen || !newline) {
- /* append to existing buffer, or buffer until next message */
- if (buflen + textlen > sizeof(buf))
- textlen = sizeof(buf) - buflen;
- memcpy(buf + buflen, text, textlen);
- buflen += textlen;
- }
+ if (!cont_len) {
+ cont_level = level;
+ cont_task = current;
+ }
+
+ /* buffer/append to earlier buffer from same thread */
+ if (cont_len + text_len > sizeof(cont_buf))
+ text_len = sizeof(cont_buf) - cont_len;
+ memcpy(cont_buf + cont_len, text, text_len);
+ cont_len += text_len;
+ } else {
+ if (cont_len && cont_task == current) {
+ if (prefix) {
+ /*
+ * New prefix in same thread; flush. Either
+ * no earlier newline, or in an interrupt.
+ */
+ log_store(facility, cont_level,
+ NULL, 0, cont_buf, cont_len);
+ cont_len = 0;
+ }

- if (newline) {
- /* end of line; flush buffer */
- if (buflen) {
- log_store(facility, buflevel,
- dict, dictlen, buf, buflen);
- printed_len += buflen;
- buflen = 0;
+ /* append to earlier buffer and flush */
+ if (cont_len + text_len > sizeof(cont_buf))
+ text_len = sizeof(cont_buf) - cont_len;
+ memcpy(cont_buf + cont_len, text, text_len);
+ cont_len += text_len;
+ log_store(facility, cont_level,
+ NULL, 0, cont_buf, cont_len);
+ cont_len = 0;
+ cont_task = NULL;
+ printed_len = cont_len;
} else {
- log_store(facility, buflevel,
- dict, dictlen, text, textlen);
- printed_len += textlen;
+ log_store(facility, level,
+ dict, dictlen, text, text_len);
+ printed_len = text_len;
}
- cont = NULL;
- } else {
- /* remember thread which filled the buffer */
- cont = current;
}

/*


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


tytso at mit

May 10, 2012, 1:14 PM

Post #40 of 100 (236 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Thu, May 10, 2012 at 09:09:46PM +0200, Kay Sievers wrote:
> We fully isolate continuation users from non-continuation users. If a
> continuation user gets interrupted by an ordinary non-continuation
> user, we will not touch the continuation buffer, we just emit the
> ordinary message. When the same thread comes back and continues its
> printing, we still append to the earlier buffer we stored.

It's not necessarily a matter of "thread comes back", although that
situation can happen too. You can get this situation quite simply if
you have two processes in foreground kernel mode on two different
CPU's sending continuation printk's at the same time.

> We will also never wrongly merge two racing continuation users into one
> line.

I'm not sure how you guarantee this? The only way you *could*
guarantee this is if you used a continuation buffer in the task_struct
for foreground kernel code, and a per-CPU continuation buffer for
interrupt code.

> Buffered line will be joined, when the same thread emits a printk()
> without any KERN_* or with KERN_CONT.

Is there any difference in any of the cases in terms of how printk's
that are prefixed with KERN_CONT versus a printk that does not have
any KERN_* prefix? If so, is there value in keeping KERN_CONT?

- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


joe at perches

May 10, 2012, 1:37 PM

Post #41 of 100 (238 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Thu, 2012-05-10 at 16:14 -0400, Ted Ts'o wrote:
> Is there any difference in any of the cases in terms of how printk's
> that are prefixed with KERN_CONT versus a printk that does not have
> any KERN_* prefix? If so, is there value in keeping KERN_CONT?

As far as I know, no.

It is a useful marker to show where prints
are actually continued.

#define KERN_CONT ""

would save a small amount of text.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


kay at vrfy

May 10, 2012, 1:38 PM

Post #42 of 100 (242 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Thu, May 10, 2012 at 10:14 PM, Ted Ts'o <tytso [at] mit> wrote:
> On Thu, May 10, 2012 at 09:09:46PM +0200, Kay Sievers wrote:
>> We fully isolate continuation users from non-continuation users. If a
>> continuation user gets interrupted by an ordinary non-continuation
>> user, we will not touch the continuation buffer, we just emit the
>> ordinary message. When the same thread comes back and continues its
>> printing, we still append to the earlier buffer we stored.
>
> It's not necessarily a matter of "thread comes back", although that
> situation can happen too.  You can get this situation quite simply if
> you have two processes in foreground kernel mode on two different
> CPU's sending continuation printk's at the same time.

The access to printk is fully serialized. If only one thread does
continuation (needs buffering), it will still own the continuation
buffer. We record the "owner" (pointer to the task) of the data.

>> We will also never wrongly merge two racing continuation users into one
>> line.
>
> I'm not sure how you guarantee this?  The only way you *could*
> guarantee this is if you used a continuation buffer in the task_struct
> for foreground kernel code, and a per-CPU continuation buffer for
> interrupt code.

Yeah adding it to struct task would reliably work. :) But as we record
the "owner" of the continuation buffer, we should be able to flush the
content of the earlier buffer, instead of wrongly merging it. The same
struct task and interrupts could go wrong, that's true.

>> Buffered line will be joined, when the same thread emits a printk()
>> without any KERN_* or with KERN_CONT.
>
> Is there any difference in any of the cases in terms of how printk's
> that are prefixed with KERN_CONT versus a printk that does not have
> any KERN_* prefix?  If so, is there value in keeping KERN_CONT?

Yeah, it is. It will instruct to merge with the buffer, but also tell
not the parse the prefix, in case you want to log stuff like "<7>" as
real data and not as a prefix.

Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


kay at vrfy

May 10, 2012, 1:39 PM

Post #43 of 100 (238 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Thu, May 10, 2012 at 10:37 PM, Joe Perches <joe [at] perches> wrote:
> On Thu, 2012-05-10 at 16:14 -0400, Ted Ts'o wrote:
>> Is there any difference in any of the cases in terms of how printk's
>> that are prefixed with KERN_CONT versus a printk that does not have
>> any KERN_* prefix?  If so, is there value in keeping KERN_CONT?
>
> As far as I know, no.
>
> It is a useful marker to show where prints
> are actually continued.
>
> #define KERN_CONT ""
>
> would save a small amount of text.

Nah, we can't do that. We need it to tell "here is your non-prefix to
parse, leave the data behind alone".

Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


joe at perches

May 10, 2012, 1:46 PM

Post #44 of 100 (236 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Thu, 2012-05-10 at 22:39 +0200, Kay Sievers wrote:
> On Thu, May 10, 2012 at 10:37 PM, Joe Perches <joe [at] perches> wrote:
> > On Thu, 2012-05-10 at 16:14 -0400, Ted Ts'o wrote:
> >> Is there any difference in any of the cases in terms of how printk's
> >> that are prefixed with KERN_CONT versus a printk that does not have
> >> any KERN_* prefix? If so, is there value in keeping KERN_CONT?
> >
> > As far as I know, no.
> >
> > It is a useful marker to show where prints
> > are actually continued.
> >
> > #define KERN_CONT ""
> >
> > would save a small amount of text.
>
> Nah, we can't do that. We need it to tell "here is your non-prefix to
> parse, leave the data behind alone".

That's where I think you're still a bit
uncertain how the _current_ printk system
works. Your _new_ printk system should
have identical behavior. Though if you
manage to use the call tree and current to
coalesce complete messages more correctly,
that'd be great.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


torvalds at linux-foundation

May 10, 2012, 1:52 PM

Post #45 of 100 (252 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Thu, May 10, 2012 at 1:46 PM, Joe Perches <joe [at] perches> wrote:
>
> That's where I think you're still a bit
> uncertain how the _current_ printk system
> works.

No, you are. Read my answer from two days ago in this thread.

KERN_CONT is *not* the same as "". Not now, not ever. If you make it
the same, you're broken.

The reason is simple: KERN_CONT "<3>" should print out the string
"<3>". If you make KERN_CONT be "", it will do the wrong thing, and
think that the <3> is a priority marker.

Please people, this is subtle, and current code does things RIGHT. Any
code that changes it to do something else is almost certainly buggy.
The new semantics had better be the same as the old one.

The change to verify that 'current' matches the previous printout is
so far the *only* sane semantic change I've seen in this thread.
Everything else has been pure garbage.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


kay at vrfy

May 10, 2012, 2:01 PM

Post #46 of 100 (240 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Thu, May 10, 2012 at 10:46 PM, Joe Perches <joe [at] perches> wrote:
> On Thu, 2012-05-10 at 22:39 +0200, Kay Sievers wrote:

>> Nah, we can't do that. We need it to tell "here is your non-prefix to
>> parse, leave the data behind alone".
>
> That's where I think you're still a bit
> uncertain how the _current_ printk system
> works.
> Your _new_ printk system should
> have identical behavior.

We must be at least as good as we are, sure.

But the aim is to be *better* not to be *identical*, especially when
things go wrong, and they do go wrong far too often in the current
code. What we have today is really not good enough. We have a lot of
context during logging (like the thread), and we should use it to make
the best out of it _before_ we log the stuff away.

>  Though if you
> manage to use the call tree and current to
> coalesce complete messages more correctly,
> that'd be great.

That's what we try. We just need to get all the details out of the
peoples heads, which are nowhere written down, to make it happen. It's
a bit of a painful process sometimes. :)

The conversion from the "put all bytes in a bag and let's find out
later what happened"-buffer to a real separated record buffer imposed
some changes to the logic, and we need to restore/adapt some useful
rules now, which could't be preserved 1:1. But I'm confident that we
manage to get a better overall picture in the end.

Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


joe at perches

May 10, 2012, 2:11 PM

Post #47 of 100 (244 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Thu, 2012-05-10 at 13:52 -0700, Linus Torvalds wrote:
> On Thu, May 10, 2012 at 1:46 PM, Joe Perches <joe [at] perches> wrote:
> >
> > That's where I think you're still a bit
> > uncertain how the _current_ printk system
> > works.
>
> No, you are. Read my answer from two days ago in this thread.
>
> KERN_CONT is *not* the same as "". Not now, not ever. If you make it
> the same, you're broken.
>
> The reason is simple: KERN_CONT "<3>" should print out the string
> "<3>".

I think it's a distinction without a difference for,
as fas as I know, that's a case that doesn't exist
in the current kernel.

$ grep -rP --include=*.[ch] "\bpr_cont\s*\(\s*\"<" *
$ grep -rP --include=*.[ch] "\bprintk\s*\(\s*KERN_CONT\s*\"<" *
arch/x86/kernel/dumpstack_32.c: printk(KERN_CONT "<%02x> ", c);
arch/x86/kernel/dumpstack_64.c: printk(KERN_CONT "<%02x> ", c);
arch/powerpc/kernel/process.c: printk(KERN_CONT "<%08x> ", instr);
drivers/media/video/tm6000/tm6000-core.c: printk(KERN_CONT "<<< ");
drivers/media/video/cx231xx/cx231xx-core.c: printk(KERN_CONT "<<<");


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


kay at vrfy

May 10, 2012, 2:15 PM

Post #48 of 100 (237 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Thu, May 10, 2012 at 11:11 PM, Joe Perches <joe [at] perches> wrote:
> On Thu, 2012-05-10 at 13:52 -0700, Linus Torvalds wrote:
>> On Thu, May 10, 2012 at 1:46 PM, Joe Perches <joe [at] perches> wrote:
>> >
>> > That's where I think you're still a bit
>> > uncertain how the _current_ printk system
>> > works.
>>
>> No, you are. Read my answer from two days ago in this thread.
>>
>> KERN_CONT is *not* the same as "". Not now, not ever. If you make it
>> the same, you're broken.
>>
>> The reason is simple: KERN_CONT "<3>" should print out the string
>> "<3>".
>
> I think it's a distinction without a difference for,
> as fas as I know, that's a case that doesn't exist
> in the current kernel.
>
> $ grep -rP --include=*.[ch] "\bpr_cont\s*\(\s*\"<" *
> $ grep -rP --include=*.[ch] "\bprintk\s*\(\s*KERN_CONT\s*\"<" *
> arch/x86/kernel/dumpstack_32.c:                         printk(KERN_CONT "<%02x> ", c);
> arch/x86/kernel/dumpstack_64.c:                         printk(KERN_CONT "<%02x> ", c);
> arch/powerpc/kernel/process.c:                          printk(KERN_CONT "<%08x> ", instr);
> drivers/media/video/tm6000/tm6000-core.c:                       printk(KERN_CONT "<<< ");
> drivers/media/video/cx231xx/cx231xx-core.c:                     printk(KERN_CONT "<<<");

It surely does, and we needed to add KERN_CONT in the past because
stuff got eaten by the prefix parser.
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=40c8cefaaf12734327db7199a56e60058d98e7b6
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=13f541c10b30fc6529200d7f9a0073217709622f

Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


torvalds at linux-foundation

May 10, 2012, 2:58 PM

Post #49 of 100 (212 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Thu, May 10, 2012 at 2:11 PM, Joe Perches <joe [at] perches> wrote:
>
> I think it's a distinction without a difference for,
> as fas as I know, that's a case that doesn't exist
> in the current kernel.

So? Even if we hadn't had this bug before (we have), your argument is
utter crap. Even if we weren't to have that patter right *now*, that
doesn't mean that it cannot happen.

Your grep is also not at all exhaustive. What if somebody does

printk("%s.."

how are you going to prove that '%s' doesn't contain a level marker
thing? It's much easier to just say "use a KERN_CONT" (or more
commonly KERN_DEFAULT) to avoid the problem.

We do need an escape model. The current KERN_CONT does that. Stop
arguing for crap.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


joe at perches

May 10, 2012, 5:13 PM

Post #50 of 100 (212 views)
Permalink
Re: [PATCH RESEND 1/3] printk: convert byte-buffer to variable-length record buffer [In reply to]

On Thu, 2012-05-10 at 14:58 -0700, Linus Torvalds wrote:
> On Thu, May 10, 2012 at 2:11 PM, Joe Perches <joe [at] perches> wrote:
> >
> > I think it's a distinction without a difference for,
> > as fas as I know, that's a case that doesn't exist
> > in the current kernel.
>
> So? Even if we hadn't had this bug before (we have),

True. I've fixed the ones you've introduced when you
changed printk semantics in the past.

> your argument is utter crap.

What happened to the pragmatic Linus Torvalds?
Did the dogmatic one replace him again temporarily?

> Even if we weren't to have that patter right *now*, that
> doesn't mean that it cannot happen.

Not what I said.

> Your grep is also not at all exhaustive.

I believe I know the printk subsystem and its uses
as well as anyone. I don't think there is a case
today.

> We do need an escape model.

Or an agreement to not to emit "<[0-7cd]>"
as the first 3 bytes after a newline or as
the start of a new printk. It could just
as easily be an escape is a required leading
non "<" character.

I think that's not onerous.

> The current KERN_CONT does that. Stop
> arguing for crap.

Correctness in all cases can be a good thing.
Simplicity may be better because complexity
is reduced.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

First page Previous page 1 2 3 4 Next page Last page  View All Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.