Date:	Tue, 13 Aug 2013 11:09:34 +0530
From:	anish singh <anish198519851985@...il.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	Shailaja Neelam <neelamshaila@...il.com>,
	Frédéric Weisbecker <fweisbec@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	james.hogan@...tec.com,
	linux-kernel-mail <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH] fix a Sparse warning in the arch/x86/kernel/irq_work.c file

On Tue, Aug 13, 2013 at 5:59 AM, Steven Rostedt <rostedt@...dmis.org> wrote:
> On Mon, 12 Aug 2013 15:18:16 -0700
> Shailaja Neelam <neelamshaila@...il.com> 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

irq_work processes event in normal interrupt context as you said but
why interrupt context ? Is it because of the fast processing which is
needed? Can we use softirq as anyways we have interrupt
disabled(functions which calls irq_work makes sure of that right?).
Hope I am not asking something very obvious.
> 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@...il.com>
>
> Reviewed-by: Steven Rostedt <rostedt@...dmis.org>

Nice explanation.
>
> -- 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists