lkml.org 
[lkml]   [2011]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC][PATCH -next] pstore: replace spin_lock with spin_trylock_irqsave in panic path
    On Tue, Sep 27, 2011 at 01:14:59PM -0400, Seiji Aguchi wrote:
    > Hi,
    >
    > [Problem]
    > Currently, pstore takes spin_trylock(&psinfo->buf_lock) in NMI context.
    > And it takes spin_lock(&psinfo->buf_lock) in other cases.
    >
    > If there are some bugs in pstore and kernel panics, spin_lock(&psinfo->buf_lock) causes deadlock
    > and panic_notifier_chain will not work.

    Ok, so I missed your 'return' first time through and originally had a
    bunch of comments. So I would suggest adding a comment explaining why we
    are returning in that failure.

    Personally, I am not sure we want to abort here at the pstore layer, it
    should probably be aborted lower. There isn't any reason why we can't
    continue from a pstore perspective (we can just bust the spinlock).

    From an ERST perspective, the state machine might be screwed up, hence
    aborting in that layer could make sense. But I don't think I agree with
    the 'return' statement.

    So I am opposed to it for now.

    Cheers,
    Don


    >
    > [Patch Description]
    > For solving this problem, this patch replaces spin_lock with spin_trylock_irqsave in panic path.
    >
    > Dead lock in panic path will not happen by applying this patch.
    >
    > Signed-off-by: Seiji Aguchi <seiji.aguchi@hds.com>
    >
    > ---
    > fs/pstore/platform.c | 17 ++++++++---------
    > 1 files changed, 8 insertions(+), 9 deletions(-)
    >
    > diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index 0472924..9882892 100644
    > --- a/fs/pstore/platform.c
    > +++ b/fs/pstore/platform.c
    > @@ -97,12 +97,15 @@ static void pstore_dump(struct kmsg_dumper *dumper,
    > else
    > why = "Unknown";
    >
    > - if (in_nmi()) {
    > - is_locked = spin_trylock(&psinfo->buf_lock);
    > - if (!is_locked)
    > - pr_err("pstore dump routine blocked in NMI, may corrupt error record\n");
    > + if (reason == KMSG_DUMP_PANIC) {
    > + is_locked = spin_trylock_irqsave(&psinfo->buf_lock, flags);
    > + if (!is_locked) {
    > + pr_err("pstore dump routine skipped in panic path\n");
    > + return;
    > + }
    > } else
    > spin_lock_irqsave(&psinfo->buf_lock, flags);
    > +
    > oopscount++;
    > while (total < kmsg_bytes) {
    > dst = psinfo->buf;
    > @@ -131,11 +134,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
    > total += l1_cpy + l2_cpy;
    > part++;
    > }
    > - if (in_nmi()) {
    > - if (is_locked)
    > - spin_unlock(&psinfo->buf_lock);
    > - } else
    > - spin_unlock_irqrestore(&psinfo->buf_lock, flags);
    > + spin_unlock_irqrestore(&psinfo->buf_lock, flags);
    > }
    >
    > static struct kmsg_dumper pstore_dumper = {
    > --
    > 1.7.1
    >


    \
     
     \ /
      Last update: 2011-09-27 19:37    [W:0.026 / U:1.632 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site