Re: [PATCH v4 2/2] Output stall traces in /proc

[Don Zickus <dzickus@redhat.com>]

(adding Peter to the discussion)

On Fri, Aug 05, 2011 at 02:26:12PM -0700, ZAK Magnus wrote:
> On Fri, Aug 5, 2011 at 11:43 AM, Don Zickus <dzickus@redhat.com> wrote:
> > I missed that you defined that as a pointer to a spinlock and assigned it
> > later.  I see what you are doing now, but I am not a fan of it because you
> > are now using the same spinlock in both the NMI context and the userspace
> > context.  This can cause deadlocks if something got screwed up in the
> > seq_printf functions or produced a very large amount of data.  Normally
> > you don't want to do that.
> >
> > What others have done like perf and the APEI error handling is use
> > something called irq_work_queue(??).  Basically you would capture the
> > tracae in the NMI context, put it on an irq_work_queue and in the
> > interrupt context save it to your global trace variable.  Then you could
> > put spin_lock_irqsave inside the proc sys function and the work queue
> > function and not have any potential deadlocks.
> Work queue? Okay. The worker thread still needs a lock in order to

not work_queue, irq_work_queue.

> share the intermediate buffer with the NMI context, though. Any chance
> of something screwing up in the middle of copying that structure,
> causing a stall and deadlocking with the NMI?

I believe irq_work_queue uses cmpxchg for all its locking and just swaps
entries on to a linked list?

> 
> Or maybe the intermediate buffer should be dynamically allocated. That
> would work without a lock, although it seems slightly inefficient.

Peter,

How does the irq_work_queue work such that you can save info in the NMI
context and safely pass it to the irq context for processing? 

> 
> Regarding the lock between the work queue thread and the system call,
> maybe that should become a mutex instead, since it's all outside of
> interrupt context at that point?

No it is still in the irq context.

Peter,

If we want to expose data captured in the NMI context through the procfs,
I assume we can pass that info along using irq_work_queue.  But then when
reading from procfs do we just lock the data with 'spin_lock_irq' to block
the irq_work_queue from manipulating the data?  (note we are expecting
data to be overwritten with fresh data, not serialized out like
trace/perf).

Cheers,
Don

> 
> > The softstall case should be ok though.
> Why's that? The soft stall traces are not written in a NMI context but
> just in a regular interrupt context, right? Doesn't that pose similar
> problems?
> 
> 
> These are weird rare corner cases anyway, right? Maybe the simplest
> thing could be to let the interrupts only try_lock(), so they might
> sometimes fail to record a stall, but it would be a pretty big
> coincidence.