[RFC] Correct behaviour of irq affinity?

[RFC] Correct behaviour of irq affinity?

[Rusty Russell]

The effect of setting desc->affinity (ie. from userspace via sysfs) has varied
over time.  In 2.6.27, the 32-bit code anded the value with cpu_online_map,
and both 32 and 64-bit did that anding whenever a cpu was unplugged.

2.6.29 consolidated this into one routine (and fixed hotplug) but introduced
another variation: anding the affinity with cfg->domain.  Is this right, or
should we just set it to what the user said?  Or as now, indicate that we're
restricting it.

If we should change it, here's what the patch looks like against x86 tip
(cpu_mask_to_apicid_and already takes cpu_online_mask into account):

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 86827d8..30906cd 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -592,10 +592,10 @@ set_desc_affinity(struct irq_desc *desc, const struct cpumask *mask)
 	if (assign_irq_vector(irq, cfg, mask))
 		return BAD_APICID;
 
-	cpumask_and(desc->affinity, cfg->domain, mask);
+	cpumask_copy(desc->affinity, mask);
 	set_extra_move_desc(desc, mask);
 
-	return apic->cpu_mask_to_apicid_and(desc->affinity, cpu_online_mask);
+	return apic->cpu_mask_to_apicid_and(desc->affinity, cfg->domain);
 }
 
 static void

Re: [RFC] Correct behaviour of irq affinity?

[Yinghai Lu]

On Mon, Mar 23, 2009 at 10:49 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> The effect of setting desc->affinity (ie. from userspace via sysfs) has varied
> over time.  In 2.6.27, the 32-bit code anded the value with cpu_online_map,
> and both 32 and 64-bit did that anding whenever a cpu was unplugged.
>
> 2.6.29 consolidated this into one routine (and fixed hotplug) but introduced
> another variation: anding the affinity with cfg->domain.  Is this right, or
> should we just set it to what the user said?  Or as now, indicate that we're
> restricting it.
>
> If we should change it, here's what the patch looks like against x86 tip
> (cpu_mask_to_apicid_and already takes cpu_online_mask into account):
>
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index 86827d8..30906cd 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -592,10 +592,10 @@ set_desc_affinity(struct irq_desc *desc, const struct cpumask *mask)
>        if (assign_irq_vector(irq, cfg, mask))
>                return BAD_APICID;
>
> -       cpumask_and(desc->affinity, cfg->domain, mask);
> +       cpumask_copy(desc->affinity, mask);
>        set_extra_move_desc(desc, mask);
>
> -       return apic->cpu_mask_to_apicid_and(desc->affinity, cpu_online_mask);
> +       return apic->cpu_mask_to_apicid_and(desc->affinity, cfg->domain);
>  }
>
>  static void
>
cfg->domain for logical flat: will be ALL_CPUS
for phys flat (aka bigsmp on 32bit) will be one cpu set mask.

so desc->affinity: for logical will be not changed, but
set_desc_affinity() return will be changed. ( not add with
cpu_online_mask anymore)
when mask is 0x0f
for phys flat, desc->affinity will be changed to 0x0f from
0x01/0x02/0x04/08, return set_desc_affinity is not changed.
so /proc/irq/xx/smp_affinity will be changed. and it does reflect that
actually affinity.

so this patch looks not right.

YH

Re: [RFC] Correct behaviour of irq affinity?

[Rusty Russell]

On Tuesday 24 March 2009 17:51:43 Yinghai Lu wrote:
> On Mon, Mar 23, 2009 at 10:49 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> > The effect of setting desc->affinity (ie. from userspace via sysfs) has varied
> > over time.  In 2.6.27, the 32-bit code anded the value with cpu_online_map,
> > and both 32 and 64-bit did that anding whenever a cpu was unplugged.
> >
> > 2.6.29 consolidated this into one routine (and fixed hotplug) but introduced
> > another variation: anding the affinity with cfg->domain.  Is this right, or
> > should we just set it to what the user said?  Or as now, indicate that we're
> > restricting it.
> >
> > If we should change it, here's what the patch looks like against x86 tip
> > (cpu_mask_to_apicid_and already takes cpu_online_mask into account):
> >
> > diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> > index 86827d8..30906cd 100644
> > --- a/arch/x86/kernel/apic/io_apic.c
> > +++ b/arch/x86/kernel/apic/io_apic.c
> > @@ -592,10 +592,10 @@ set_desc_affinity(struct irq_desc *desc, const struct cpumask *mask)
> >        if (assign_irq_vector(irq, cfg, mask))
> >                return BAD_APICID;
> >
> > -       cpumask_and(desc->affinity, cfg->domain, mask);
> > +       cpumask_copy(desc->affinity, mask);
> >        set_extra_move_desc(desc, mask);
> >
> > -       return apic->cpu_mask_to_apicid_and(desc->affinity, cpu_online_mask);
> > +       return apic->cpu_mask_to_apicid_and(desc->affinity, cfg->domain);
> >  }
> >
> >  static void
> >
> cfg->domain for logical flat: will be ALL_CPUS
> for phys flat (aka bigsmp on 32bit) will be one cpu set mask.
> 
> so desc->affinity: for logical will be not changed, but
> set_desc_affinity() return will be changed. ( not add with
> cpu_online_mask anymore)

No, internally cpu_mask_to_apicid_and() does and with cpu_online_mask
already, eg in include/asm/bigsmp/apic.h:

static inline unsigned int cpu_mask_to_apicid_and(const struct cpumask *cpumask,
						  const struct cpumask *andmask)
{
	int cpu;

	/*
	 * We're using fixed IRQ delivery, can only return one phys APIC ID.
	 * May as well be the first.
	 */
	for_each_cpu_and(cpu, cpumask, andmask)
		if (cpumask_test_cpu(cpu, cpu_online_mask))
			break;
	if (cpu < nr_cpu_ids)
		return cpu_to_logical_apicid(cpu);

	return BAD_APICID;
}

> when mask is 0x0f
> for phys flat, desc->affinity will be changed to 0x0f from
> 0x01/0x02/0x04/08, return set_desc_affinity is not changed.
> so /proc/irq/xx/smp_affinity will be changed. and it does reflect that
> actually affinity.
> 
> so this patch looks not right.

Only change should be that smp_affinity will reflect actual affinity, not
affinity user set.

Thanks,
Rusty.

Re: [RFC] Correct behaviour of irq affinity?

[Yinghai Lu]

Rusty Russell wrote:
> On Tuesday 24 March 2009 17:51:43 Yinghai Lu wrote:
>> On Mon, Mar 23, 2009 at 10:49 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> The effect of setting desc->affinity (ie. from userspace via sysfs) has varied
>>> over time.  In 2.6.27, the 32-bit code anded the value with cpu_online_map,
>>> and both 32 and 64-bit did that anding whenever a cpu was unplugged.
>>>
>>> 2.6.29 consolidated this into one routine (and fixed hotplug) but introduced
>>> another variation: anding the affinity with cfg->domain.  Is this right, or
>>> should we just set it to what the user said?  Or as now, indicate that we're
>>> restricting it.
>>>
>>> If we should change it, here's what the patch looks like against x86 tip
>>> (cpu_mask_to_apicid_and already takes cpu_online_mask into account):
>>>
>>> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
>>> index 86827d8..30906cd 100644
>>> --- a/arch/x86/kernel/apic/io_apic.c
>>> +++ b/arch/x86/kernel/apic/io_apic.c
>>> @@ -592,10 +592,10 @@ set_desc_affinity(struct irq_desc *desc, const struct cpumask *mask)
>>>        if (assign_irq_vector(irq, cfg, mask))
>>>                return BAD_APICID;
>>>
>>> -       cpumask_and(desc->affinity, cfg->domain, mask);
>>> +       cpumask_copy(desc->affinity, mask);
>>>        set_extra_move_desc(desc, mask);
>>>
>>> -       return apic->cpu_mask_to_apicid_and(desc->affinity, cpu_online_mask);
>>> +       return apic->cpu_mask_to_apicid_and(desc->affinity, cfg->domain);
>>>  }
>>>
>>>  static void
>>>
>> cfg->domain for logical flat: will be ALL_CPUS
>> for phys flat (aka bigsmp on 32bit) will be one cpu set mask.
>>
>> so desc->affinity: for logical will be not changed, but
>> set_desc_affinity() return will be changed. ( not add with
>> cpu_online_mask anymore)
> 
> No, internally cpu_mask_to_apicid_and() does and with cpu_online_mask
> already, eg in include/asm/bigsmp/apic.h:
> 
> static inline unsigned int cpu_mask_to_apicid_and(const struct cpumask *cpumask,
> 						  const struct cpumask *andmask)
> {
> 	int cpu;
> 
> 	/*
> 	 * We're using fixed IRQ delivery, can only return one phys APIC ID.
> 	 * May as well be the first.
> 	 */
> 	for_each_cpu_and(cpu, cpumask, andmask)
> 		if (cpumask_test_cpu(cpu, cpu_online_mask))
> 			break;
> 	if (cpu < nr_cpu_ids)
> 		return cpu_to_logical_apicid(cpu);
> 
> 	return BAD_APICID;
> }
> 
>> when mask is 0x0f
>> for phys flat, desc->affinity will be changed to 0x0f from
>> 0x01/0x02/0x04/08, return set_desc_affinity is not changed.
>> so /proc/irq/xx/smp_affinity will be changed. and it does reflect that
>> actually affinity.
>>
>> so this patch looks not right.
> 
> Only change should be that smp_affinity will reflect actual affinity, not
> affinity user set.

ok.
how about
static unsigned int flat_cpu_mask_to_apicid_and(const struct cpumask *cpumask,
                                                const struct cpumask *andmask)
{
        unsigned long mask1 = cpumask_bits(cpumask)[0] & APIC_ALL_CPUS;
        unsigned long mask2 = cpumask_bits(andmask)[0] & APIC_ALL_CPUS;

        return mask1 & mask2;
}

change it use
default_cpu_mask_to_apicid_and ?

YH

Re: [RFC] Correct behaviour of irq affinity?

[Eric W. Biederman]

Rusty Russell <rusty@rustcorp.com.au> writes:

> The effect of setting desc->affinity (ie. from userspace via sysfs) has varied
> over time.  In 2.6.27, the 32-bit code anded the value with cpu_online_map,
> and both 32 and 64-bit did that anding whenever a cpu was unplugged.
>
> 2.6.29 consolidated this into one routine (and fixed hotplug) but introduced
> another variation: anding the affinity with cfg->domain.  Is this right, or
> should we just set it to what the user said?  Or as now, indicate that we're
> restricting it.
>
> If we should change it, here's what the patch looks like against x86 tip
> (cpu_mask_to_apicid_and already takes cpu_online_mask into account):

desc->affinity should be what the user requested, if it is at all
possible to honor the user space request.  YH the fact that we do not
currently exercise the full freedom that user space gives us is
irrelevant.

Further setting desc->affinity to the user space request is what
x86_64 did before the grand merger. 

Likewise desc->affinity & cfg->domain & cpu_online_map going into the
selection of apic id, is what the code did before the grand merger,
and what the code is currently doing.  So logically that looks good.

YH has a point that several of the implementations of
cpu_mask_to_apic_id do not take cpu_online_map into account and should
probably be fixed.  flat_cpu_mask_to_apicid was the one I could find.

Also now that I look at it there is one other bug in this routine
that you have missed.  set_extra_move_desc should be called before
we set desc->affinity, as it compares that with the new value to
see if we are going to be running on a new cpu, and if so we may
need to reallocate irq_desc onto a new numa node.  set_extra_move_desc
looks a little fishy but it doesn't stand a chance if it is called
with the wrong data.

Overall I like it.

Do you think you could fix those two issues and regenerate the patch?

> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index 86827d8..30906cd 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -592,10 +592,10 @@ set_desc_affinity(struct irq_desc *desc, const struct cpumask *mask)
>  	if (assign_irq_vector(irq, cfg, mask))
>  		return BAD_APICID;
>  
> -	cpumask_and(desc->affinity, cfg->domain, mask);
> +	cpumask_copy(desc->affinity, mask);
>  	set_extra_move_desc(desc, mask);
>  
> -	return apic->cpu_mask_to_apicid_and(desc->affinity, cpu_online_mask);
> +	return apic->cpu_mask_to_apicid_and(desc->affinity, cfg->domain);
>  }
>  
>  static void

Eric

Re: [RFC] Correct behaviour of irq affinity?

[Yinghai Lu]

Eric W. Biederman wrote:
> 
> Also now that I look at it there is one other bug in this routine
> that you have missed.  set_extra_move_desc should be called before
> we set desc->affinity, as it compares that with the new value to
> see if we are going to be running on a new cpu, and if so we may
> need to reallocate irq_desc onto a new numa node.  set_extra_move_desc
> looks a little fishy but it doesn't stand a chance if it is called
> with the wrong data.


you are right, that is introduce bycommit 22f65d31b25a320a5246592160bcb102d2791c45

Author: Mike Travis <travis@sgi.com>
Date:   Tue Dec 16 17:33:56 2008 -0800

    x86: Update io_apic.c to use new cpumask API
    
    Impact: cleanup, consolidate patches, use new API
    
    Consolidate the following into a single patch to adapt to new
    sparseirq code in arch/x86/kernel/io_apic.c, add allocation of
    cpumask_var_t's in domain and old_domain, and reduce further
    merge conflicts.  Only one file (arch/x86/kernel/io_apic.c) is
    changed in all of these patches.
    
        0006-x86-io_apic-change-irq_cfg-domain-old_domain-to.patch
        0007-x86-io_apic-set_desc_affinity.patch
        0008-x86-io_apic-send_cleanup_vector.patch
        0009-x86-io_apic-eliminate-remaining-cpumask_ts-from-st.patch
        0021-x86-final-cleanups-in-io_apic-to-use-new-cpumask-AP.patch

will send one patch for that.

YH

[PATCH] x86: fix set_extra_move_desc calling

[Yinghai Lu]

Impact: fix bug with desc moving when logical flat

Eric pointed out that we should compare with old affinity.
acctually this bug is introduced by:
| commit 22f65d31b25a320a5246592160bcb102d2791c45
|
| Author: Mike Travis <travis@sgi.com>
| Date:   Tue Dec 16 17:33:56 2008 -0800
|
|    x86: Update io_apic.c to use new cpumask API
|
|    Impact: cleanup, consolidate patches, use new API

so need to move that before...

---
 arch/x86/kernel/apic/io_apic.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6/arch/x86/kernel/apic/io_apic.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/io_apic.c
+++ linux-2.6/arch/x86/kernel/apic/io_apic.c
@@ -592,8 +592,9 @@ set_desc_affinity(struct irq_desc *desc,
 	if (assign_irq_vector(irq, cfg, mask))
 		return BAD_APICID;
 
-	cpumask_and(desc->affinity, cfg->domain, mask);
+	/* check that before desc->addinity get updated */
 	set_extra_move_desc(desc, mask);
+	cpumask_and(desc->affinity, cfg->domain, mask);
 
 	return apic->cpu_mask_to_apicid_and(desc->affinity, cpu_online_mask);
 }

[tip:x86/apic] x86: fix set_extra_move_desc calling

[Yinghai Lu]

Commit-ID:  fa74c9073370e57fa28e02aff13f4d7b1806505c
Gitweb:     http://git.kernel.org/tip/fa74c9073370e57fa28e02aff13f4d7b1806505c
Author:     Yinghai Lu <yinghai@kernel.org>
AuthorDate: Tue, 24 Mar 2009 13:23:16 -0700
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 24 Mar 2009 22:12:10 +0100

x86: fix set_extra_move_desc calling

Impact: fix bug with irq-descriptor moving when logical flat

Rusty observed:

> The effect of setting desc->affinity (ie. from userspace via sysfs) has varied
> over time.  In 2.6.27, the 32-bit code anded the value with cpu_online_map,
> and both 32 and 64-bit did that anding whenever a cpu was unplugged.
>
> 2.6.29 consolidated this into one routine (and fixed hotplug) but introduced
> another variation: anding the affinity with cfg->domain.  Is this right, or
> should we just set it to what the user said?  Or as now, indicate that we're
> restricting it.

Eric pointed out that desc->affinity should be what the user requested,
if it is at all possible to honor the user space request.

This bug got introduced by commit 22f65d31b "x86: Update io_apic.c to use
new cpumask API".

Fix it by moving the masking to before the descriptor moving ...

Reported-by: Rusty Russell <rusty@rustcorp.com.au>
Reported-by: Eric W. Biederman <ebiederm@xmission.com>
LKML-Reference: <49C94134.4000408@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/kernel/apic/io_apic.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 86827d8..1ed6c06 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -592,8 +592,9 @@ set_desc_affinity(struct irq_desc *desc, const struct cpumask *mask)
 	if (assign_irq_vector(irq, cfg, mask))
 		return BAD_APICID;
 
-	cpumask_and(desc->affinity, cfg->domain, mask);
+	/* check that before desc->addinity get updated */
 	set_extra_move_desc(desc, mask);
+	cpumask_and(desc->affinity, cfg->domain, mask);
 
 	return apic->cpu_mask_to_apicid_and(desc->affinity, cpu_online_mask);
 }

[PATCH 1/3] x86: fix set_extra_move_desc calling

[Yinghai Lu]

Impact: fix bug with desc moving when logical flat

Eric pointed out that should compare with old desc.
acctually this bug is introduced by:
| commit 22f65d31b25a320a5246592160bcb102d2791c45
|
| Author: Mike Travis <travis@sgi.com>
| Date:   Tue Dec 16 17:33:56 2008 -0800
|
|    x86: Update io_apic.c to use new cpumask API
|
|    Impact: cleanup, consolidate patches, use new API

so need to move that before...

---
 arch/x86/kernel/apic/io_apic.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6/arch/x86/kernel/apic/io_apic.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/io_apic.c
+++ linux-2.6/arch/x86/kernel/apic/io_apic.c
@@ -592,8 +592,9 @@ set_desc_affinity(struct irq_desc *desc,
 	if (assign_irq_vector(irq, cfg, mask))
 		return BAD_APICID;
 
-	cpumask_and(desc->affinity, cfg->domain, mask);
+	/* check that before desc->addinity get updated */
 	set_extra_move_desc(desc, mask);
+	cpumask_and(desc->affinity, cfg->domain, mask);
 
 	return apic->cpu_mask_to_apicid_and(desc->affinity, cpu_online_mask);
 }

[PATCH 2/3] x86: use default_cpu_mask_to_apicid for 64bit

[Yinghai Lu]

Impact: cleanup

use online_mask directly with 64bit too.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 arch/x86/include/asm/apic.h         |   20 ++++++++++----------
 arch/x86/kernel/apic/apic_flat_64.c |   18 ++----------------
 2 files changed, 12 insertions(+), 26 deletions(-)

Index: linux-2.6/arch/x86/include/asm/apic.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/apic.h
+++ linux-2.6/arch/x86/include/asm/apic.h
@@ -489,10 +489,19 @@ static inline int default_apic_id_regist
 	return physid_isset(read_apic_id(), phys_cpu_present_map);
 }
 
+static inline int default_phys_pkg_id(int cpuid_apic, int index_msb)
+{
+	return cpuid_apic >> index_msb;
+}
+
+extern int default_apicid_to_node(int logical_apicid);
+
+#endif
+
 static inline unsigned int
 default_cpu_mask_to_apicid(const struct cpumask *cpumask)
 {
-	return cpumask_bits(cpumask)[0];
+	return cpumask_bits(cpumask)[0] & APIC_ALL_CPUS;
 }
 
 static inline unsigned int
@@ -506,15 +515,6 @@ default_cpu_mask_to_apicid_and(const str
 	return (unsigned int)(mask1 & mask2 & mask3);
 }
 
-static inline int default_phys_pkg_id(int cpuid_apic, int index_msb)
-{
-	return cpuid_apic >> index_msb;
-}
-
-extern int default_apicid_to_node(int logical_apicid);
-
-#endif
-
 static inline unsigned long default_check_apicid_used(physid_mask_t bitmap, int apicid)
 {
 	return physid_isset(apicid, bitmap);
Index: linux-2.6/arch/x86/kernel/apic/apic_flat_64.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/apic_flat_64.c
+++ linux-2.6/arch/x86/kernel/apic/apic_flat_64.c
@@ -159,20 +159,6 @@ static int flat_apic_id_registered(void)
 	return physid_isset(read_xapic_id(), phys_cpu_present_map);
 }
 
-static unsigned int flat_cpu_mask_to_apicid(const struct cpumask *cpumask)
-{
-	return cpumask_bits(cpumask)[0] & APIC_ALL_CPUS;
-}
-
-static unsigned int flat_cpu_mask_to_apicid_and(const struct cpumask *cpumask,
-						const struct cpumask *andmask)
-{
-	unsigned long mask1 = cpumask_bits(cpumask)[0] & APIC_ALL_CPUS;
-	unsigned long mask2 = cpumask_bits(andmask)[0] & APIC_ALL_CPUS;
-
-	return mask1 & mask2;
-}
-
 static int flat_phys_pkg_id(int initial_apic_id, int index_msb)
 {
 	return hard_smp_processor_id() >> index_msb;
@@ -213,8 +199,8 @@ struct apic apic_flat =  {
 	.set_apic_id			= set_apic_id,
 	.apic_id_mask			= 0xFFu << 24,
 
-	.cpu_mask_to_apicid		= flat_cpu_mask_to_apicid,
-	.cpu_mask_to_apicid_and		= flat_cpu_mask_to_apicid_and,
+	.cpu_mask_to_apicid		= default_cpu_mask_to_apicid,
+	.cpu_mask_to_apicid_and		= default_cpu_mask_to_apicid_and,
 
 	.send_IPI_mask			= flat_send_IPI_mask,
 	.send_IPI_mask_allbutself	= flat_send_IPI_mask_allbutself,

[tip:x86/apic] x86: use default_cpu_mask_to_apicid for 64bit

[Yinghai Lu]

Commit-ID:  f56e5034121c4911a155ba907076ab920754626d
Gitweb:     http://git.kernel.org/tip/f56e5034121c4911a155ba907076ab920754626d
Author:     Yinghai Lu <yinghai@kernel.org>
AuthorDate: Tue, 24 Mar 2009 14:16:30 -0700
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 24 Mar 2009 22:28:38 +0100

x86: use default_cpu_mask_to_apicid for 64bit

Impact: cleanup

Use online_mask directly on 64bit too.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
LKML-Reference: <49C94DAE.9070300@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/include/asm/apic.h         |   20 ++++++++++----------
 arch/x86/kernel/apic/apic_flat_64.c |   18 ++----------------
 2 files changed, 12 insertions(+), 26 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 00f5962..130a9e2 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -489,10 +489,19 @@ static inline int default_apic_id_registered(void)
 	return physid_isset(read_apic_id(), phys_cpu_present_map);
 }
 
+static inline int default_phys_pkg_id(int cpuid_apic, int index_msb)
+{
+	return cpuid_apic >> index_msb;
+}
+
+extern int default_apicid_to_node(int logical_apicid);
+
+#endif
+
 static inline unsigned int
 default_cpu_mask_to_apicid(const struct cpumask *cpumask)
 {
-	return cpumask_bits(cpumask)[0];
+	return cpumask_bits(cpumask)[0] & APIC_ALL_CPUS;
 }
 
 static inline unsigned int
@@ -506,15 +515,6 @@ default_cpu_mask_to_apicid_and(const struct cpumask *cpumask,
 	return (unsigned int)(mask1 & mask2 & mask3);
 }
 
-static inline int default_phys_pkg_id(int cpuid_apic, int index_msb)
-{
-	return cpuid_apic >> index_msb;
-}
-
-extern int default_apicid_to_node(int logical_apicid);
-
-#endif
-
 static inline unsigned long default_check_apicid_used(physid_mask_t bitmap, int apicid)
 {
 	return physid_isset(apicid, bitmap);
diff --git a/arch/x86/kernel/apic/apic_flat_64.c b/arch/x86/kernel/apic/apic_flat_64.c
index f933822..0014714 100644
--- a/arch/x86/kernel/apic/apic_flat_64.c
+++ b/arch/x86/kernel/apic/apic_flat_64.c
@@ -159,20 +159,6 @@ static int flat_apic_id_registered(void)
 	return physid_isset(read_xapic_id(), phys_cpu_present_map);
 }
 
-static unsigned int flat_cpu_mask_to_apicid(const struct cpumask *cpumask)
-{
-	return cpumask_bits(cpumask)[0] & APIC_ALL_CPUS;
-}
-
-static unsigned int flat_cpu_mask_to_apicid_and(const struct cpumask *cpumask,
-						const struct cpumask *andmask)
-{
-	unsigned long mask1 = cpumask_bits(cpumask)[0] & APIC_ALL_CPUS;
-	unsigned long mask2 = cpumask_bits(andmask)[0] & APIC_ALL_CPUS;
-
-	return mask1 & mask2;
-}
-
 static int flat_phys_pkg_id(int initial_apic_id, int index_msb)
 {
 	return hard_smp_processor_id() >> index_msb;
@@ -213,8 +199,8 @@ struct apic apic_flat =  {
 	.set_apic_id			= set_apic_id,
 	.apic_id_mask			= 0xFFu << 24,
 
-	.cpu_mask_to_apicid		= flat_cpu_mask_to_apicid,
-	.cpu_mask_to_apicid_and		= flat_cpu_mask_to_apicid_and,
+	.cpu_mask_to_apicid		= default_cpu_mask_to_apicid,
+	.cpu_mask_to_apicid_and		= default_cpu_mask_to_apicid_and,
 
 	.send_IPI_mask			= flat_send_IPI_mask,
 	.send_IPI_mask_allbutself	= flat_send_IPI_mask_allbutself,

Re: [tip:x86/apic] x86: use default_cpu_mask_to_apicid for 64bit

[Ingo Molnar]

build failure:

arch/x86/kernel/apic/io_apic.c:600: error: ‘struct irq_cfg’ has no member named ‘cpu_domain’

	Ingo

[PATCH 3/3] x86: Correct behaviour of irq affinity -v2

[Yinghai Lu]

From: Rusty Russell <rusty@rustcorp.com.au>

Impact: get correct smp_affinity as user requested

The effect of setting desc->affinity (ie. from userspace via sysfs) has varied
over time.  In 2.6.27, the 32-bit code anded the value with cpu_online_map,
and both 32 and 64-bit did that anding whenever a cpu was unplugged.

2.6.29 consolidated this into one routine (and fixed hotplug) but introduced
another variation: anding the affinity with cfg->domain.  Is this right, or
should we just set it to what the user said?  Or as now, indicate that we're
restricting it.

If we should change it, here's what the patch looks like against x86 tip
(cpu_mask_to_apicid_and already takes cpu_online_mask into account)

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

---
 arch/x86/kernel/apic/io_apic.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-2.6/arch/x86/kernel/apic/io_apic.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/io_apic.c
+++ linux-2.6/arch/x86/kernel/apic/io_apic.c
@@ -594,9 +594,10 @@ set_desc_affinity(struct irq_desc *desc,
 
 	/* check that before desc->addinity get updated */
 	set_extra_move_desc(desc, mask);
-	cpumask_and(desc->affinity, cfg->domain, mask);
 
-	return apic->cpu_mask_to_apicid_and(desc->affinity, cpu_online_mask);
+	cpumask_copy(desc->affinity, mask);
+
+	return apic->cpu_mask_to_apicid_and(desc->affinity, cfg->domain);
 }
 
 static void

[PATCH 3/3] x86: Correct behaviour of irq affinity

[Yinghai Lu]

From: Rusty Russell <rusty@rustcorp.com.au>

Impact: get correct smp_affinity as user requested

The effect of setting desc->affinity (ie. from userspace via sysfs) has varied
over time.  In 2.6.27, the 32-bit code anded the value with cpu_online_map,
and both 32 and 64-bit did that anding whenever a cpu was unplugged.

2.6.29 consolidated this into one routine (and fixed hotplug) but introduced
another variation: anding the affinity with cfg->domain.  Is this right, or
should we just set it to what the user said?  Or as now, indicate that we're
restricting it.

If we should change it, here's what the patch looks like against x86 tip
(cpu_mask_to_apicid_and already takes cpu_online_mask into account)

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

---
 arch/x86/kernel/apic/io_apic.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-2.6/arch/x86/kernel/apic/io_apic.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/io_apic.c
+++ linux-2.6/arch/x86/kernel/apic/io_apic.c
@@ -594,9 +594,10 @@ set_desc_affinity(struct irq_desc *desc,
 
 	/* check that before desc->addinity get updated */
 	set_extra_move_desc(desc, mask);
-	cpumask_and(desc->affinity, cfg->domain, mask);
 
-	return apic->cpu_mask_to_apicid_and(desc->affinity, cpu_online_mask);
+	cpumask_copy(desc->affinity, mask);
+
+	return apic->cpu_mask_to_apicid_and(desc->affinity, cfg->cpu_domain);
 }
 
 static void

[tip:x86/apic] x86: Correct behaviour of irq affinity

[Rusty Russell]

Commit-ID:  85a328f27b927e3f42563c76e1a6992068100433
Gitweb:     http://git.kernel.org/tip/85a328f27b927e3f42563c76e1a6992068100433
Author:     Rusty Russell <rusty@rustcorp.com.au>
AuthorDate: Tue, 24 Mar 2009 14:17:19 -0700
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 24 Mar 2009 22:28:39 +0100

x86: Correct behaviour of irq affinity

Impact: get correct smp_affinity as user requested

The effect of setting desc->affinity (ie. from userspace via sysfs) has
varied over time.  In 2.6.27, the 32-bit code anded the value with
cpu_online_map, and both 32 and 64-bit did that anding whenever a cpu
was unplugged.

2.6.29 consolidated this into one routine (and fixed hotplug) but
introduced another variation: anding the affinity with cfg->domain.

We should just set it to what the user said - if possible.

(cpu_mask_to_apicid_and already takes cpu_online_mask into account)

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
LKML-Reference: <49C94DDF.2010703@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/kernel/apic/io_apic.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 1ed6c06..91476aa 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -594,9 +594,10 @@ set_desc_affinity(struct irq_desc *desc, const struct cpumask *mask)
 
 	/* check that before desc->addinity get updated */
 	set_extra_move_desc(desc, mask);
-	cpumask_and(desc->affinity, cfg->domain, mask);
 
-	return apic->cpu_mask_to_apicid_and(desc->affinity, cpu_online_mask);
+	cpumask_copy(desc->affinity, mask);
+
+	return apic->cpu_mask_to_apicid_and(desc->affinity, cfg->cpu_domain);
 }
 
 static void

[tip:x86/apic] x86: Correct behaviour of irq affinity

[Rusty Russell]

Commit-ID:  e06b1b56f9bfcc91e1f175fe8d8bf3e35dafa080
Gitweb:     http://git.kernel.org/tip/e06b1b56f9bfcc91e1f175fe8d8bf3e35dafa080
Author:     Rusty Russell <rusty@rustcorp.com.au>
AuthorDate: Tue, 24 Mar 2009 14:17:19 -0700
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 25 Mar 2009 18:48:29 +0100

x86: Correct behaviour of irq affinity

Impact: get correct smp_affinity as user requested

The effect of setting desc->affinity (ie. from userspace via sysfs) has
varied over time.  In 2.6.27, the 32-bit code anded the value with
cpu_online_map, and both 32 and 64-bit did that anding whenever a cpu
was unplugged.

2.6.29 consolidated this into one routine (and fixed hotplug) but
introduced another variation: anding the affinity with cfg->domain.

We should just set it to what the user said - if possible.

(cpu_mask_to_apicid_and already takes cpu_online_mask into account)

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
LKML-Reference: <49C94DDF.2010703@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/kernel/apic/io_apic.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 1ed6c06..d990408 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -594,9 +594,10 @@ set_desc_affinity(struct irq_desc *desc, const struct cpumask *mask)
 
 	/* check that before desc->addinity get updated */
 	set_extra_move_desc(desc, mask);
-	cpumask_and(desc->affinity, cfg->domain, mask);
 
-	return apic->cpu_mask_to_apicid_and(desc->affinity, cpu_online_mask);
+	cpumask_copy(desc->affinity, mask);
+
+	return apic->cpu_mask_to_apicid_and(desc->affinity, cfg->domain);
 }
 
 static void

Re: [RFC] Correct behaviour of irq affinity?

[Rusty Russell]

On Tuesday 24 March 2009 23:09:37 Eric W. Biederman wrote:
> desc->affinity should be what the user requested, if it is at all
> possible to honor the user space request.  YH the fact that we do not
> currently exercise the full freedom that user space gives us is
> irrelevant.

Yep, OK.

> YH has a point that several of the implementations of
> cpu_mask_to_apic_id do not take cpu_online_map into account and should
> probably be fixed.  flat_cpu_mask_to_apicid was the one I could find.

Also the numaq apic.h.  I'll do an audit and send a patch.

> Also now that I look at it there is one other bug in this routine
> that you have missed.  set_extra_move_desc should be called before
> we set desc->affinity, as it compares that with the new value to
> see if we are going to be running on a new cpu, and if so we may
> need to reallocate irq_desc onto a new numa node.  set_extra_move_desc
> looks a little fishy but it doesn't stand a chance if it is called
> with the wrong data.

Yes, agree with Yinghai's fix.  I'll re-spin my patch on top of his.

Thanks for looking at this!
Rusty.

Re: [RFC] Correct behaviour of irq affinity?

[Rusty Russell]

On Wednesday 25 March 2009 11:03:08 Rusty Russell wrote:
> On Tuesday 24 March 2009 23:09:37 Eric W. Biederman wrote:
> > cpu_mask_to_apic_id do not take cpu_online_map into account and should
> > probably be fixed.  flat_cpu_mask_to_apicid was the one I could find.
> 
> Also the numaq apic.h.  I'll do an audit and send a patch.

Actually, numaq is just weird, so I'll assume it's correct.  I couldn't
find any others.

> Yes, agree with Yinghai's fix.  I'll re-spin my patch on top of his.

But he already did it while I slept in.

Thanks Yinghai!
Rusty.

Re: [RFC] Correct behaviour of irq affinity?

[Yinghai Lu]

Rusty Russell wrote:
> On Wednesday 25 March 2009 11:03:08 Rusty Russell wrote:
>> On Tuesday 24 March 2009 23:09:37 Eric W. Biederman wrote:
>>> cpu_mask_to_apic_id do not take cpu_online_map into account and should
>>> probably be fixed.  flat_cpu_mask_to_apicid was the one I could find.
>> Also the numaq apic.h.  I'll do an audit and send a patch.
> 
> Actually, numaq is just weird, so I'll assume it's correct.  I couldn't
> find any others.
> 
>> Yes, agree with Yinghai's fix.  I'll re-spin my patch on top of his.
> 
> But he already did it while I slept in.
> 

waiting for ingo to put that three into tip/master:

[PATCH 1/3] x86: fix set_extra_move_desc calling
[PATCH 2/3] x86: use default_cpu_mask_to_apicid for 64bit
[PATCH 3/3] x86: Correct behaviour of irq affinity -v2

YH