Messages in this thread | | | Date | Tue, 10 Dec 2013 09:36:47 -0800 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE() |
| |
On Tue, Dec 10, 2013 at 02:24:48PM +0100, Ingo Molnar wrote: > > * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote: > > > On Thu, Dec 05, 2013 at 10:33:34AM +0100, Ingo Molnar wrote: > > > > > > * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote: > > > > > > > + (*) The compiler is within its rights to reorder memory accesses unless > > > > + you tell it not to. For example, consider the following interaction > > > > + between process-level code and an interrupt handler: > > > > + > > > > + void process_level(void) > > > > + { > > > > + msg = get_message(); > > > > + flag = true; > > > > + } > > > > + > > > > + void interrupt_handler(void) > > > > + { > > > > + if (flag) > > > > + process_message(msg); > > > > + } > > > > + > > > > + There is nothing to prevent the the compiler from transforming > > > > + process_level() to the following, in fact, this might well be a > > > > + win for single-threaded code: > > > > + > > > > + void process_level(void) > > > > + { > > > > + flag = true; > > > > + msg = get_message(); > > > > + } > > > > + > > > > + If the interrupt occurs between these two statement, then > > > > + interrupt_handler() might be passed a garbled msg. Use ACCESS_ONCE() > > > > + to prevent this as follows: > > > > + > > > > + void process_level(void) > > > > + { > > > > + ACCESS_ONCE(msg) = get_message(); > > > > + ACCESS_ONCE(flag) = true; > > > > + } > > > > + > > > > + void interrupt_handler(void) > > > > + { > > > > + if (ACCESS_ONCE(flag)) > > > > + process_message(ACCESS_ONCE(msg)); > > > > + } > > > > > > Technically, if the interrupt handler is the innermost context, the > > > ACCESS_ONCE() is not needed in the interrupt_handler() code. > > > > > > Since for the vast majority of Linux code IRQ handlers are the most > > > atomic contexts (very few drivers deal with NMIs) I suspect we should > > > either remove that ACCESS_ONCE() from the example or add a comment > > > explaining that in many cases those are superfluous? > > > > How about the following additional paragraph? > > > > Note that the ACCESS_ONCE() wrappers in interrupt_handler() > > are needed if this interrupt handler can itself be interrupted > > by something that also accesses 'flag' and 'msg', for example, > > a nested interrupt or an NMI. Otherwise, ACCESS_ONCE() is not > > needed in interrupt_handler() other than for documentation purposes. > > Sounds great to me! > > Note that nested IRQs generally don't happen on modern Linux anymore, > we run almost all hardirqs with irqs disabled and in fact have a > warning to detect irq handlers that enable irqs: > > res = action->handler(irq, action->dev_id); > trace_irq_handler_exit(irq, action, res); > > if (WARN_ONCE(!irqs_disabled(),"irq %u handler %pF enabled interrupts\n", > irq, action->handler)) > local_irq_disable();
Good point! I added the following at the end of the paragraph:
(Note also that nested interrupts do not typically occur in modern Linux kernels, in fact, if an interrupt handler returns with interrupts enabled, you will get a WARN_ONCE() splat.)
I guess an IRQ handler could momentarily enable interrupts as long as it disabled them again before returning, but I don't see any reason to encourage that practice in Documentation/memory-barriers.txt. ;-)
Thanx, Paul
| |