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>,
Linus Torvalds <torvalds@linux-foundation.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Thomas Gleixner <tglx@linutronix.de>,
linux-kernel@vger.kernel.org
Subject: syslog: was: [PATCH next v2 3/3] printk: remove logbuf_lock, add syslog_lock
Date: Fri, 4 Dec 2020 16:57:46 +0100 [thread overview]
Message-ID: <X8pceqpK+sAudugq@alley> (raw)
In-Reply-To: <20201201205341.3871-4-john.ogness@linutronix.de>
On Tue 2020-12-01 21:59:41, John Ogness wrote:
> Since the ringbuffer is lockless, there is no need for it to be
> protected by @logbuf_lock. Remove @logbuf_lock.
>
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1490,19 +1444,30 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
> return -ENOMEM;
>
> time = printk_time;
> - logbuf_lock_irq();
> clr_seq = atomic64_read(&clear_seq);
>
> /*
> * Find first record that fits, including all following records,
> * into the user-provided buffer for this dump.
> */
> +
> prb_for_each_info(clr_seq, prb, seq, &info, &line_count)
> len += get_record_print_text_size(&info, line_count, true, time);
>
> - /* move first record forward until length fits into the buffer */
> + /*
> + * Keep track of the latest in case new records are coming in fast
> + * and overwriting the older records.
> + */
"overwriting the older records" sounds like the code is somehow able
to remove the overwritten records from "len". But it is not true.
> + newest_seq = seq;
> +
> + /*
> + * Move first record forward until length fits into the buffer. This
> + * is a best effort attempt. If @newest_seq is reached because the
> + * ringbuffer is wrapping too fast, just start filling the buffer
> + * from there.
> + */
It might be that I do not understand English well. But "start filling
the buffer from there" sounds like we start filling the buffer from
"newest_seq".
What about the following?
/*
* 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 the best effort that prevents an infinite loop.
*/
newest_seq = seq;
> prb_for_each_info(clr_seq, prb, seq, &info, &line_count) {
> - if (len <= size)
> + if (len <= size || info.seq > newest_seq)
> break;
> len -= get_record_print_text_size(&info, line_count, true, time);
> }
> @@ -1568,8 +1529,11 @@ int do_syslog(int type, char __user *buf, int len, int source)
> return 0;
> if (!access_ok(buf, len))
> return -EFAULT;
> + spin_lock_irq(&syslog_lock);
> + seq = syslog_seq;
> + spin_unlock_irq(&syslog_lock);
It would deserve a comment that the locking is needed to guarantee
atomicity of the operation.
> error = wait_event_interruptible(log_wait,
> - prb_read_valid(prb, syslog_seq, NULL));
> + prb_read_valid(prb, seq, NULL));
> if (error)
> return error;
> error = syslog_print(buf, len);
> @@ -2809,11 +2856,7 @@ void register_console(struct console *newcon)
> nr_ext_console_drivers++;
>
> if (newcon->flags & CON_PRINTBUFFER) {
> - /*
> - * console_unlock(); will print out the buffered messages
> - * for us.
> - */
> - logbuf_lock_irqsave(flags);
> + spin_lock_irqsave(&syslog_lock, flags);
We should take the lock only around assigning syslog_seq. And add a
comment that it guarantees atomic update.
> /*
> * We're about to replay the log buffer. Only do this to the
> * just-registered console to avoid excessive message spam to
> @@ -2826,7 +2869,7 @@ void register_console(struct console *newcon)
> exclusive_console = newcon;
> exclusive_console_stop_seq = console_seq;
> console_seq = syslog_seq;
> - logbuf_unlock_irqrestore(flags);
> + spin_unlock_irqrestore(&syslog_lock, flags);
> }
> console_unlock();
> console_sysfs_notify();
Best Regards,
Petr
next prev parent reply other threads:[~2020-12-04 15:58 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-01 20:53 [PATCH next v2 0/3] printk: remove logbuf_lock John Ogness
2020-12-01 20:53 ` [PATCH next v2 1/3] printk: inline log_output(),log_store() in vprintk_store() John Ogness
2020-12-03 15:57 ` Petr Mladek
2020-12-03 16:25 ` John Ogness
2020-12-04 6:13 ` Sergey Senozhatsky
2020-12-04 8:26 ` Petr Mladek
2020-12-01 20:53 ` [PATCH next v2 2/3] printk: change @clear_seq to atomic64_t John Ogness
2020-12-04 9:12 ` Petr Mladek
2020-12-06 20:23 ` John Ogness
2020-12-07 9:34 ` Peter Zijlstra
2020-12-07 10:03 ` John Ogness
2020-12-07 12:56 ` Peter Zijlstra
2020-12-07 12:56 ` Petr Mladek
2020-12-07 16:46 ` David Laight
2020-12-08 20:34 ` Sergey Senozhatsky
2020-12-08 22:30 ` John Ogness
2020-12-09 1:04 ` Sergey Senozhatsky
2020-12-09 8:16 ` Peter Zijlstra
2020-12-09 9:22 ` Sergey Senozhatsky
2020-12-09 10:46 ` Sergey Senozhatsky
2020-12-09 11:00 ` Peter Zijlstra
2020-12-09 11:28 ` Sergey Senozhatsky
2020-12-09 12:29 ` Peter Zijlstra
2020-12-09 8:07 ` Peter Zijlstra
2020-12-01 20:53 ` [PATCH next v2 3/3] printk: remove logbuf_lock, add syslog_lock John Ogness
2020-12-04 6:41 ` Sergey Senozhatsky
2020-12-06 20:44 ` John Ogness
2020-12-04 15:52 ` devkmsg: was " Petr Mladek
2020-12-06 20:51 ` John Ogness
2020-12-07 9:56 ` Petr Mladek
2020-12-04 15:57 ` Petr Mladek [this message]
2020-12-06 21:06 ` syslog: was: " John Ogness
2020-12-07 10:01 ` Petr Mladek
2020-12-04 16:10 ` recursion handling: " Petr Mladek
2020-12-05 4:25 ` Sergey Senozhatsky
2020-12-06 22:08 ` John Ogness
2020-12-05 9:41 ` Sergey Senozhatsky
2020-12-06 22:17 ` John Ogness
2020-12-06 21:44 ` John Ogness
2020-12-07 11:17 ` Petr Mladek
2020-12-04 16:15 ` vprintk_store: was: " Petr Mladek
2020-12-06 22:30 ` John Ogness
2020-12-07 12:46 ` Petr Mladek
2020-12-04 16:19 ` consoles: " Petr Mladek
2020-12-05 4:39 ` Sergey Senozhatsky
2020-12-07 9:50 ` Petr Mladek
2020-12-08 20:51 ` Sergey Senozhatsky
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=X8pceqpK+sAudugq@alley \
--to=pmladek@suse.com \
--cc=gregkh@linuxfoundation.org \
--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 \
--cc=torvalds@linux-foundation.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).