All of lore.kernel.org
 help / color / mirror / Atom feed
From: Petr Mladek <pmladek@suse.com>
To: John Ogness <john.ogness@linutronix.de>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH printk-rework 03/12] printk: consolidate kmsg_dump_get_buffer/syslog_print_all code
Date: Tue, 2 Feb 2021 13:31:04 +0100	[thread overview]
Message-ID: <YBlGCBKgOzf3At/c@alley> (raw)
In-Reply-To: <87pn1kjexp.fsf@jogness.linutronix.de>

On Mon 2021-02-01 10:55:22, John Ogness wrote:
> On 2021-01-29, Petr Mladek <pmladek@suse.com> wrote:
> >> The logic for finding records to fit into a buffer is the same for
> >> kmsg_dump_get_buffer() and syslog_print_all(). Introduce a helper
> >> function find_first_fitting_seq() to handle this logic.
> >> 
> >> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> >> ---
> >>  kernel/printk/printk.c | 71 ++++++++++++++++++++++++------------------
> >>  1 file changed, 41 insertions(+), 30 deletions(-)
> >> 
> >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> >> index 1929aa372e7f..ec2174882b8e 100644
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -1421,6 +1421,41 @@ static size_t get_record_print_text_size(struct printk_info *info,
> >>  	return ((prefix_len * line_count) + info->text_len + 1);
> >>  }
> >>  
> >> +/*
> >> + * Beginning with @start_seq, find the first record where it and all following
> >> + * records up to (but not including) @max_seq fit into @size.
> >
> > Please, mention that all existing messages are checked when @max_seq
> > == -1 is used.
> 
> -1 is not special for that purpose, but I will add a comment that if
>  there is no required upper bound, the caller can use -1.

Thanks. I think that it is good to mention that the function is able
to handle this situation. For example, it will not wait until
a message with @max_seq is available ;-)

> >> + */
> >> +static u64 find_first_fitting_seq(u64 start_seq, u64 max_seq, size_t size,
> >> +				  struct printk_info *info, bool syslog, bool time)
> >> +{

> >> +	/*
> >> +	 * Move first record forward until length fits into the buffer. Ignore
> >> +	 * newest messages that were not counted in the above cycle. Messages
> >> +	 * might appear and get lost in the meantime. This is a best effort
> >> +	 * that prevents an infinite loop that could occur with a retry.
> >> +	 */
> >> +	if (seq < max_seq)
> >> +		max_seq = seq;
> >
> > This made my head twist around several times ;-)
> >
> > It should never be true in kmsg_dump_get_buffer().
> 
> Correct. It is there because of syslog_print_all().
> 
> > And there was nothing like this in the original syslog_print_all().
> 
> With logbuf_lock, it is not possible that new messages arrive in between
> these two loops. But without logbuf_lock, it _is_ possible and this
> needs to be handled.

I see.

> I can expand the commit message to mention this necessary change.

Yes, please. I am fine with the code now.



> >> @@ -3436,26 +3461,12 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog,
> >>  
> >>  	/*
> >>  	 * Find first record that fits, including all following records,
> >> -	 * into the user-provided buffer for this dump.
> >> +	 * into the user-provided buffer for this dump. Pass in size-1
> >> +	 * because this function (by way of record_print_text()) will
> >> +	 * not write more than size-1 bytes of text into @buf.
> >
> > We should do the same also in syslog_print_all(). It must have the
> > same problem. The last message might get lost when there is not
> > a space for the trailing '\0' that was not counted before.
> 
> No, it does not have the same problem. I also made the mistake [0] of
> thinking that.
> 
> copy_to_user() is the function filling the buffer, not
> record_print_text(). And it will (and always has) fill the full
> buffer. Only kmsg_dump_get_buffer() has the bizarre semantics of not
> using the full buffer.

Right. I got it the wrong way.

> > And it might be better to actually change the condition in
> > find_first_fitting_seq(). I mean to replace:
> >
> > 	if (len <= size || info.seq >= max_seq)
> > with
> > 	if (len < size || info.seq >= max_seq)
> 
> I would prefer not twisting syslog_print_all() to act like
> kmsg_dump_get_buffer().

I agree. It is not a common problem after all.

Best Regards,
Petr

  reply	other threads:[~2021-02-02 12:31 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-26 21:15 [PATCH printk-rework 00/12] printk: remove logbuf_lock John Ogness
2021-01-26 21:15 ` [PATCH printk-rework 01/12] printk: kmsg_dump: remove unused fields John Ogness
2021-01-26 21:15 ` [PATCH printk-rework 02/12] printk: refactor kmsg_dump_get_buffer() John Ogness
2021-01-26 21:15 ` [PATCH printk-rework 03/12] printk: consolidate kmsg_dump_get_buffer/syslog_print_all code John Ogness
     [not found]   ` <YBQgTQYTA5p6Wgj6@alley>
2021-02-01  9:49     ` John Ogness
2021-02-02 12:31       ` Petr Mladek [this message]
2021-01-26 21:15 ` [PATCH printk-rework 04/12] printk: define CONSOLE_LOG_MAX in printk.h John Ogness
     [not found]   ` <YBQtbKrdwUAZQB9v@alley>
2021-02-01  8:24     ` LINE_MAX: was: " John Ogness
2021-02-02 11:22       ` Petr Mladek
2021-01-26 21:15 ` [PATCH printk-rework 05/12] printk: use seqcount_latch for clear_seq John Ogness
2021-01-26 21:15 ` [PATCH printk-rework 06/12] printk: use atomic64_t for devkmsg_user.seq John Ogness
2021-01-26 21:15 ` [PATCH printk-rework 07/12] printk: add syslog_lock John Ogness
2021-02-01 12:26   ` Petr Mladek
2021-02-01 13:11     ` John Ogness
2021-02-02 12:50       ` Petr Mladek
2021-01-26 21:15 ` [PATCH printk-rework 08/12] printk: introduce a kmsg_dump iterator John Ogness
2021-02-01 13:17   ` Petr Mladek
2021-02-01 13:32     ` John Ogness
2021-01-26 21:15 ` [PATCH printk-rework 09/12] um: synchronize kmsg_dumper John Ogness
2021-02-01 10:26   ` Petr Mladek
2021-02-01 14:15   ` Petr Mladek
2021-02-01 16:51     ` John Ogness
2021-02-01 16:54       ` Richard Weinberger
2021-02-01 20:25         ` John Ogness
2021-02-01 20:40           ` Richard Weinberger
2021-02-02 13:26       ` Petr Mladek
2021-01-26 21:15 ` [PATCH printk-rework 10/12] hv: " John Ogness
2021-01-27 21:32   ` Michael Kelley
2021-02-01 10:56     ` John Ogness
2021-01-26 21:15 ` [PATCH printk-rework 11/12] printk: remove logbuf_lock John Ogness
2021-02-02  9:15   ` Petr Mladek
2021-02-02 11:41     ` John Ogness
2021-02-02 16:11       ` Petr Mladek
2021-01-26 21:15 ` [PATCH printk-rework 12/12] printk: kmsg_dump: remove _nolock() variants John Ogness
2021-02-02  9:45   ` Petr Mladek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YBlGCBKgOzf3At/c@alley \
    --to=pmladek@suse.com \
    --cc=john.ogness@linutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=sergey.senozhatsky.work@gmail.com \
    --cc=sergey.senozhatsky@gmail.com \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.