Hi, On Mon, Sep 21, 2015 at 09:46:30AM -0700, David Cohen wrote: > On September 21, 2015 9:27:43 AM PDT, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > >On Mon, 21 Sep 2015, Felipe Balbi wrote: > > > >> On Mon, Sep 21, 2015 at 11:37:47AM -0400, Alan Stern wrote: > >> > On Mon, 21 Sep 2015, Felipe Balbi wrote: > >> > > >> > > On Mon, Sep 21, 2015 at 10:50:10AM -0400, Alan Stern wrote: > >> > > > On Mon, 21 Sep 2015, Felipe Balbi wrote: > >> > > > > >> > > > > On Mon, Sep 21, 2015 at 10:31:15AM -0400, Alan Stern wrote: > >> > > > > > On Mon, 21 Sep 2015, Roger Quadros wrote: > >> > > > > > > >> > > > > > > Using spin_lock() in hard irq handler is pointless > >> > > > > > > and causes a BUG() in RT (real-time) configuration > >> > > > > > > so get rid of it. > >> > > > > > > >> > > > > > Wait a minute. Who says spin_lock is pointless in an IRQ > >handler? > >> > > > > > >> > > > > in the top half IRQs are already disabled, how can this race > >? > >> > > > > >> > > > It can race with code running on a different CPU. > >> > > > >> > > fair point. > >> > > >> > In fact, isn't this the main purpose of spinlocks? There's no > >point > >> > using a spinlock to protect against races occurring on a single > >CPU, > >> > because (in non-RT situations) the kernel can't schedule or preempt > >> > while a spinlock is held, so no race can occur. The whole idea of > >> > spinlocks is to protect against cross-CPU races. > >> > > >> > Now, maybe the spinlock usage that Roger is removing really is > >> > unnecessary in this top-half handler. I don't know; I haven't > >looked > >> > at the code. But in general, spinlocks are highly necessary in IRQ > >> > handlers. > >> > >> this does raise some questions of what to do then ? We can't use > >> spin_locks when in RT because they're reimplemented as mutexes > >> and can sleep. Converting every single driver to raw_spin_locks > >> defeats the purpose of having everything and their dogs preemptable. > >> > >> Difficult to say. Let's defer this patch until we can come up > >> with a good answer to this. > > > >Someone more familiar with the -RT patches should be able to answer > >these questions. > > Spin lock (without irqsave) may have a bug if the irq handler spin locks the > same one, and the irqs aren't already disabled. > E.g. any context (atomic or not) spin locks, then the irq handler takes place on > *same* cpu and tries to spin lock again. It will be a deadlock. > > I believe that's what Roger misinterpreted with his patch description. no, no. you're missing the point here. The problem is that when RT is applied, spinlocks get reimplemented as RT-aware mutexes which works pretty well as long as you don't install your own top and bottom halves. If you do, RT patch can't force your handler to run as a thread and you're left with, essentially, a mutex for synchronization in hardirq context. So the question is really for Steve, what should we do here ? Use a raw_spinlock ? I'd like to avoid that if possible. If we have another option, I'm all ears. -- balbi
Attachment:
signature.asc
Description: PGP signature
