Re: syslog: was: [PATCH next v2 3/3] printk: remove logbuf_lock, add syslog_lock

[John Ogness]

On 2020-12-04, Petr Mladek <pmladek@xxxxxxxx> 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