Re: [PATCH v3 3/3] printk: use the lockless ringbuffer
在 2020年07月03日 19:54, John Ogness 写道: > On 2020-07-02, lijiang wrote: >> About the VMCOREINFO part, I made some tests based on the kernel patch >> v3, the makedumpfile and crash-utility can work as expected with your >> patch(userspace patch), but, unfortunately, the >> vmcore-dmesg(kexec-tools) can't correctly read the printk ring buffer >> information, and get the following error: >> >> "Missing the log_buf symbol" >> >> The kexec-tools(vmcore-dmesg) should also have a similar patch, just like >> in the makedumpfile and crash-utility. > > A patched kexec-tools is available here [0]. > > I did not test using 32-bit dumps on 64-bit machines and vice versa. But > it should work. > > John Ogness > > [0] https://github.com/Linutronix/kexec-tools.git (printk branch) > After applying this patch, the vmcore-dmesg can work. Thank you, John Ogness.
Re: [PATCH v3 3/3] printk: use the lockless ringbuffer
在 2020年07月02日 21:31, Petr Mladek 写道: > On Thu 2020-07-02 17:43:22, lijiang wrote: >> 在 2020年07月02日 17:02, John Ogness 写道: >>> On 2020-07-02, lijiang wrote: About the VMCOREINFO part, I made some tests based on the kernel patch v3, the makedumpfile and crash-utility can work as expected with your patch(userspace patch), but, unfortunately, the vmcore-dmesg(kexec-tools) can't correctly read the printk ring buffer information, and get the following error: "Missing the log_buf symbol" The kexec-tools(vmcore-dmesg) should also have a similar patch, just like in the makedumpfile and crash-utility. >>> >>> Yes, a patch for this is needed (as well as for any other related >>> software floating around the internet). >>> >>> I have no RFC patches for vmcore-dmesg. Looking at the code, I think it >>> would be quite straight forward to port the makedumpfile patch. I will >> >> Yes, it should be a similar patch. >> >>> try to make some time for this. >>> >> That would be nice. Thank you, John Ogness. >> >>> I do not want to patch any other software for this. I think with 3 >>> examples (crash, makedumpfile, vmcore-dmesg), others should be able to >> >> It's good enough to have the patch for the makedumpfile, crash and >> vmcore-dmesg, >> which can ensure the kdump(userspace) work well. > > I agree that this three are the most important ones and should be > enough. > > Thanks a lot for working on it and testing it. > My pleasure. I will test the vmcore-dmesg later. Thanks. Lianbo > Best Regards, > Petr >
Re: [PATCH v3 3/3] printk: use the lockless ringbuffer
On 2020-07-02, lijiang wrote: > About the VMCOREINFO part, I made some tests based on the kernel patch > v3, the makedumpfile and crash-utility can work as expected with your > patch(userspace patch), but, unfortunately, the > vmcore-dmesg(kexec-tools) can't correctly read the printk ring buffer > information, and get the following error: > > "Missing the log_buf symbol" > > The kexec-tools(vmcore-dmesg) should also have a similar patch, just like > in the makedumpfile and crash-utility. A patched kexec-tools is available here [0]. I did not test using 32-bit dumps on 64-bit machines and vice versa. But it should work. John Ogness [0] https://github.com/Linutronix/kexec-tools.git (printk branch)
Re: [PATCH v3 3/3] printk: use the lockless ringbuffer
On Thu 2020-07-02 17:43:22, lijiang wrote: > 在 2020年07月02日 17:02, John Ogness 写道: > > On 2020-07-02, lijiang wrote: > >> About the VMCOREINFO part, I made some tests based on the kernel patch > >> v3, the makedumpfile and crash-utility can work as expected with your > >> patch(userspace patch), but, unfortunately, the vmcore-dmesg(kexec-tools) > >> can't correctly read the printk ring buffer information, and get the > >> following error: > >> > >> "Missing the log_buf symbol" > >> > >> The kexec-tools(vmcore-dmesg) should also have a similar patch, just like > >> in the makedumpfile and crash-utility. > > > > Yes, a patch for this is needed (as well as for any other related > > software floating around the internet). > > > > I have no RFC patches for vmcore-dmesg. Looking at the code, I think it > > would be quite straight forward to port the makedumpfile patch. I will > > Yes, it should be a similar patch. > > > try to make some time for this. > > > That would be nice. Thank you, John Ogness. > > > I do not want to patch any other software for this. I think with 3 > > examples (crash, makedumpfile, vmcore-dmesg), others should be able to > > It's good enough to have the patch for the makedumpfile, crash and > vmcore-dmesg, > which can ensure the kdump(userspace) work well. I agree that this three are the most important ones and should be enough. Thanks a lot for working on it and testing it. Best Regards, Petr
Re: buffer allocation: was: [PATCH v3 3/3] printk: use the lockless ringbuffer
On Mon 2020-06-29 23:57:59, John Ogness wrote: > On 2020-06-29, Petr Mladek wrote: > >> @@ @@ void __init setup_log_buf(int early) > >> + prb_init(_rb_dynamic, > >> + new_log_buf, order_base_2(new_log_buf_len), > >> + new_dict_buf, order_base_2(new_log_buf_len), > >> + new_descs, order_base_2(new_descs_count)); > > > > order_base_2() is safe. But the result might be tat some allocated > > space is not used. > > > > I would prefer to make sure that new_log_buf_len is rounded, e.g. > > by roundup_pow_of_two(), at the beginning of the function. Then we > > could use ilog2() here. > > new_log_buf_len can only be set within log_buf_len_update(), and it > is already doing exactly what you want: > > if (size) > size = roundup_pow_of_two(size); > if (size > log_buf_len) > new_log_buf_len = (unsigned long)size; I know. I though about repeating this in setup_log_bug() to make it error proof. But it is not really needed. I do not resist on it. > I can switch to ilog2() instead of the more conservative order_base_2(). Yes, please change it to ilog2(). Best Regards, Petr
Re: [PATCH v3 3/3] printk: use the lockless ringbuffer
在 2020年07月02日 17:02, John Ogness 写道: > On 2020-07-02, lijiang wrote: >> About the VMCOREINFO part, I made some tests based on the kernel patch >> v3, the makedumpfile and crash-utility can work as expected with your >> patch(userspace patch), but, unfortunately, the vmcore-dmesg(kexec-tools) >> can't correctly read the printk ring buffer information, and get the >> following error: >> >> "Missing the log_buf symbol" >> >> The kexec-tools(vmcore-dmesg) should also have a similar patch, just like >> in the makedumpfile and crash-utility. > > Yes, a patch for this is needed (as well as for any other related > software floating around the internet). > > I have no RFC patches for vmcore-dmesg. Looking at the code, I think it > would be quite straight forward to port the makedumpfile patch. I will Yes, it should be a similar patch. > try to make some time for this. > That would be nice. Thank you, John Ogness. > I do not want to patch any other software for this. I think with 3 > examples (crash, makedumpfile, vmcore-dmesg), others should be able to It's good enough to have the patch for the makedumpfile, crash and vmcore-dmesg, which can ensure the kdump(userspace) work well. Thanks. Lianbo > implement the changes to their software without needing my help. > > John Ogness >
Re: [PATCH v3 3/3] printk: use the lockless ringbuffer
On 2020-07-02, lijiang wrote: > About the VMCOREINFO part, I made some tests based on the kernel patch > v3, the makedumpfile and crash-utility can work as expected with your > patch(userspace patch), but, unfortunately, the vmcore-dmesg(kexec-tools) > can't correctly read the printk ring buffer information, and get the > following error: > > "Missing the log_buf symbol" > > The kexec-tools(vmcore-dmesg) should also have a similar patch, just like > in the makedumpfile and crash-utility. Yes, a patch for this is needed (as well as for any other related software floating around the internet). I have no RFC patches for vmcore-dmesg. Looking at the code, I think it would be quite straight forward to port the makedumpfile patch. I will try to make some time for this. I do not want to patch any other software for this. I think with 3 examples (crash, makedumpfile, vmcore-dmesg), others should be able to implement the changes to their software without needing my help. John Ogness
Re: [PATCH v3 3/3] printk: use the lockless ringbuffer
Hi, John Ogness
About the VMCOREINFO part, I made some tests based on the kernel patch
v3, the makedumpfile and crash-utility can work as expected with your
patch(userspace patch), but, unfortunately, the vmcore-dmesg(kexec-tools)
can't correctly read the printk ring buffer information, and get the
following error:
"Missing the log_buf symbol"
The kexec-tools(vmcore-dmesg) should also have a similar patch, just like
in the makedumpfile and crash-utility.
BTW: If you already have a patch for the kexec-tools, would you mind sharing
it? I will make a test for the vmcore-dmesg.
Thanks.
Lianbo
在 2020年06月18日 22:49, John Ogness 写道:
> Replace the existing ringbuffer usage and implementation with
> lockless ringbuffer usage. Even though the new ringbuffer does not
> require locking, all existing locking is left in place. Therefore,
> this change is purely replacing the underlining ringbuffer.
>
> Changes that exist due to the ringbuffer replacement:
>
> - The VMCOREINFO has been updated for the new structures.
>
> - Dictionary data is now stored in a separate data buffer from the
> human-readable messages. The dictionary data buffer is set to the
> same size as the message buffer. Therefore, the total required
> memory for both dictionary and message data is
> 2 * (2 ^ CONFIG_LOG_BUF_SHIFT) for the initial static buffers and
> 2 * log_buf_len (the kernel parameter) for the dynamic buffers.
>
> - Record meta-data is now stored in a separate array of descriptors.
> This is an additional 72 * (2 ^ (CONFIG_LOG_BUF_SHIFT - 5)) bytes
> for the static array and 72 * (log_buf_len >> 5) bytes for the
> dynamic array.
>
> Signed-off-by: John Ogness
> ---
> include/linux/kmsg_dump.h | 2 -
> kernel/printk/printk.c| 944 --
> 2 files changed, 497 insertions(+), 449 deletions(-)
>
> diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
> index 3378bcbe585e..c9b0abe5ca91 100644
> --- a/include/linux/kmsg_dump.h
> +++ b/include/linux/kmsg_dump.h
> @@ -45,8 +45,6 @@ struct kmsg_dumper {
> bool registered;
>
> /* private state of the kmsg iterator */
> - u32 cur_idx;
> - u32 next_idx;
> u64 cur_seq;
> u64 next_seq;
> };
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 8c14835be46c..7642ef634956 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -55,6 +55,7 @@
> #define CREATE_TRACE_POINTS
> #include
>
> +#include "printk_ringbuffer.h"
> #include "console_cmdline.h"
> #include "braille.h"
> #include "internal.h"
> @@ -294,30 +295,24 @@ enum con_msg_format_flags {
> static int console_msg_format = MSG_FORMAT_DEFAULT;
>
> /*
> - * The printk log buffer consists of a chain of concatenated variable
> - * length records. Every record starts with a record header, containing
> - * the overall length of the record.
> + * The printk log buffer consists of a sequenced collection of records, each
> + * containing variable length message and dictionary text. Every record
> + * also contains its own meta-data (@info).
> *
> - * The heads to the first and last entry in the buffer, as well as the
> - * sequence numbers of these entries are maintained when messages are
> - * stored.
> + * Every record meta-data carries the timestamp in microseconds, as well as
> + * the standard userspace syslog level and syslog facility. The usual kernel
> + * messages use LOG_KERN; userspace-injected messages always carry a matching
> + * syslog facility, by default LOG_USER. The origin of every message can be
> + * reliably determined that way.
> *
> - * If the heads indicate available messages, the length in the header
> - * tells the start next message. A length == 0 for the next message
> - * indicates a wrap-around to the beginning of the buffer.
> + * The human readable log message of a record is available in @text, the
> + * length of the message text in @text_len. The stored message is not
> + * terminated.
> *
> - * Every record carries the monotonic timestamp in microseconds, as well as
> - * the standard userspace syslog level and syslog facility. The usual
> - * kernel messages use LOG_KERN; userspace-injected messages always carry
> - * a matching syslog facility, by default LOG_USER. The origin of every
> - * message can be reliably determined that way.
> - *
> - * The human readable log message directly follows the message header. The
> - * length of the message text is stored in the header, the stored message
> - * is not terminated.
> - *
> - * Optionally, a message can carry a dictionary of properties (key/value
> pairs),
> - * to provide userspace with a machine-readable message context.
> + * Optionally, a record can carry a dictionary of properties (key/value
> + * pairs), to provide userspace with a machine-readable message context. The
> + * length of the dictionary is available in @dict_len. The dictionary is not
> + * terminated.
> *
> * Examples for
Re: pending output optimization: was: [PATCH v3 3/3] printk: use the lockless ringbuffer
On 2020-06-25, Petr Mladek wrote:
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -2009,9 +2056,9 @@ asmlinkage int vprintk_emit(int facility, int level,
>>
>> /* This stops the holder of console_sem just where we want him */
>> logbuf_lock_irqsave(flags);
>> -curr_log_seq = log_next_seq;
>> +pending_output = !prb_read_valid(prb, console_seq, NULL);
>> printed_len = vprintk_store(facility, level, dict, dictlen, fmt, args);
>> -pending_output = (curr_log_seq != log_next_seq);
>> +pending_output &= prb_read_valid(prb, console_seq, NULL);
>
> This will stop working after we remove the locks. Consoles will be
> able to handle messages while the new one is being added. There will
> be no gurantee that someone is still hadling the previously pending
> output.
>
> Please, always handle consoles when printed_len is not zero!!!
>
> The pending output was just an optimization added recently. Nobody
> requested it. It was just an idea that made sense.
OK. I will insert a patch before this one that reverts commit
3ac37a93fa92 ("printk: lock/unlock console only for new logbuf
entries"). Then there is no @pending_output for me to implement and it
will be clear that this series is changing/reverting some printk
behavior.
John Ogness
Re: buffer allocation: was: [PATCH v3 3/3] printk: use the lockless ringbuffer
On 2020-06-29, Petr Mladek wrote: >> @@ @@ void __init setup_log_buf(int early) >> +prb_init(_rb_dynamic, >> + new_log_buf, order_base_2(new_log_buf_len), >> + new_dict_buf, order_base_2(new_log_buf_len), >> + new_descs, order_base_2(new_descs_count)); > > order_base_2() is safe. But the result might be tat some allocated > space is not used. > > I would prefer to make sure that new_log_buf_len is rounded, e.g. > by roundup_pow_of_two(), at the beginning of the function. Then we > could use ilog2() here. new_log_buf_len can only be set within log_buf_len_update(), and it is already doing exactly what you want: if (size) size = roundup_pow_of_two(size); if (size > log_buf_len) new_log_buf_len = (unsigned long)size; I can switch to ilog2() instead of the more conservative order_base_2(). John Ogness
Re: syslog size unread: was: [PATCH v3 3/3] printk: use the lockless ringbuffer
On 2020-06-25, Petr Mladek wrote:
> On Thu 2020-06-18 16:55:19, John Ogness wrote:
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -1609,11 +1633,15 @@ int do_syslog(int type, char __user *buf, int len,
>> int source)
>> break;
>> /* Number of chars in the log buffer */
>> case SYSLOG_ACTION_SIZE_UNREAD:
>> +if (source != SYSLOG_FROM_PROC) {
>> +text = kmalloc(LOG_LINE_MAX + PREFIX_MAX, GFP_KERNEL);
>> +if (!text)
>> +return -ENOMEM;
>
> The buffer is needed only to count lines to count the size of added
> prefixes. Could we use the new prb_read_valid_info() that allows to
> get the number of lines without actually reading the buffer?
Yes!
For the next version I introduce a macro to iterate just the meta data:
#define prb_for_each_info(from, rb, s, i, lc) \
for ((s) = from; prb_read_valid_info(rb, s, i, lc); (s) = (i)->seq + 1)
This can be used in all 3 locations where prb_count_lines() is used. In
all three places, there is no need to be copying the data. And for
SYSLOG_ACTION_SIZE_UNREAD, there is no need for the record and kmalloc'd
buffer.
This also means that prb_count_lines() will become a private static
helper of the ringbuffer.
John Ogness
Re: buffer allocation: was: [PATCH v3 3/3] printk: use the lockless ringbuffer
On Fri 2020-06-26 17:02:48, John Ogness wrote:
> On 2020-06-25, Petr Mladek wrote:
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> + free = __LOG_BUF_LEN;
> >> + prb_for_each_record(0, _rb_static, seq, )
> >> + free -= add_to_rb(_rb_dynamic, );
> >> +
> >> + prb = _rb_dynamic;
> >
> > This might deserve a comment that this is safe (no lost message)
> > because it is called early enough when everything is still running
> > on the boot CPU.
>
> I will add a comment and an extra check to make sure.
>
> Once everything is lockless, new messages could appear (for example, due
> to NMI messages). The simple check should probably change to a loop. But
> let us not worry about that at this point.
Yup.
> Below is a new version of the relevant patch snippets against mainline
> just to show you how I plan to make it look for the next version.
>
> John Ogness
>
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ @@
> +#define PRB_AVGBITS 5/* 32 character average length */
> +
> +#if CONFIG_LOG_BUF_SHIFT <= PRB_AVGBITS
> +#error CONFIG_LOG_BUF_SHIFT value too small.
> +#endif
> +_DEFINE_PRINTKRB(printk_rb_static, CONFIG_LOG_BUF_SHIFT - PRB_AVGBITS,
> + PRB_AVGBITS, PRB_AVGBITS, &__log_buf[0]);
> +
> @@ @@
> void __init setup_log_buf(int early)
> {
> + unsigned int new_descs_count;
> + struct prb_desc *new_descs;
> + struct printk_info info;
> + struct printk_record r;
> + size_t new_descs_size;
> unsigned long flags;
> + char *new_dict_buf;
> char *new_log_buf;
> unsigned int free;
> + u64 seq;
>
> /*
>* Some archs call setup_log_buf() multiple times - first is very
> @@ @@ void __init setup_log_buf(int early)
> if (!new_log_buf_len)
> return;
>
> + new_descs_count = new_log_buf_len >> PRB_AVGBITS;
> + if (new_descs_count == 0) {
> + pr_err("new_log_buf_len: %lu too small\n", new_log_buf_len);
> + return;
> + }
> +
> new_log_buf = memblock_alloc(new_log_buf_len, LOG_ALIGN);
> if (unlikely(!new_log_buf)) {
> - pr_err("log_buf_len: %lu bytes not available\n",
> - new_log_buf_len);
> + pr_err("log_buf_len: %lu text bytes not available\n",
> +new_log_buf_len);
> + return;
> + }
> +
> + new_dict_buf = memblock_alloc(new_log_buf_len, LOG_ALIGN);
> + if (unlikely(!new_dict_buf)) {
> + pr_err("log_buf_len: %lu dict bytes not available\n",
> +new_log_buf_len);
> + memblock_free(__pa(new_log_buf), new_log_buf_len);
> return;
> }
>
> + new_descs_size = new_descs_count * sizeof(struct prb_desc);
> + new_descs = memblock_alloc(new_descs_size, LOG_ALIGN);
> + if (unlikely(!new_descs)) {
> + pr_err("log_buf_len: %lu desc bytes not available\n",
> +new_descs_size);
> + memblock_free(__pa(new_dict_buf), new_log_buf_len);
> + memblock_free(__pa(new_log_buf), new_log_buf_len);
> + return;
> + }
> +
> + prb_rec_init_rd(, ,
> + _text_buf[0], sizeof(setup_text_buf),
> + _dict_buf[0], sizeof(setup_dict_buf));
> +
> + prb_init(_rb_dynamic,
> + new_log_buf, order_base_2(new_log_buf_len),
> + new_dict_buf, order_base_2(new_log_buf_len),
> + new_descs, order_base_2(new_descs_count));
order_base_2() is safe. But the result might be tat some allocated
space is not used.
I would prefer to make sure that new_log_buf_len is rounded, e.g.
by roundup_pow_of_two(), at the beginning of the function. Then we
could use ilog2() here.
Otherwise, it looks fine to me.
Best Regards,
Petr
Re: record_printk_text tricks: was: [PATCH v3 3/3] printk: use the lockless ringbuffer
On 2020-06-25, Petr Mladek wrote:
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -1344,72 +1304,150 @@ static size_t print_prefix(const struct printk_log
>> *msg, bool syslog,
>> return len;
>> }
>>
>> -static size_t msg_print_text(const struct printk_log *msg, bool syslog,
>> - bool time, char *buf, size_t size)
>> +static size_t record_print_text(struct printk_record *r, bool syslog,
>> +bool time)
>> {
>> -const char *text = log_text(msg);
>> -size_t text_size = msg->text_len;
>> -size_t len = 0;
>> +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];
>> -const size_t prefix_len = print_prefix(msg, syslog, time, prefix);
>> +bool truncated = false;
>> +size_t prefix_len;
>> +size_t line_len;
>> +size_t len = 0;
>> +char *next;
>>
>> -do {
>> -const char *next = memchr(text, '\n', text_size);
>> -size_t text_len;
>> +prefix_len = info_print_prefix(r->info, syslog, time, prefix);
>>
>> +/*
>> + * Add the prefix for each line by shifting the rest of the text to
>> + * make room for each 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.
>
> This should go up as the function description.
>
> Sigh, this function does (already did) many subltle decisions. We
> should mention them in the description. My current understanding is
> the following:
>
> /*
> * Prepare the record for printing. The text is shifted in the given
> * buffer to avoid a need for another one. The following operations
> * are done:
> *
> * - Add prefix for each line.
> * - Add the trailing newline that has been removed in vprintk_store().
> * - Drop truncated lines that do not longer fit into the buffer.
> *
> * Return: The length of the updated text, including the added
> * prefixes, newline. The dropped line(s) are not counted.
> */
OK.
>> + *
>> + * @text_len: bytes of unprocessed text
>> + * @line_len: bytes of current line (newline _not_ included)
>> + * @text: pointer to beginning of current line
>> + * @len: number of bytes processed (size of r->text_buf done)
>
> Please, move this to the variable declaration.
I find it more helpful to see these critical definitions directly above
the loop. When I add them to the variable declarations, I find it really
ugly and hard to reference. Maybe you could show me an example of what
you want it to look like?
>> + */
>> +for (;;) {
>> +next = memchr(text, '\n', text_len);
>> if (next) {
>> -text_len = next - text;
>> -next++;
>> -text_size -= next - text;
>> +line_len = next - text;
>> } else {
>> -text_len = text_size;
>> +/*
>> + * No newline. If the text was previously truncated,
>> + * assume this line was truncated and do not include
>> + * it.
>> + */
>
> The word "assume" is confusing. The last line _must_ have been truncated when
> "truncated" is set. I would write:
>
> /* Drop incomplete truncated lines. */
OK.
>> +if (truncated)
>> +break;
>> +line_len = text_len;
>> }
>>
>> -if (buf) {
>> -if (prefix_len + text_len + 1 >= size - len)
>> +/*
>> + * Ensure there is enough buffer available to shift this line
>> + * (and add a newline at the end).
>> + */
>> +if (len + prefix_len + line_len + 1 > buf_size)
>> +break;
>> +
>> +/*
>> + * Ensure there is enough buffer available to shift all
>> + * remaining text (and add a newline at the end). If this
>> + * test fails, then there must be a newline (i.e.
>> + * text_len > line_len because the previous test succeeded).
>> + */
>> +if (len + prefix_len + text_len + 1 > buf_size) {
>> +/*
>> + * Truncate @text_len so that there is enough space
>> + * for a prefix. A newline will not be added because
>> + * the last line of the text is now truncated and
>> + * will not be included.
>> + */
>> +text_len = (buf_size - len) - prefix_len;
>
> This is confusing. The check requires that one more character gets truncated.
> And it should be perfectly fine to truncate '\n' from the previous
> line. The final '\n' is always
Re: buffer allocation: was: [PATCH v3 3/3] printk: use the lockless ringbuffer
On 2020-06-25, Petr Mladek wrote:
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -1176,11 +1068,46 @@ static void __init set_percpu_data_ready(void)
>> __printk_percpu_data_ready = true;
>> }
>>
>> +static unsigned int __init add_to_rb(struct printk_ringbuffer *rb,
>> + struct printk_record *r)
>> +{
>> +struct prb_reserved_entry e;
>> +struct printk_record dest_r;
>> +
>> +prb_rec_init_wr(_r, r->info->text_len, r->info->dict_len);
>> +
>> +if (!prb_reserve(, rb, _r))
>> +return 0;
>> +
>> +memcpy(_r.text_buf[0], >text_buf[0], dest_r.text_buf_size);
>> +if (dest_r.dict_buf) {
>> +memcpy(_r.dict_buf[0], >dict_buf[0],
>> + dest_r.dict_buf_size);
>> +}
>> +dest_r.info->facility = r->info->facility;
>> +dest_r.info->level = r->info->level;
>> +dest_r.info->flags = r->info->flags;
>> +dest_r.info->ts_nsec = r->info->ts_nsec;
>> +dest_r.info->caller_id = r->info->caller_id;
>> +
>> +prb_commit();
>> +
>> +return prb_record_text_space();
>> +}
>> +
>> +static char setup_text_buf[CONSOLE_EXT_LOG_MAX] __initdata;
>> +static char setup_dict_buf[CONSOLE_EXT_LOG_MAX] __initdata;
>> +
>> void __init setup_log_buf(int early)
>> {
>> +struct prb_desc *new_descs;
>> +struct printk_info info;
>> +struct printk_record r;
>> unsigned long flags;
>> +char *new_dict_buf;
>> char *new_log_buf;
>> unsigned int free;
>> +u64 seq;
>>
>> /*
>> * Some archs call setup_log_buf() multiple times - first is very
>> @@ -1201,17 +1128,50 @@ void __init setup_log_buf(int early)
>>
>> new_log_buf = memblock_alloc(new_log_buf_len, LOG_ALIGN);
>> if (unlikely(!new_log_buf)) {
>> -pr_err("log_buf_len: %lu bytes not available\n",
>> +pr_err("log_buf_len: %lu text bytes not available\n",
>> new_log_buf_len);
>> return;
>> }
>>
>> +new_dict_buf = memblock_alloc(new_log_buf_len, LOG_ALIGN);
>> +if (unlikely(!new_dict_buf)) {
>> +/* dictionary failure is allowed */
>> +pr_err("log_buf_len: %lu dict bytes not available\n",
>> +new_log_buf_len);
>> +}
>> +
>> +new_descs = memblock_alloc((new_log_buf_len >> PRB_AVGBITS) *
>> + sizeof(struct prb_desc), LOG_ALIGN);
>> +if (unlikely(!new_descs)) {
>> +pr_err("log_buf_len: %lu desc bytes not available\n",
>> +new_log_buf_len >> PRB_AVGBITS);
>> +if (new_dict_buf)
>> +memblock_free(__pa(new_dict_buf), new_log_buf_len);
>> +memblock_free(__pa(new_log_buf), new_log_buf_len);
>> +return;
>> +}
>> +
>> +prb_rec_init_rd(, ,
>> +_text_buf[0], sizeof(setup_text_buf),
>> +_dict_buf[0], sizeof(setup_dict_buf));
>> +
>> logbuf_lock_irqsave(flags);
>> +
>> +prb_init(_rb_dynamic,
>> + new_log_buf, bits_per(new_log_buf_len) - 1,
>> + new_dict_buf, bits_per(new_log_buf_len) - 1,
>
> This does not check whether new_dict_buf was really allocated.
Thank you.
> I suggest to make it easy and switch to the new buffers only when
> all three could get allocated.
Agreed.
> Well, I still feel a bit uneasy about these PRB_AVGBITS operations,
> including new_log_buf_len >> PRB_AVGBITS used above.
>
> A more safe design would be to add some sanity checks at the beginning
> of the function. And maybe convert new_log_buf_let to number of bits.
> Then operate with the number of bits in the rest of the function. It
> might be easier to make sure that we are on the safe side.
I will clean it up with additional checks and temporary variables.
>> log_buf_len = new_log_buf_len;
>> log_buf = new_log_buf;
>> new_log_buf_len = 0;
>> -free = __LOG_BUF_LEN - log_next_idx;
>> -memcpy(log_buf, __log_buf, __LOG_BUF_LEN);
>> +
>> +free = __LOG_BUF_LEN;
>> +prb_for_each_record(0, _rb_static, seq, )
>> +free -= add_to_rb(_rb_dynamic, );
>> +
>> +prb = _rb_dynamic;
>
> This might deserve a comment that this is safe (no lost message)
> because it is called early enough when everything is still running
> on the boot CPU.
I will add a comment and an extra check to make sure.
Once everything is lockless, new messages could appear (for example, due
to NMI messages). The simple check should probably change to a loop. But
let us not worry about that at this point.
Below is a new version of the relevant patch snippets against mainline
just to show you how I plan to make it look for the next version.
John Ogness
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ @@
+#define PRB_AVGBITS 5 /* 32 character average length */
+
+#if CONFIG_LOG_BUF_SHIFT <= PRB_AVGBITS
+#error CONFIG_LOG_BUF_SHIFT value too small.
+#endif
Re: truncate dict: was: Re: [PATCH v3 3/3] printk: use the lockless ringbuffer
On 2020-06-25, Petr Mladek wrote:
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -594,22 +473,26 @@ static u32 msg_used_size(u16 text_len, u16 dict_len,
>> u32 *pad_len)
>> #define MAX_LOG_TAKE_PART 4
>> static const char trunc_msg[] = "";
>>
>> -static u32 truncate_msg(u16 *text_len, u16 *trunc_msg_len,
>> -u16 *dict_len, u32 *pad_len)
>> +static void truncate_msg(u16 *text_len, u16 *trunc_msg_len, u16 *dict_len)
>> {
>> /*
>> * The message should not take the whole buffer. Otherwise, it might
>> * get removed too soon.
>> */
>> u32 max_text_len = log_buf_len / MAX_LOG_TAKE_PART;
>> +
>> if (*text_len > max_text_len)
>> *text_len = max_text_len;
>> -/* enable the warning message */
>> +
>> +/* enable the warning message (if there is room) */
>> *trunc_msg_len = strlen(trunc_msg);
>> +if (*text_len >= *trunc_msg_len)
>> +*text_len -= *trunc_msg_len;
>> +else
>> +*trunc_msg_len = 0;
>> +
>> /* disable the "dict" completely */
>> *dict_len = 0;
>
> The dictionary does not longer need to be removed at this point.
> It is stored in a separate buffer. It is ignored by prb_reserve()
> when there is not enough space for it.
Agreed. @dict_len argument will also be dropped from the function.
John Ogness
Re: record_printk_text tricks: was: [PATCH v3 3/3] printk: use the lockless ringbuffer
On Thu 2020-06-25 14:09:46, Petr Mladek wrote:
> On Thu 2020-06-18 16:55:19, John Ogness wrote:
> > Replace the existing ringbuffer usage and implementation with
> > lockless ringbuffer usage. Even though the new ringbuffer does not
> > require locking, all existing locking is left in place. Therefore,
> > this change is purely replacing the underlining ringbuffer.
>
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > +static size_t record_print_text(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)
> > +{
Nit: This should get called get_record_print_text_size(). It will make
it more clear that it counts the prefixes added for printing.
Best Regards,
Petr
PS: I have finished review of this 3rd patch. I am going to look at
the 2nd patch the following week if I find the courage ;-)
syslog size unread: was: [PATCH v3 3/3] printk: use the lockless ringbuffer
On Thu 2020-06-18 16:55:19, John Ogness wrote:
> Replace the existing ringbuffer usage and implementation with
> lockless ringbuffer usage. Even though the new ringbuffer does not
> require locking, all existing locking is left in place. Therefore,
> this change is purely replacing the underlining ringbuffer.
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1609,11 +1633,15 @@ int do_syslog(int type, char __user *buf, int len,
> int source)
> break;
> /* Number of chars in the log buffer */
> case SYSLOG_ACTION_SIZE_UNREAD:
> + if (source != SYSLOG_FROM_PROC) {
> + text = kmalloc(LOG_LINE_MAX + PREFIX_MAX, GFP_KERNEL);
> + if (!text)
> + return -ENOMEM;
The buffer is needed only to count lines to count the size of
added prefixes. Could we use the new prb_read_valid_info() that
allows to get the number of lines without actually reading the buffer?
> +
> + }
> logbuf_lock_irq();
> - if (syslog_seq < log_first_seq) {
> + if (syslog_seq < prb_first_seq(prb)) {
> /* messages are gone, move to first one */
> - syslog_seq = log_first_seq;
> - syslog_idx = log_first_idx;
> + syslog_seq = prb_first_seq(prb);
> syslog_partial = 0;
> }
> if (source == SYSLOG_FROM_PROC) {
Best Regards,
Petr
pending output optimization: was: [PATCH v3 3/3] printk: use the lockless ringbuffer
On Thu 2020-06-18 16:55:19, John Ogness wrote: > Replace the existing ringbuffer usage and implementation with > lockless ringbuffer usage. Even though the new ringbuffer does not > require locking, all existing locking is left in place. Therefore, > this change is purely replacing the underlining ringbuffer. > > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -2009,9 +2056,9 @@ asmlinkage int vprintk_emit(int facility, int level, > > /* This stops the holder of console_sem just where we want him */ > logbuf_lock_irqsave(flags); > - curr_log_seq = log_next_seq; > + pending_output = !prb_read_valid(prb, console_seq, NULL); > printed_len = vprintk_store(facility, level, dict, dictlen, fmt, args); > - pending_output = (curr_log_seq != log_next_seq); > + pending_output &= prb_read_valid(prb, console_seq, NULL); This will stop working after we remove the locks. Consoles will be able to handle messages while the new one is being added. There will be no gurantee that someone is still hadling the previously pending output. Please, always handle consoles when printed_len is not zero!!! The pending output was just an optimization added recently. Nobody requested it. It was just an idea that made sense. This new code is a ticking bomb. It is far from obvious that it _must_ get removed after we remove the lock. And it will be hard to debug why the consoles are sometimes not handled. > logbuf_unlock_irqrestore(flags); > > /* If called from the scheduler, we can not call up(). */ Best Regards, Petr
record_printk_text tricks: was: [PATCH v3 3/3] printk: use the lockless ringbuffer
On Thu 2020-06-18 16:55:19, John Ogness wrote:
> Replace the existing ringbuffer usage and implementation with
> lockless ringbuffer usage. Even though the new ringbuffer does not
> require locking, all existing locking is left in place. Therefore,
> this change is purely replacing the underlining ringbuffer.
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1344,72 +1304,150 @@ static size_t print_prefix(const struct printk_log
> *msg, bool syslog,
> return len;
> }
>
> -static size_t msg_print_text(const struct printk_log *msg, bool syslog,
> - bool time, char *buf, size_t size)
> +static size_t record_print_text(struct printk_record *r, bool syslog,
> + bool time)
> {
> - const char *text = log_text(msg);
> - size_t text_size = msg->text_len;
> - size_t len = 0;
> + 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];
> - const size_t prefix_len = print_prefix(msg, syslog, time, prefix);
> + bool truncated = false;
> + size_t prefix_len;
> + size_t line_len;
> + size_t len = 0;
> + char *next;
>
> - do {
> - const char *next = memchr(text, '\n', text_size);
> - size_t text_len;
> + prefix_len = info_print_prefix(r->info, syslog, time, prefix);
>
> + /*
> + * Add the prefix for each line by shifting the rest of the text to
> + * make room for each 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.
This should go up as the function description.
Sigh, this function does (already did) many subltle decisions. We
should mention them in the description. My current understanding is
the following:
/*
* Prepare the record for printing. The text is shifted in the given
* buffer to avoid a need for another one. The following operations
* are done:
*
* - Add prefix for each line.
* - Add the trailing newline that has been removed in vprintk_store().
* - Drop truncated lines that do not longer fit into the buffer.
*
* Return: The length of the updated text, including the added
* prefixes, newline. The dropped line(s) are not counted.
*/
> + *
> + * @text_len: bytes of unprocessed text
> + * @line_len: bytes of current line (newline _not_ included)
> + * @text: pointer to beginning of current line
> + * @len: number of bytes processed (size of r->text_buf done)
Please, move this to the variable declaration.
> + */
> + for (;;) {
> + next = memchr(text, '\n', text_len);
> if (next) {
> - text_len = next - text;
> - next++;
> - text_size -= next - text;
> + line_len = next - text;
> } else {
> - text_len = text_size;
> + /*
> + * No newline. If the text was previously truncated,
> + * assume this line was truncated and do not include
> + * it.
> + */
The word "assume" is confusing. The last line _must_ have been truncated when
"truncated" is set. I would write:
/* Drop incomplete truncated lines. */
> + if (truncated)
> + break;
> + line_len = text_len;
> }
>
> - if (buf) {
> - if (prefix_len + text_len + 1 >= size - len)
> + /*
> + * Ensure there is enough buffer available to shift this line
> + * (and add a newline at the end).
> + */
> + if (len + prefix_len + line_len + 1 > buf_size)
> + break;
> +
> + /*
> + * Ensure there is enough buffer available to shift all
> + * remaining text (and add a newline at the end). If this
> + * test fails, then there must be a newline (i.e.
> + * text_len > line_len because the previous test succeeded).
> + */
> + if (len + prefix_len + text_len + 1 > buf_size) {
> + /*
> + * Truncate @text_len so that there is enough space
> + * for a prefix. A newline will not be added because
> + * the last line of the text is now truncated and
> + * will not be included.
> + */
> + text_len = (buf_size - len) - prefix_len;
This is confusing. The check requires that one more character gets truncated.
And it should be perfectly fine to truncate '\n' from the previous
line. The final '\n' is always added.
There are more problems with this
buffer allocation: was: [PATCH v3 3/3] printk: use the lockless ringbuffer
On Thu 2020-06-18 16:55:19, John Ogness wrote:
> Replace the existing ringbuffer usage and implementation with
> lockless ringbuffer usage. Even though the new ringbuffer does not
> require locking, all existing locking is left in place. Therefore,
> this change is purely replacing the underlining ringbuffer.
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1176,11 +1068,46 @@ static void __init set_percpu_data_ready(void)
> __printk_percpu_data_ready = true;
> }
>
> +static unsigned int __init add_to_rb(struct printk_ringbuffer *rb,
> + struct printk_record *r)
> +{
> + struct prb_reserved_entry e;
> + struct printk_record dest_r;
> +
> + prb_rec_init_wr(_r, r->info->text_len, r->info->dict_len);
> +
> + if (!prb_reserve(, rb, _r))
> + return 0;
> +
> + memcpy(_r.text_buf[0], >text_buf[0], dest_r.text_buf_size);
> + if (dest_r.dict_buf) {
> + memcpy(_r.dict_buf[0], >dict_buf[0],
> +dest_r.dict_buf_size);
> + }
> + dest_r.info->facility = r->info->facility;
> + dest_r.info->level = r->info->level;
> + dest_r.info->flags = r->info->flags;
> + dest_r.info->ts_nsec = r->info->ts_nsec;
> + dest_r.info->caller_id = r->info->caller_id;
> +
> + prb_commit();
> +
> + return prb_record_text_space();
> +}
> +
> +static char setup_text_buf[CONSOLE_EXT_LOG_MAX] __initdata;
> +static char setup_dict_buf[CONSOLE_EXT_LOG_MAX] __initdata;
> +
> void __init setup_log_buf(int early)
> {
> + struct prb_desc *new_descs;
> + struct printk_info info;
> + struct printk_record r;
> unsigned long flags;
> + char *new_dict_buf;
> char *new_log_buf;
> unsigned int free;
> + u64 seq;
>
> /*
>* Some archs call setup_log_buf() multiple times - first is very
> @@ -1201,17 +1128,50 @@ void __init setup_log_buf(int early)
>
> new_log_buf = memblock_alloc(new_log_buf_len, LOG_ALIGN);
> if (unlikely(!new_log_buf)) {
> - pr_err("log_buf_len: %lu bytes not available\n",
> + pr_err("log_buf_len: %lu text bytes not available\n",
> new_log_buf_len);
> return;
> }
>
> + new_dict_buf = memblock_alloc(new_log_buf_len, LOG_ALIGN);
> + if (unlikely(!new_dict_buf)) {
> + /* dictionary failure is allowed */
> + pr_err("log_buf_len: %lu dict bytes not available\n",
> + new_log_buf_len);
> + }
> +
> + new_descs = memblock_alloc((new_log_buf_len >> PRB_AVGBITS) *
> +sizeof(struct prb_desc), LOG_ALIGN);
> + if (unlikely(!new_descs)) {
> + pr_err("log_buf_len: %lu desc bytes not available\n",
> + new_log_buf_len >> PRB_AVGBITS);
> + if (new_dict_buf)
> + memblock_free(__pa(new_dict_buf), new_log_buf_len);
> + memblock_free(__pa(new_log_buf), new_log_buf_len);
> + return;
> + }
> +
> + prb_rec_init_rd(, ,
> + _text_buf[0], sizeof(setup_text_buf),
> + _dict_buf[0], sizeof(setup_dict_buf));
> +
> logbuf_lock_irqsave(flags);
> +
> + prb_init(_rb_dynamic,
> + new_log_buf, bits_per(new_log_buf_len) - 1,
> + new_dict_buf, bits_per(new_log_buf_len) - 1,
This does not check whether new_dict_buf was really allocated.
We should reuse the static one when it was not allocated. But it might
become tricky. We would need to preserve the sequence
numbers of each copied messages and do not copy the dictionary.
I suggest to make it easy and switch to the new buffers only when
all three could get allocated.
Well, we still should try to preserve the sequence numbers. For this,
we might need to allow storing empty messages instead of the lost ones.
> + new_descs, (bits_per(new_log_buf_len) - 1) - PRB_AVGBITS);
This is likely safe because new_log_buf_len is assigned only when it is
greater than the previous one. As a result bits_per(new_log_buf_len)
- 1) should be greater than PRB_AVGBITS.
Well, I still feel a bit uneasy about these PRB_AVGBITS operations,
including new_log_buf_len >> PRB_AVGBITS used above.
A more safe design would be to add some sanity checks at the beginning of
the function. And maybe convert new_log_buf_let to number of bits.
Then operate with the number of bits in the rest of the function.
It might be easier to make sure that we are on the safe side.
> log_buf_len = new_log_buf_len;
> log_buf = new_log_buf;
> new_log_buf_len = 0;
> - free = __LOG_BUF_LEN - log_next_idx;
> - memcpy(log_buf, __log_buf, __LOG_BUF_LEN);
> +
> + free = __LOG_BUF_LEN;
> + prb_for_each_record(0, _rb_static, seq, )
> + free -= add_to_rb(_rb_dynamic, );
> +
> + prb = _rb_dynamic;
This might deserve a comment that this is safe (no lost message)
because
truncate dict: was: Re: [PATCH v3 3/3] printk: use the lockless ringbuffer
On Thu 2020-06-18 16:55:19, John Ogness wrote:
> Replace the existing ringbuffer usage and implementation with
> lockless ringbuffer usage. Even though the new ringbuffer does not
> require locking, all existing locking is left in place. Therefore,
> this change is purely replacing the underlining ringbuffer.
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -594,22 +473,26 @@ static u32 msg_used_size(u16 text_len, u16 dict_len,
> u32 *pad_len)
> #define MAX_LOG_TAKE_PART 4
> static const char trunc_msg[] = "";
>
> -static u32 truncate_msg(u16 *text_len, u16 *trunc_msg_len,
> - u16 *dict_len, u32 *pad_len)
> +static void truncate_msg(u16 *text_len, u16 *trunc_msg_len, u16 *dict_len)
> {
> /*
>* The message should not take the whole buffer. Otherwise, it might
>* get removed too soon.
>*/
> u32 max_text_len = log_buf_len / MAX_LOG_TAKE_PART;
> +
> if (*text_len > max_text_len)
> *text_len = max_text_len;
> - /* enable the warning message */
> +
> + /* enable the warning message (if there is room) */
> *trunc_msg_len = strlen(trunc_msg);
> + if (*text_len >= *trunc_msg_len)
> + *text_len -= *trunc_msg_len;
> + else
> + *trunc_msg_len = 0;
> +
> /* disable the "dict" completely */
> *dict_len = 0;
The dictionary does not longer need to be removed at this point.
It is stored in a separate buffer. It is ignored by prb_reserve()
when there is not enough space for it.
> - /* compute the size again, count also the warning message */
> - return msg_used_size(*text_len + *trunc_msg_len, 0, pad_len);
> }
>
> /* insert record into the buffer, discard old ones, update heads */
Best Regards,
Petr
PS: I am still in the middle of review of this patch. I decided to
send the tree comments that I already have separately.
Re: [PATCH v3 3/3] printk: use the lockless ringbuffer
Hi John, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linux/master] [also build test WARNING on linus/master v5.8-rc1 next-20200618] [cannot apply to pmladek/for-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/John-Ogness/printk-replace-ringbuffer/20200618-225239 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1b5044021070efa3259f3e9548dc35d1eb6aa844 config: x86_64-randconfig-s021-20200618 (attached as .config) compiler: gcc-9 (Debian 9.3.0-13) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.2-rc1-10-gc17b1b06-dirty # save the attached .config to linux build tree make W=1 C=1 ARCH=x86_64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot sparse warnings: (new ones prefixed by >>) kernel/printk/printk.c:397:1: sparse: sparse: symbol 'log_wait' was not declared. Should it be static? >> kernel/printk/printk.c:437:1: sparse: sparse: symbol >> '_printk_rb_static_dict' was not declared. Should it be static? Please review and possibly fold the followup patch. --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
[PATCH v3 3/3] printk: use the lockless ringbuffer
Replace the existing ringbuffer usage and implementation with
lockless ringbuffer usage. Even though the new ringbuffer does not
require locking, all existing locking is left in place. Therefore,
this change is purely replacing the underlining ringbuffer.
Changes that exist due to the ringbuffer replacement:
- The VMCOREINFO has been updated for the new structures.
- Dictionary data is now stored in a separate data buffer from the
human-readable messages. The dictionary data buffer is set to the
same size as the message buffer. Therefore, the total required
memory for both dictionary and message data is
2 * (2 ^ CONFIG_LOG_BUF_SHIFT) for the initial static buffers and
2 * log_buf_len (the kernel parameter) for the dynamic buffers.
- Record meta-data is now stored in a separate array of descriptors.
This is an additional 72 * (2 ^ (CONFIG_LOG_BUF_SHIFT - 5)) bytes
for the static array and 72 * (log_buf_len >> 5) bytes for the
dynamic array.
Signed-off-by: John Ogness
---
include/linux/kmsg_dump.h | 2 -
kernel/printk/printk.c| 944 --
2 files changed, 497 insertions(+), 449 deletions(-)
diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
index 3378bcbe585e..c9b0abe5ca91 100644
--- a/include/linux/kmsg_dump.h
+++ b/include/linux/kmsg_dump.h
@@ -45,8 +45,6 @@ struct kmsg_dumper {
bool registered;
/* private state of the kmsg iterator */
- u32 cur_idx;
- u32 next_idx;
u64 cur_seq;
u64 next_seq;
};
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8c14835be46c..7642ef634956 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -55,6 +55,7 @@
#define CREATE_TRACE_POINTS
#include
+#include "printk_ringbuffer.h"
#include "console_cmdline.h"
#include "braille.h"
#include "internal.h"
@@ -294,30 +295,24 @@ enum con_msg_format_flags {
static int console_msg_format = MSG_FORMAT_DEFAULT;
/*
- * The printk log buffer consists of a chain of concatenated variable
- * length records. Every record starts with a record header, containing
- * the overall length of the record.
+ * The printk log buffer consists of a sequenced collection of records, each
+ * containing variable length message and dictionary text. Every record
+ * also contains its own meta-data (@info).
*
- * The heads to the first and last entry in the buffer, as well as the
- * sequence numbers of these entries are maintained when messages are
- * stored.
+ * Every record meta-data carries the timestamp in microseconds, as well as
+ * the standard userspace syslog level and syslog facility. The usual kernel
+ * messages use LOG_KERN; userspace-injected messages always carry a matching
+ * syslog facility, by default LOG_USER. The origin of every message can be
+ * reliably determined that way.
*
- * If the heads indicate available messages, the length in the header
- * tells the start next message. A length == 0 for the next message
- * indicates a wrap-around to the beginning of the buffer.
+ * The human readable log message of a record is available in @text, the
+ * length of the message text in @text_len. The stored message is not
+ * terminated.
*
- * Every record carries the monotonic timestamp in microseconds, as well as
- * the standard userspace syslog level and syslog facility. The usual
- * kernel messages use LOG_KERN; userspace-injected messages always carry
- * a matching syslog facility, by default LOG_USER. The origin of every
- * message can be reliably determined that way.
- *
- * The human readable log message directly follows the message header. The
- * length of the message text is stored in the header, the stored message
- * is not terminated.
- *
- * Optionally, a message can carry a dictionary of properties (key/value
pairs),
- * to provide userspace with a machine-readable message context.
+ * Optionally, a record can carry a dictionary of properties (key/value
+ * pairs), to provide userspace with a machine-readable message context. The
+ * length of the dictionary is available in @dict_len. The dictionary is not
+ * terminated.
*
* Examples for well-defined, commonly used property names are:
* DEVICE=b12:8 device identifier
@@ -331,21 +326,19 @@ static int console_msg_format = MSG_FORMAT_DEFAULT;
* follows directly after a '=' character. Every property is terminated by
* a '\0' character. The last property is not terminated.
*
- * Example of a message structure:
- * ff 8f 00 00 00 00 00 00 monotonic time in nsec
- * 0008 34 00record is 52 bytes long
- * 000a0b 00 text is 11 bytes long
- * 000c 1f 00dictionary is 23 bytes long
- * 000e03 00 LOG_KERN (facility) LOG_ERR (level)
- * 0010 69 74 27 73 20 61 20 6c "it's a l"
- * 69 6e 65 "ine"
- * 001b 44 45 56 49 43
