Re: [PATCH printk-rework 08/14] printk: add syslog_lock

[Petr Mladek <pmladek@suse.com>]

On Thu 2021-02-18 09:18:11, John Ogness wrote:
> The global variables @syslog_seq, @syslog_partial, @syslog_time
> and write access to @clear_seq are protected by @logbuf_lock.
> Once @logbuf_lock is removed, these variables will need their
> own synchronization method. Introduce @syslog_lock for this
> purpose.
> 
> @syslog_lock is a raw_spin_lock for now. This simplifies the
> transition to removing @logbuf_lock. Once @logbuf_lock and the
> safe buffers are removed, @syslog_lock can change to spin_lock.
> ---
>  kernel/printk/printk.c | 41 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 20c21a25143d..401df370832b 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> +/* Return a consistent copy of @syslog_seq. */
> +static u64 read_syslog_seq_irq(void)
> +{
> +	u64 seq;
> +
> +	raw_spin_lock_irq(&syslog_lock);
> +	seq = syslog_seq;
> +	raw_spin_unlock_irq(&syslog_lock);

Is there any particular reason to disable interrupts here?

It would make sense only when the lock could be taken in IRQ
context. Then we would need to always disable interrupts when
the lock is taken. And if it is taken in IRQ context, we would
need to safe flags.

I guess that you got confused because it is used in
wait_event_interruptible(). The name is misleading.
"interruptible" means wake_up_process() and not IRQ here.

IMHO, we should remove _irq suffix from the lock operations
and also from the function name.

> +
> +	return seq;
> +}
> +
>  int do_syslog(int type, char __user *buf, int len, int source)
>  {
>  	struct printk_info info;
> @@ -1664,8 +1688,9 @@ int do_syslog(int type, char __user *buf, int len, int source)
>  			return 0;
>  		if (!access_ok(buf, len))
>  			return -EFAULT;
> +
>  		error = wait_event_interruptible(log_wait,
> -				prb_read_valid(prb, syslog_seq, NULL));
> +				prb_read_valid(prb, read_syslog_seq_irq(), NULL));
>  		if (error)
>  			return error;
>  		error = syslog_print(buf, len);

Otherwise, the patch looks good to me.

Best Regards,
Petr