Re: [PATCH] fix a Sparse warning in the arch/x86/kernel/irq_work.cfile

[Steven Rostedt]

On Mon, 12 Aug 2013 15:18:16 -0700
Shailaja Neelam <neelamshaila@xxxxxxxxx> wrote:

> I am a high school student trying to become familiar with the
> opensource process and linux kernel. This is my first submission to
> the ITC mailing list.

Hi Shailaja,

Welcome to Linux kernel development :-)

> 
> My patch is for the file arch/x86/kernel/irq_work.c in the vesion
> linux-3.10 kernel. When I ran the kernel with Sparse, the error read:
> arch/x86/kernel/irq_work.c:21:
> 6: warning: symbol 'arch_irq_work_raise'
> was not declared. Should it be static?
> 
> To fix this (rather than add static) I declared the symbol in the
> header file linux/irq_work.h.

Correct, because adding static would have been a bug.


> Afterwards, my error did not show up
> when I ran the kernel with Sparse again. I also ran the command "make
> menuconfig" to change the kernel version so that I could assure the
> correct kernel was running when I tested it, and it was. Then I test
> built the kernel. It built and rebooted correctly.

The patch looks good. Let me give you a bit more information and
background on that function. Just your FYI.

The purpose of irq_work() in general, is to trigger some event in a
critical location that is not safe to do the event you want to trigger.
For example, in perf, it may be executing within a Non-Maskable
Interrupt (NMI) or within the scheduler with the runqueue (rq) lock
held. If it would like to wake up a task that is monitoring the perf
output, it can't because to do so would require taking the rq lock and
possibly causing a deadlock.

Instead, it calls irq_work() to do the event within a normal interrupt
context. Some architectures have the ability to trigger an interrupt on
the current CPU that it is running on. This way, if we are in an NMI,
we trigger the self interrupt to the CPU, but because NMIs run with
interrupts disable, and the rq lock is held with interrupts disabled,
the interrupt will stay pending until interrupts are enabled again (out
of NMI and out from holding the rq lock). When interrupts are enabled,
the interrupt that we sent will trigger and run our event in a safe
location (someplace that allows for interrupts to be enabled).

But to do this self triggering of an interrupt requires specific
architecture knowledge. As Linux supports 30 architectures, and few
of us have hardware to test on each of these or even know how to write
code for all of them, we have ways to do things for just the
architectures we are familiar with. Some architectures may not even
have the ability to do what we want, even if we knew the architecture
well.

The "arch_irq_work_raise()" function is one of these cases. For
architectures that do not support a self triggering interrupt, or one
that we just didn't get to yet, we create a generic
arch_irq_work_raise() function that just does our work at the next
timer interrupt. This isn't the most efficient way, because that next
timer interrupt may be 10 milliseconds away. But we annotate that
function with the gcc "__attribute__((weak))" attribute (defined in
include/linux/compiler-gcc.h as "__weak").

What the __weak attribute does, is to tell the compiler (linker really)
that this function is to be used if it is not defined someplace else.
Then, in an architecture that has this specific optimization, we write
an arch_irq_work_raise() function without the "__weak" attribute, and
the linker will use that function instead at all of the call sites that
reference it. This way, the generic code can support architectures that
does the optimization as well as those that don't.


> 
> Signed-off-by: Shailaja Neelam <neelamshaila@xxxxxxxxx>

Reviewed-by: Steven Rostedt <rostedt@xxxxxxxxxxx>

-- Steve


> ---
> --- linux-3.10/include/linux/irq_
> work.h    2013-06-30 15:13:29.000000000 -0700
> +++ linux-3.10.change/include/linux/irq_work.h    2013-07-24
> 12:06:15.521140635 -0700
> @@ -33,6 +33,7 @@ void init_irq_work(struct irq_work *work
>  void irq_work_queue(struct irq_work *work);
>  void irq_work_run(void);
>  void irq_work_sync(struct irq_work *work);
> +void arch_irq_work_raise(void);
> 
>  #ifdef CONFIG_IRQ_WORK
>  bool irq_work_needs_cpu(void);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/