[PATCH] printk: Alternative approach for inline dumping

From: Petr Mladek
Date: Fri Feb 14 2020 - 10:14:18 EST

Next message: Alex Shi: "[PATCH 1/2] locking/rtmutex: Remove unused rt_mutex_cmpxchg_relaxed()"
Previous message: Jonathan Cameron: "Re: [PATCH v2] iio: trigger: stm32-timer: enable clock when in master mode"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

line_count in struct printk_record looks a bit error prone. It causes
a system crash when people forget to initialize it. It seems better
to read this information via a separate API, for example,
prg_read_valid_info().

record_print_text_inline() is really complicated[*]. It is yet
another variant of the tricky logic used in record_print_text().
It would be great to actually reuse the existing function.

[*] I know that you created it on my request.

Signed-off-by: Petr Mladek <pmladek@xxxxxxxx>
---
kernel/printk/printk.c | 134 +++++++++++++-------------------------
kernel/printk/printk_ringbuffer.c | 55 +++++++++-------
kernel/printk/printk_ringbuffer.h | 7 +-
3 files changed, 84 insertions(+), 112 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 5ad67ff60cd9..6b7d6716b178 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -883,7 +883,6 @@ static int devkmsg_open(struct inode *inode, struct file *file)
user->record.text_buf_size = sizeof(user->text_buf);
user->record.dict_buf = &user->dict_buf[0];
user->record.dict_buf_size = sizeof(user->dict_buf);
- user->record.text_line_count = NULL;

logbuf_lock_irq();
user->seq = prb_first_seq(prb);
@@ -1283,87 +1282,50 @@ static size_t record_print_text(const struct printk_record *r, bool syslog,
return len;
}

-static size_t record_print_text_inline(struct printk_record *r, bool syslog,
- bool time)
+static size_t
+get_record_text_size(struct printk_info *info, unsigned int line_count,
+ bool syslog, bool time)
{
- size_t text_len = r->info->text_len;
- size_t buf_size = r->text_buf_size;
- char *text = r->text_buf;
- char prefix[PREFIX_MAX];
- bool truncated = false;
size_t prefix_len;
- size_t len = 0;

- prefix_len = info_print_prefix(r->info, syslog, time, prefix);
-
- if (!text) {
- /* SYSLOG_ACTION_* buffer size only calculation */
- unsigned int line_count = 1;
-
- if (r->text_line_count)
- line_count = *(r->text_line_count);
- /*
- * Each line will be preceded with a prefix. The intermediate
- * newlines are already within the text, but a final trailing
- * newline will be added.
- */
- return ((prefix_len * line_count) + r->info->text_len + 1);
- }
+ prefix_len = info_print_prefix(info, syslog, time, NULL);

/*
- * Add the prefix for each line by shifting the rest of the text to
- * make room for the prefix. If the buffer is not large enough for all
- * the prefixes, then drop the trailing text and report the largest
- * length that includes full lines with their prefixes.
+ * Each line will be preceded with a prefix. The intermediate
+ * newlines are already within the text, but a final trailing
+ * newline will be added.
*/
- while (text_len) {
- size_t line_len;
- char *next;
-
- next = memchr(text, '\n', text_len);
- if (next) {
- line_len = next - text;
- } else {
- /*
- * If the text has been truncated, assume this line
- * was truncated and do not include this text.
- */
- if (truncated)
- break;
- line_len = text_len;
- }
+ return ((prefix_len * line_count) + info->text_len + 1);
+}

- /*
- * Is there enough buffer available to shift this line
- * (and add a newline at the end)?
- */
- if (len + prefix_len + line_len >= buf_size)
- break;
+static size_t record_print_text_inline(struct printk_record *r, bool syslog,
+ bool time)
+{
+ size_t text_len = r->info->text_len;
+ size_t text_buf_size = r->text_buf_size;
+ struct printk_info *info = r->info;
+ size_t record_len;
+ char *text = r->text_buf;
+ char *text_moved;
+ unsigned int line_count;
+ size_t len = 0;

- /*
- * Is there enough buffer available to shift all remaining
- * text (and add a newline at the end)?
- */
- if (len + prefix_len + text_len >= buf_size) {
- text_len = (buf_size - len) - prefix_len;
- truncated = true;
- }
+ if (!text)
+ return 0;

- memmove(text + prefix_len, text, text_len);
- memcpy(text, prefix, prefix_len);
+ line_count = prb_count_lines(text, text_len);
+ record_len = get_record_text_size(info, line_count, syslog, time);

- text += prefix_len + line_len;
- text_len -= line_len;
+ if (text_buf_size < record_len)
+ return 0;

- if (text_len) {
- text_len--;
- text++;
- } else {
- *text = '\n';
- }
+ /* Make space for timestamps */
+ text_moved = text + (record_len - text_len);
+ memmove(text_moved, text, text_len);

- len += prefix_len + line_len + 1;
- }
+ r->text_buf = text_moved;
+ len = record_print_text(r, syslog, time, text, text_buf_size);
+ r->text_buf = text;

return len;
}
@@ -3167,13 +3129,15 @@ bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog,
goto out;

/* Count text lines instead of reading text? */
- if (!line)
- r.text_line_count = &line_count;
-
- if (!prb_read_valid(prb, dumper->cur_seq, &r))
- goto out;
-
- l = record_print_text_inline(&r, syslog, printk_time);
+ if (!line) {
+ if (!prb_read_valid_info(prb, dumper->cur_seq, &info, &line_count))
+ goto out;
+ l = get_record_text_size(&info, line_count, syslog, printk_time);
+ } else {
+ if (!prb_read_valid(prb, dumper->cur_seq, &r))
+ goto out;
+ l = record_print_text_inline(&r, syslog, printk_time);
+ }

dumper->cur_seq = r.info->seq + 1;
ret = true;
@@ -3241,7 +3205,8 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog,
/* initially, only count text lines */
struct printk_record r = {
.info = &info,
- .text_line_count = &line_count,
+ .text_buf = buf,
+ .text_buf_size = size,
};
unsigned long flags;
u64 seq;
@@ -3267,30 +3232,25 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog,

/* calculate length of entire buffer */
seq = dumper->cur_seq;
- while (prb_read_valid(prb, seq, &r)) {
+ while (prb_read_valid_info(prb, seq, &info, &line_count)) {
if (r.info->seq >= dumper->next_seq)
break;
- l += record_print_text_inline(&r, true, time);
+ l += get_record_text_size(&info, line_count, true, time);
seq = r.info->seq + 1;
}

/* move first record forward until length fits into the buffer */
seq = dumper->cur_seq;
- while (l >= size && prb_read_valid(prb, seq, &r)) {
+ while (l >= size && prb_read_valid_info(prb, seq, &info, &line_count)) {
if (r.info->seq >= dumper->next_seq)
break;
- l -= record_print_text_inline(&r, true, time);
+ l -= get_record_text_size(&info, line_count, true, time);
seq = r.info->seq + 1;
}

/* last message in next interation */
next_seq = seq;

- /* actually read data into the buffer now */
- r.text_buf = buf;
- r.text_buf_size = size;
- r.text_line_count = NULL;
-
l = 0;
while (prb_read_valid(prb, seq, &r)) {
if (r.info->seq >= dumper->next_seq)
diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index 796257f226ee..69976a49f828 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -893,7 +893,6 @@ bool prb_reserve(struct prb_reserved_entry *e, struct printk_ringbuffer *rb,
r->dict_buf_size = 0;

r->info = &d->info;
- r->text_line_count = NULL;

/* Set default values for the sizes. */
d->info.text_len = r->text_buf_size;
@@ -1002,6 +1001,21 @@ static char *get_data(struct prb_data_ring *data_ring,
return &db->data[0];
}

+unsigned long prb_count_lines(char *text, unsigned int text_size)
+{
+ unsigned int line_count;
+ char *next;
+
+ line_count = 1;
+ while ((next = memchr(text, '\n', text_size)) != NULL) {
+ text_size -= (next - text);
+ text = next;
+ line_count++;
+ }
+
+ return line_count;
+}
+
/*
* Given @blk_lpos, copy an expected @len of data into the provided buffer.
* If @line_count is provided, count the number of lines in the data.
@@ -1034,21 +1048,8 @@ static bool copy_data(struct prb_data_ring *data_ring,
}

/* Caller interested in the line count? */
- if (line_count) {
- unsigned long next_size = data_size;
- char *next = data;
-
- *line_count = 0;
-
- while (next_size) {
- (*line_count)++;
- next = memchr(next, '\n', next_size);
- if (!next)
- break;
- next++;
- next_size = data_size - (next - data);
- }
- }
+ if (line_count)
+ *line_count = prb_count_lines(data, data_size);

/* Caller interested in the data content? */
if (!buf || !buf_size)
@@ -1094,7 +1095,7 @@ static int desc_read_committed(struct prb_desc_ring *desc_ring,
* See desc_read_committed() for error return values.
*/
static int prb_read(struct printk_ringbuffer *rb, u64 seq,
- struct printk_record *r)
+ struct printk_record *r, unsigned int *line_count)
{
struct prb_desc_ring *desc_ring = &rb->desc_ring;
struct prb_desc *rdesc = to_desc(desc_ring, seq);
@@ -1121,7 +1122,7 @@ static int prb_read(struct printk_ringbuffer *rb, u64 seq,
/* Copy text data. If it fails, this is a data-less descriptor. */
if (!copy_data(&rb->text_data_ring, &desc.text_blk_lpos,
desc.info.text_len, r->text_buf, r->text_buf_size,
- r->text_line_count)) {
+ line_count)) {
return -ENOENT;
}

@@ -1212,12 +1213,12 @@ EXPORT_SYMBOL(prb_first_seq);
* See the description of prb_read_valid() for details.
*/
bool _prb_read_valid(struct printk_ringbuffer *rb, u64 *seq,
- struct printk_record *r)
+ struct printk_record *r, unsigned int *line_count)
{
u64 tail_seq;
int err;

- while ((err = prb_read(rb, *seq, r))) {
+ while ((err = prb_read(rb, *seq, r, line_count))) {
tail_seq = prb_first_seq(rb);

if (*seq < tail_seq) {
@@ -1264,10 +1265,20 @@ bool _prb_read_valid(struct printk_ringbuffer *rb, u64 *seq,
bool prb_read_valid(struct printk_ringbuffer *rb, u64 seq,
struct printk_record *r)
{
- return _prb_read_valid(rb, &seq, r);
+ return _prb_read_valid(rb, &seq, r, NULL);
}
EXPORT_SYMBOL(prb_read_valid);

+bool prb_read_valid_info(struct printk_ringbuffer *rb, u64 seq,
+ struct printk_info *info, unsigned int *line_count)
+{
+ struct printk_record r = {
+ .info = info,
+ };
+
+ return _prb_read_valid(rb, &seq, &r, line_count);
+}
+
/**
* prb_next_seq() - Get the sequence number after the last available record.
*
@@ -1287,7 +1298,7 @@ u64 prb_next_seq(struct printk_ringbuffer *rb)

do {
/* Search forward from the oldest descriptor. */
- if (!_prb_read_valid(rb, &seq, NULL))
+ if (!_prb_read_valid(rb, &seq, NULL, NULL))
return seq;
seq++;
} while (seq);
diff --git a/kernel/printk/printk_ringbuffer.h b/kernel/printk/printk_ringbuffer.h
index 4dc428427e7f..005b000fdb5b 100644
--- a/kernel/printk/printk_ringbuffer.h
+++ b/kernel/printk/printk_ringbuffer.h
@@ -28,8 +28,6 @@ struct printk_info {
* the reader provides the @info, @text_buf, @dict_buf buffers. On success,
* the struct pointed to by @info will be filled and the char arrays pointed
* to by @text_buf and @dict_buf will be filled with text and dict data.
- * If @text_line_count is provided, the number of lines in @text_buf will
- * be counted.
*/
struct printk_record {
struct printk_info *info;
@@ -37,7 +35,6 @@ struct printk_record {
char *dict_buf;
unsigned int text_buf_size;
unsigned int dict_buf_size;
- unsigned int *text_line_count;
};

/* Specifies the position/span of a data block. */
@@ -288,6 +285,8 @@ struct printk_record name = { \
.dict_buf_size = buf_size, \
}

+unsigned long prb_count_lines(char *text, unsigned int text_size);
+
/* Writer Interface */

bool prb_reserve(struct prb_reserved_entry *e, struct printk_ringbuffer *rb,
@@ -304,6 +303,8 @@ unsigned int prb_record_text_space(struct prb_reserved_entry *e);

bool prb_read_valid(struct printk_ringbuffer *rb, u64 seq,
struct printk_record *r);
+bool prb_read_valid_info(struct printk_ringbuffer *rb, u64 seq,
+ struct printk_info *info, unsigned int *line_count);

u64 prb_first_seq(struct printk_ringbuffer *rb);
u64 prb_next_seq(struct printk_ringbuffer *rb);
--
2.16.4

Next message: Alex Shi: "[PATCH 1/2] locking/rtmutex: Remove unused rt_mutex_cmpxchg_relaxed()"
Previous message: Jonathan Cameron: "Re: [PATCH v2] iio: trigger: stm32-timer: enable clock when in master mode"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]