Messages in this thread | | | From | John Ogness <> | Subject | Re: syslog: was: [PATCH next v2 3/3] printk: remove logbuf_lock, add syslog_lock | Date | Sun, 06 Dec 2020 22:12:21 +0106 |
| |
On 2020-12-04, Petr Mladek <pmladek@suse.com> wrote: > 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. >> + */
Your suggestion to merge this and the next comment block is fine.
>> + 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;
OK.
>> 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.
OK.
>> 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.
OK. So you just want "exclusive_console = newcon;" moved outside the critical section.
>> /* >> * 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();
John Ogness
| |