Re: [PATCH printk-rework 07/12] printk: add syslog_lock

[Petr Mladek]

On Mon 2021-02-01 14:17:55, John Ogness wrote:
> On 2021-02-01, Petr Mladek <pmladek@xxxxxxxx> 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.
> >
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -390,8 +390,12 @@ DEFINE_RAW_SPINLOCK(logbuf_lock);
> >>  		printk_safe_exit_irqrestore(flags);	\
> >>  	} while (0)
> >>  
> >> +/* syslog_lock protects syslog_* variables and write access to clear_seq. */
> >> +static DEFINE_RAW_SPINLOCK(syslog_lock);
> >
> > I am not expert on RT code but I think that it prefers the generic
> > spinlocks. syslog_lock seems to be used in a normal context.
> > IMHO, it does not need to be a raw spinlock.
> >
> > Note that using normal spinlock would require switching the locking
> > order. logbuf_lock is a raw lock. Normal spinlock must not be taken
> > under a raw spinlock.
> >
> > Or we could switch syslog_lock to the normal spinlock later
> > after logbuf_lock is removed.
> 
> I was planning on this last option because I think it is the
> simplest. There are places such as syslog_print_all() where the
> printk_safe_enter() and logbuf_lock locking are not at the same place as
> the syslog_lock locking (and syslog_lock is inside).
>
> Once the safe buffers are removed, syslog_lock can transition to a
> spinlock. (spinlock's must not be under local_irq_save().)

Fair enough. Please, mention in the commit message that it
will get switched to normal spinlock later. And the the raw
spinlock is used to make the transition more straightforward
or something like this.
 
> >> +
> >>  #ifdef CONFIG_PRINTK
> >>  DECLARE_WAIT_QUEUE_HEAD(log_wait);
> >> +/* All 3 protected by @syslog_lock. */
> >>  /* the next printk record to read by syslog(READ) or /proc/kmsg */
> >>  static u64 syslog_seq;
> >>  static size_t syslog_partial;
> >> @@ -1648,8 +1661,14 @@ int do_syslog(int type, char __user *buf, int len, int source)
> >>  			return 0;
> >>  		if (!access_ok(buf, len))
> >>  			return -EFAULT;
> >> +
> >> +		/* Get a consistent copy of @syslog_seq. */
> >> +		raw_spin_lock_irq(&syslog_lock);
> >> +		seq = syslog_seq;
> >> +		raw_spin_unlock_irq(&syslog_lock);
> >> +
> >>  		error = wait_event_interruptible(log_wait,
> >> -				prb_read_valid(prb, syslog_seq, NULL));
> >> +				prb_read_valid(prb, seq, NULL));
> >
> > Hmm, this will not detect when syslog_seq gets cleared in parallel.
> > I hope that nobody rely on this behavior. But who knows?
> >
> > A solution might be to have also syslog_seq latched. But I am
> > not sure if it is worth it.
> >
> > I am for taking the risk and use the patch as it is now. Let's keep
> > the code for now. We could always use the latched variable when
> > anyone complains. Just keep it in mind.
> 
> We could add a simple helper:
> 
> /* Get a consistent copy of @syslog_seq. */
> static u64 syslog_seq_read(void)
> {
>     unsigned long flags;
> 
>     raw_spin_lock_irqsave(&syslog_lock, flags);
>     seq = syslog_seq;
>     raw_spin_unlock_irqrestore(&syslog_lock, flags);
>     return seq;
> }
> 
> Then change the code to:
> 
>     error = wait_event_interruptible(log_wait,
>                     prb_read_valid(prb, read_syslog_seq(), NULL));
> 

Great idea! Please, use it but without the flags. IMHO, using flags
might be confusing when reading the come. It might create false
expectations...

> register_console() could also make use of the function. (That is why I
> am suggesting the flags variant.)

I think that flags are actually not needed in register_console() as
mentioned in the other mail. Anyway, we could keep register_console()
as is (opencoded) for now.

Best Regards,
Petr