[PATCH RFC] irq: Honor /proc/irq core affinity when an IRQ thread is created

[PATCH RFC] irq: Honor /proc/irq core affinity when an IRQ thread is created

[Sankara Muthukrishnan]

Hi everyone,

I would like to get some feedback from the RT community before
submitting to the mainline.

Currently, by writing to /proc/irq/../smp_affinity file, core affinity
of already running IRQ threads can be modified. However, after writing
to the /proc file, an IRQ thread that gets created later does not
inherit the affinity specified in the file. This happens because the
irq action is registered in irq description only after setup_affinity
function is called. This patch addresses this problem by calling
setup_affinity function after the irq action is updated in the irq
description structure. It looks like the existing behavior (without
this patch) is inconsistent because in the case of multiple action
handlers for an irq, the IRQ thread associated with the new action
handler that is being registered does not inherit the core affinity
from the /proc file, however, the IRQ threads of the existing action
handlers do inherit the affinity.

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 87dc0539cebb26bd7122e1f2a608be7325ade437..05c64c5fc2be5c2db34e74e5a5f4d3c6f9c74bca
100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1083,9 +1083,6 @@ __setup_irq(unsigned int irq, struct irq_desc
*desc, struct irqaction *new)
         if (new->flags & IRQF_NO_SOFTIRQ_CALL)
             irq_settings_set_no_softirq_call(desc);

-        /* Set default affinity mask once everything is setup */
-        setup_affinity(irq, desc, mask);

Re: [PATCH RFC] irq: Honor /proc/irq core affinity when an IRQ thread is created

[Thomas Gleixner]

On Mon, 27 Aug 2012, Sankara Muthukrishnan wrote:

> Hi everyone,
> 
> I would like to get some feedback from the RT community before
> submitting to the mainline.
> 
> Currently, by writing to /proc/irq/../smp_affinity file, core affinity
> of already running IRQ threads can be modified. However, after writing
> to the /proc file, an IRQ thread that gets created later does not
> inherit the affinity specified in the file. This happens because the
> irq action is registered in irq description only after setup_affinity
> function is called. This patch addresses this problem by calling
> setup_affinity function after the irq action is updated in the irq
> description structure. It looks like the existing behavior (without
> this patch) is inconsistent because in the case of multiple action
> handlers for an irq, the IRQ thread associated with the new action
> handler that is being registered does not inherit the core affinity
> from the /proc file, however, the IRQ threads of the existing action
> handlers do inherit the affinity.

I understand the problem, but the proposed patch is not going to solve
it. setup_affinity() is just called in the !shared case, which is the
first action which is set up. So that works for the first irq, but not
for later ones.

Does the following patch fix the problem for both shared and non
shared interrupts ?
 
Thanks,

	tglx
---
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 4c69326..a16ff28 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -936,6 +936,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		 */
 		get_task_struct(t);
 		new->thread = t;
+		set_bit(IRQTF_AFFINITY, &new->thread_flags);
 	}
 
 	if (!alloc_cpumask_var(&mask, GFP_KERNEL)) {

Re: [PATCH RFC] irq: Honor /proc/irq core affinity when an IRQ thread is created

[Sankara Muthukrishnan]

On Tue, Sep 4, 2012 at 10:43 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Mon, 27 Aug 2012, Sankara Muthukrishnan wrote:
>
> > Hi everyone,
> >
> > I would like to get some feedback from the RT community before
> > submitting to the mainline.
> >
> > Currently, by writing to /proc/irq/../smp_affinity file, core affinity
> > of already running IRQ threads can be modified. However, after writing
> > to the /proc file, an IRQ thread that gets created later does not
> > inherit the affinity specified in the file. This happens because the
> > irq action is registered in irq description only after setup_affinity
> > function is called. This patch addresses this problem by calling
> > setup_affinity function after the irq action is updated in the irq
> > description structure. It looks like the existing behavior (without
> > this patch) is inconsistent because in the case of multiple action
> > handlers for an irq, the IRQ thread associated with the new action
> > handler that is being registered does not inherit the core affinity
> > from the /proc file, however, the IRQ threads of the existing action
> > handlers do inherit the affinity.
>
> I understand the problem, but the proposed patch is not going to solve
> it. setup_affinity() is just called in the !shared case, which is the
> first action which is set up. So that works for the first irq, but not
> for later ones.
>
> Does the following patch fix the problem for both shared and non
> shared interrupts ?

Thanks Thomas. Your suggestion works for non-shared. I do not have
devices sharing interrupts on my board to quickly verify. However, I
hacked a kernel module to simulate shared irq's and the kernel source
(to set the affinity before waiting for interrupts in the irq thread)
and verified that it works for "shared interrupts" also.

However, I was wondering whether we are relaxing some checks that
setup_affinity does (like checking for flags such as
IRQD_NO_BALANCING) by setting the flag IRQTF_AFFINITY after creating
the kernel thread directly as you suggested. May be, it does not have
practical implications. But, I was wondering whether it is a good idea
to go through all the checks as the checks are done when someone
writes to /proc/irq/.../smp_affinity file for consistency. Is the
following patch a bad idea (basically call setup_affinity for both
shared and non-shared interrupts)? This also works in my testing. I am
curious why "setup_affinity" was called only under "if (!shared)" case
in the released kernel.

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 87dc0539cebb26bd7122e1f2a608be7325ade437..05c64c5fc2be5c2db34e74e5a5f4d3c6f9c74bca
100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1083,9 +1083,6 @@ __setup_irq(unsigned int irq, struct irq_desc
*desc, struct irqaction *new)
         if (new->flags & IRQF_NO_SOFTIRQ_CALL)
             irq_settings_set_no_softirq_call(desc);

-        /* Set default affinity mask once everything is setup */
-        setup_affinity(irq, desc, mask);
-
     } else if (new->flags & IRQF_TRIGGER_MASK) {
         unsigned int nmsk = new->flags & IRQF_TRIGGER_MASK;
         unsigned int omsk = irq_settings_get_trigger_mask(desc);
@@ -1099,6 +1096,17 @@ __setup_irq(unsigned int irq, struct irq_desc
*desc, struct irqaction *new)
     new->irq = irq;
     *old_ptr = new;

+    /* Set default affinity mask once everything is setup.
+    setup_affinity(irq, desc, mask);

     /* Reset broken irq detection when installing new handler */
     desc->irq_count = 0;
     desc->irqs_unhandled = 0;



>
> Thanks,
>
>         tglx
> ---
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 4c69326..a16ff28 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -936,6 +936,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>                  */
>                 get_task_struct(t);
>                 new->thread = t;
> +               set_bit(IRQTF_AFFINITY, &new->thread_flags);
>         }
>
>         if (!alloc_cpumask_var(&mask, GFP_KERNEL)) {

Re: [PATCH RFC] irq: Honor /proc/irq core affinity when an IRQ thread is created

[Thomas Gleixner]

On Wed, 5 Sep 2012, Sankara Muthukrishnan wrote:
> On Tue, Sep 4, 2012 at 10:43 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > Does the following patch fix the problem for both shared and non
> > shared interrupts ?
> 
> Thanks Thomas. Your suggestion works for non-shared. I do not have
> devices sharing interrupts on my board to quickly verify. However, I
> hacked a kernel module to simulate shared irq's and the kernel source
> (to set the affinity before waiting for interrupts in the irq thread)
> and verified that it works for "shared interrupts" also.
> 
> However, I was wondering whether we are relaxing some checks that
> setup_affinity does (like checking for flags such as
> IRQD_NO_BALANCING) by setting the flag IRQTF_AFFINITY after creating
> the kernel thread directly as you suggested. May be, it does not have
> practical implications. But, I was wondering whether it is a good idea
> to go through all the checks as the checks are done when someone
> writes to /proc/irq/.../smp_affinity file for consistency. Is the
> following patch a bad idea (basically call setup_affinity for both
> shared and non-shared interrupts)? This also works in my testing. I am
> curious why "setup_affinity" was called only under "if (!shared)" case
> in the released kernel.

The reason is that it is sufficient to do that for the first interrupt
which gets set up. If another interrupt handler gets registered in the
shared case then its pointless to call setup_affinity() as everything
is already in place. The only thing which does not follow is the
shared thread.

So now for the IRQTF_AFFINITY flag. We really want to set that
unconditionally so all irq threads even if the NO_BALANCING flag is
set move to the cpu(s) on which the interrupt is assigned
to. NO_BALANCING is just making sure that the user space irq balancer
cannot move around the interrupt, but it still has an affinity
assigned.

Thanks,

	tglx