lkml.org 
[lkml]   [2021]   [Feb]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH printk-rework 08/14] printk: add syslog_lock
    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

    \
     
     \ /
      Last update: 2021-02-19 14:33    [W:3.008 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site