| Message ID | 1405947761-26004-4-git-send-email-elder@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 07/21/2014 08:02 AM, Alex Elder wrote: > Two global variables, "syslog_prev" and "console_prev", maintain a > copy of the flags value used in the log record most recently > formatted for syslog or the console, respectively. > > Initially there is no previous formatted log record, and these > variables simply have an initial value 0. And occasionally log > records can get consumed at a rate such that syslog or the console > can't keep up, in which case those variables (along with their > corresponding position variables) must be reset. Here too, they're > reset to 0. > > This patch changes it so the initial value used is LOG_NEWLINE. > That is, if we know nothing about the prevously-formatted log > record, we can assume it was complete, and ended with a newline. > One exception is that occasionally we reset our syslog or console > (etc.) position variables. In that case the previously-formatted > record flags value is still valid, so we preserve that information. > > This is being done to support the next patch. Initializing > these variables this way makes LOG_NEWLINE and LOG_CONT be > mutually exclusive, and the next patch uses that fact to simplify > some logic. > > Signed-off-by: Alex Elder <elder@linaro.org> > Reviewed-by: Petr Mládek <pmladek@suse.cz> I have one change I'd like to suggest on this one. Petr, could you offer your opinion? > --- > kernel/printk/printk.c | 44 ++++++++++++++++++++++++++++++-------------- > 1 file changed, 30 insertions(+), 14 deletions(-) > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 9f11eab..2f43116 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c . . . > @@ -2156,10 +2168,14 @@ again: > "%s** %u printk messages dropped **\n", > (console_prev & LOG_CONT) ? "\n" : "", > (unsigned)(log_first_seq - console_seq)); > - /* messages are gone, move to first one */ > + /* > + * Messages are gone, move to first one. > + * Don't discard what we know about the > + * previously-formatted record. > + */ > console_seq = log_first_seq; > console_idx = log_first_idx; > - console_prev = 0; > + console_prev &= LOG_NEWLINE|LOG_CONT; In this one spot, I think console_prev should simply be initialized with LOG_NEWLINE. The reason is that the "printk messages dropped" message will be inserted into the formatted output, and is hence the last formatted line. And that message is (now) terminated with a newline. -Alex > } else { > len = 0; > } . . . -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tue 2014-07-22 06:48:22, Alex Elder wrote: > On 07/21/2014 08:02 AM, Alex Elder wrote: > > Two global variables, "syslog_prev" and "console_prev", maintain a > > copy of the flags value used in the log record most recently > > formatted for syslog or the console, respectively. > > > > Initially there is no previous formatted log record, and these > > variables simply have an initial value 0. And occasionally log > > records can get consumed at a rate such that syslog or the console > > can't keep up, in which case those variables (along with their > > corresponding position variables) must be reset. Here too, they're > > reset to 0. > > > > This patch changes it so the initial value used is LOG_NEWLINE. > > That is, if we know nothing about the prevously-formatted log > > record, we can assume it was complete, and ended with a newline. > > One exception is that occasionally we reset our syslog or console > > (etc.) position variables. In that case the previously-formatted > > record flags value is still valid, so we preserve that information. > > > > This is being done to support the next patch. Initializing > > these variables this way makes LOG_NEWLINE and LOG_CONT be > > mutually exclusive, and the next patch uses that fact to simplify > > some logic. > > > > Signed-off-by: Alex Elder <elder@linaro.org> > > Reviewed-by: Petr Mládek <pmladek@suse.cz> > > I have one change I'd like to suggest on this one. > > Petr, could you offer your opinion? > > > > --- > > kernel/printk/printk.c | 44 ++++++++++++++++++++++++++++++-------------- > > 1 file changed, 30 insertions(+), 14 deletions(-) > > > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > > index 9f11eab..2f43116 100644 > > --- a/kernel/printk/printk.c > > +++ b/kernel/printk/printk.c > > > . . . > > > @@ -2156,10 +2168,14 @@ again: > > "%s** %u printk messages dropped **\n", > > (console_prev & LOG_CONT) ? "\n" : "", > > (unsigned)(log_first_seq - console_seq)); > > - /* messages are gone, move to first one */ > > + /* > > + * Messages are gone, move to first one. > > + * Don't discard what we know about the > > + * previously-formatted record. > > + */ > > console_seq = log_first_seq; > > console_idx = log_first_idx; > > - console_prev = 0; > > + console_prev &= LOG_NEWLINE|LOG_CONT; > > In this one spot, I think console_prev should simply be > initialized with LOG_NEWLINE. > > The reason is that the "printk messages dropped" message will > be inserted into the formatted output, and is hence the last > formatted line. And that message is (now) terminated with > a newline. Sure. Great catch! I actually mentioned it when commenting v4, see https://lkml.org/lkml/2014/7/21/153 . But I forgot it today :-/ Best Regards, Petr -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 9f11eab..2f43116 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -236,7 +236,7 @@ DECLARE_WAIT_QUEUE_HEAD(log_wait); /* the next printk record to read by syslog(READ) or /proc/kmsg */ static u64 syslog_seq; static u32 syslog_idx; -static enum log_flags syslog_prev; +static enum log_flags syslog_prev = LOG_NEWLINE; static size_t syslog_partial; /* index and sequence number of the first record stored in the buffer */ @@ -250,7 +250,7 @@ static u32 log_next_idx; /* the next printk record to write to the console */ static u64 console_seq; static u32 console_idx; -static enum log_flags console_prev; +static enum log_flags console_prev = LOG_NEWLINE; /* the next printk record to read after the last 'clear' command */ static u64 clear_seq; @@ -1078,10 +1078,14 @@ static int syslog_print(char __user *buf, int size) raw_spin_lock_irq(&logbuf_lock); if (syslog_seq < log_first_seq) { - /* messages are gone, move to first one */ + /* + * Messages are gone, move to first one. + * Don't discard what we know about the + * previously-formatted record. + */ syslog_seq = log_first_seq; syslog_idx = log_first_idx; - syslog_prev = 0; + syslog_prev &= LOG_NEWLINE|LOG_CONT; syslog_partial = 0; } if (syslog_seq == log_next_seq) { @@ -1154,7 +1158,7 @@ static int syslog_print_all(char __user *buf, int size, bool clear) */ seq = clear_seq; idx = clear_idx; - prev = 0; + prev = LOG_NEWLINE; while (seq < log_next_seq) { struct printk_log *msg = log_from_idx(idx); @@ -1167,7 +1171,7 @@ static int syslog_print_all(char __user *buf, int size, bool clear) /* move first record forward until length fits into the buffer */ seq = clear_seq; idx = clear_idx; - prev = 0; + prev = LOG_NEWLINE; while (len > size && seq < log_next_seq) { struct printk_log *msg = log_from_idx(idx); @@ -1203,10 +1207,14 @@ static int syslog_print_all(char __user *buf, int size, bool clear) raw_spin_lock_irq(&logbuf_lock); if (seq < log_first_seq) { - /* messages are gone, move to next one */ + /* + * Messages are gone, move to first one. + * Don't discard what we know about the + * previously-formatted record. + */ seq = log_first_seq; idx = log_first_idx; - prev = 0; + prev &= LOG_NEWLINE|LOG_CONT; } } } @@ -1308,10 +1316,14 @@ int do_syslog(int type, char __user *buf, int len, bool from_file) case SYSLOG_ACTION_SIZE_UNREAD: raw_spin_lock_irq(&logbuf_lock); if (syslog_seq < log_first_seq) { - /* messages are gone, move to first one */ + /* + * Messages are gone, move to first one. + * Don't discard what we know about the + * previously-formatted record. + */ syslog_seq = log_first_seq; syslog_idx = log_first_idx; - syslog_prev = 0; + syslog_prev &= LOG_NEWLINE|LOG_CONT; syslog_partial = 0; } if (from_file) { @@ -2156,10 +2168,14 @@ again: "%s** %u printk messages dropped **\n", (console_prev & LOG_CONT) ? "\n" : "", (unsigned)(log_first_seq - console_seq)); - /* messages are gone, move to first one */ + /* + * Messages are gone, move to first one. + * Don't discard what we know about the + * previously-formatted record. + */ console_seq = log_first_seq; console_idx = log_first_idx; - console_prev = 0; + console_prev &= LOG_NEWLINE|LOG_CONT; } else { len = 0; } @@ -2873,7 +2889,7 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, /* calculate length of entire buffer */ seq = dumper->cur_seq; idx = dumper->cur_idx; - prev = 0; + prev = LOG_NEWLINE; while (seq < dumper->next_seq) { struct printk_log *msg = log_from_idx(idx); @@ -2886,7 +2902,7 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog, /* move first record forward until length fits into the buffer */ seq = dumper->cur_seq; idx = dumper->cur_idx; - prev = 0; + prev = LOG_NEWLINE; while (l > size && seq < dumper->next_seq) { struct printk_log *msg = log_from_idx(idx);