lkml.org 
[lkml]   [2011]   [Aug]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: pstore: change mutex locking to spin_locks
On Wed, Aug 17, 2011 at 02:22:25PM -0700, Andrew Morton wrote:
> On Fri, 12 Aug 2011 10:54:51 -0700
> "Luck, Tony" <tony.luck@intel.com> wrote:
>
> > From: Don Zickus <dzickus@redhat.com>
> >
> > pstore was using mutex locking to protect read/write access to the
> > backend plug-ins. This causes problems when pstore is executed in
> > an NMI context through panic() -> kmsg_dump().
> >
> > This patch changes the mutex to a spin_lock_irqsave then also checks to
> > see if we are in an NMI context. If we are in an NMI and can't get the
> > lock, just print a message stating that and blow by the locking.
> >
> > All this is probably a hack around the bigger locking problem but it
> > solves my current situation of trying to sleep in an NMI context.
> >
> > Tested by loading the lkdtm module and executing a HARDLOCKUP which
> > will cause the machine to panic inside the nmi handler.
> >
> > ...
> >
> > + 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");
> > + } else
> > + spin_lock_irqsave(&psinfo->buf_lock, flags);
> > oopscount++;
> > while (total < kmsg_bytes) {
> > dst = psinfo->buf;
> > @@ -123,7 +131,11 @@ static void pstore_dump(struct kmsg_dumper *dumper,
> > total += l1_cpy + l2_cpy;
> > part++;
> > }
> > - mutex_unlock(&psinfo->buf_mutex);
> > + if (in_nmi()) {
> > + if (is_locked)
> > + spin_unlock(&psinfo->buf_lock);
> > + } else
> > + spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> > }
>
> It's still bad if lockdep is enabled. See
> kernel/lockdep.c:lock_acquire() and lock_release(). They aren't
> NMI-safe.
>
> One approach would be to switch to bit_spin_lock(). Which will break
> if/when bit spinlocks get lockdep-enabled, so don't do that.
>
> A better approach would be to use the underlying spinlock functions
> which bypass lockdep, but I cannot immediately locate those amongst
> the misama of spinlock interface mess.

Probably the raw_spin_* stuff. I think those purposely avoid the lockdep
mechanisms.

>
> This problem of locking-vs-NMIs has been "solved" several times before
> but I don't recall any standardized approach being developed. Does
> anyone have a favorite implementation to look at?

The ones I have looked at perf and apei/ghes, had issues of receiving data
in an NMI context and trying to pass it to userspace. This was solved
with irq_work_queue. Sometimes one can sprinkle some cmpxchg commands in
there to quickly write to registers from a normal context which seems to
play nicely with a process in an NMI context wants to write to the same
register.

But in this case we have a filesystem that can be read/written to from a
normal context and also written to from an NMI context. irq_work_queue
doesn't apply here and I don't think you can just cmpxchg a PAGE full of
data into a firmware storage area. This doesn't even get into the state
machine the kernel needs to walk through to store the data (which can
easily be interrupted by an NMI).

I would be excited in there was a solution that we can copy, but I didn't
see any nor would I really expect one in this unusual case.

The patch that I provided that Tony reposted is just a lesser of two evils
approach. It is still flawed, just not as much as before. The idea was
to buy time until we could think of a better approach to solving this.

Cheers,
Don



\
 
 \ /
  Last update: 2011-08-18 15:01    [W:0.085 / U:0.752 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site