[PATCH -v2] x86: allocate cpumask during check irq vectors

[PATCH -v2] x86: allocate cpumask during check irq vectors

[Yinghai Lu]

Fix warning:
arch/x86/kernel/irq.c: In function check_irq_vectors_for_cpu_disable:
arch/x86/kernel/irq.c:337:1: warning: the frame size of 2052 bytes is larger than 2048 bytes

when NR_CPUS=8192

We should use zalloc_cpumask_var() instead.

-v2: update to GFP_ATOMIC instead and free the allocated cpumask at last.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Prarit Bhargava <prarit@redhat.com>

---
 arch/x86/kernel/irq.c |   24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

Index: linux-2.6/arch/x86/kernel/irq.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/irq.c
+++ linux-2.6/arch/x86/kernel/irq.c
@@ -277,11 +277,18 @@ int check_irq_vectors_for_cpu_disable(vo
 	unsigned int this_cpu, vector, this_count, count;
 	struct irq_desc *desc;
 	struct irq_data *data;
-	struct cpumask affinity_new, online_new;
+	cpumask_var_t affinity_new, online_new;
+
+	if (!alloc_cpumask_var(&affinity_new, GFP_ATOMIC))
+		return -ENOMEM;
+	if (!alloc_cpumask_var(&online_new, GFP_ATOMIC)) {
+		free_cpumask_var(affinity_new);
+		return -ENOMEM;
+	}
 
 	this_cpu = smp_processor_id();
-	cpumask_copy(&online_new, cpu_online_mask);
-	cpu_clear(this_cpu, online_new);
+	cpumask_copy(online_new, cpu_online_mask);
+	cpumask_clear_cpu(this_cpu, online_new);
 
 	this_count = 0;
 	for (vector = FIRST_EXTERNAL_VECTOR; vector < NR_VECTORS; vector++) {
@@ -289,8 +296,8 @@ int check_irq_vectors_for_cpu_disable(vo
 		if (irq >= 0) {
 			desc = irq_to_desc(irq);
 			data = irq_desc_get_irq_data(desc);
-			cpumask_copy(&affinity_new, data->affinity);
-			cpu_clear(this_cpu, affinity_new);
+			cpumask_copy(affinity_new, data->affinity);
+			cpumask_clear_cpu(this_cpu, affinity_new);
 
 			/* Do not count inactive or per-cpu irqs. */
 			if (!irq_has_action(irq) || irqd_is_per_cpu(data))
@@ -311,12 +318,15 @@ int check_irq_vectors_for_cpu_disable(vo
 			 * mask is not zero; that is the down'd cpu is the
 			 * last online cpu in a user set affinity mask.
 			 */
-			if (cpumask_empty(&affinity_new) ||
-			    !cpumask_subset(&affinity_new, &online_new))
+			if (cpumask_empty(affinity_new) ||
+			    !cpumask_subset(affinity_new, online_new))
 				this_count++;
 		}
 	}
 
+	free_cpumask_var(affinity_new);
+	free_cpumask_var(online_new);
+
 	count = 0;
 	for_each_online_cpu(cpu) {
 		if (cpu == this_cpu)

Re: [PATCH -v2] x86: allocate cpumask during check irq vectors

[Ingo Molnar]

* Yinghai Lu <yinghai@kernel.org> wrote:

> Fix warning:
> arch/x86/kernel/irq.c: In function check_irq_vectors_for_cpu_disable:
> arch/x86/kernel/irq.c:337:1: warning: the frame size of 2052 bytes is larger than 2048 bytes
> 
> when NR_CPUS=8192
> 
> We should use zalloc_cpumask_var() instead.
> 
> -v2: update to GFP_ATOMIC instead and free the allocated cpumask at last.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Prarit Bhargava <prarit@redhat.com>
> 
> ---
>  arch/x86/kernel/irq.c |   24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> Index: linux-2.6/arch/x86/kernel/irq.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/irq.c
> +++ linux-2.6/arch/x86/kernel/irq.c
> @@ -277,11 +277,18 @@ int check_irq_vectors_for_cpu_disable(vo
>  	unsigned int this_cpu, vector, this_count, count;
>  	struct irq_desc *desc;
>  	struct irq_data *data;
> -	struct cpumask affinity_new, online_new;
> +	cpumask_var_t affinity_new, online_new;
> +
> +	if (!alloc_cpumask_var(&affinity_new, GFP_ATOMIC))
> +		return -ENOMEM;
> +	if (!alloc_cpumask_var(&online_new, GFP_ATOMIC)) {
> +		free_cpumask_var(affinity_new);
> +		return -ENOMEM;
> +	}

Atomic allocations can fail easily if the system is under duress.

Thanks,

	Ingo

Re: [PATCH -v2] x86: allocate cpumask during check irq vectors

[Prarit Bhargava]

On 01/25/2014 03:02 AM, Ingo Molnar wrote:
> 
> * Yinghai Lu <yinghai@kernel.org> wrote:
> 
>> Fix warning:
>> arch/x86/kernel/irq.c: In function check_irq_vectors_for_cpu_disable:
>> arch/x86/kernel/irq.c:337:1: warning: the frame size of 2052 bytes is larger than 2048 bytes
>>
>> when NR_CPUS=8192
>>
>> We should use zalloc_cpumask_var() instead.
>>
>> -v2: update to GFP_ATOMIC instead and free the allocated cpumask at last.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> Cc: Prarit Bhargava <prarit@redhat.com>
>>
>> ---
>>  arch/x86/kernel/irq.c |   24 +++++++++++++++++-------
>>  1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> Index: linux-2.6/arch/x86/kernel/irq.c
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/kernel/irq.c
>> +++ linux-2.6/arch/x86/kernel/irq.c
>> @@ -277,11 +277,18 @@ int check_irq_vectors_for_cpu_disable(vo
>>  	unsigned int this_cpu, vector, this_count, count;
>>  	struct irq_desc *desc;
>>  	struct irq_data *data;
>> -	struct cpumask affinity_new, online_new;
>> +	cpumask_var_t affinity_new, online_new;
>> +
>> +	if (!alloc_cpumask_var(&affinity_new, GFP_ATOMIC))
>> +		return -ENOMEM;
>> +	if (!alloc_cpumask_var(&online_new, GFP_ATOMIC)) {
>> +		free_cpumask_var(affinity_new);
>> +		return -ENOMEM;
>> +	}
> 
> Atomic allocations can fail easily if the system is under duress.

Then the hotplug attempt will fail which IMO is okay.  With GFP_KERNEL we might
hang the system.

Ingo -- the original submit (and a Reviewed-by: Cheng Gong) is here,

http://marc.info/?l=linux-kernel&m=139035727501266&w=2

Thanks,

P.

> 
> Thanks,
> 
> 	Ingo

Re: [PATCH -v2] x86: allocate cpumask during check irq vectors

[Ingo Molnar]

* Prarit Bhargava <prarit@redhat.com> wrote:

> On 01/25/2014 03:02 AM, Ingo Molnar wrote:
> > 
> > * Yinghai Lu <yinghai@kernel.org> wrote:
> > 
> >> Fix warning:
> >> arch/x86/kernel/irq.c: In function check_irq_vectors_for_cpu_disable:
> >> arch/x86/kernel/irq.c:337:1: warning: the frame size of 2052 bytes is larger than 2048 bytes
> >>
> >> when NR_CPUS=8192
> >>
> >> We should use zalloc_cpumask_var() instead.
> >>
> >> -v2: update to GFP_ATOMIC instead and free the allocated cpumask at last.
> >>
> >> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >> Cc: Prarit Bhargava <prarit@redhat.com>
> >>
> >> ---
> >>  arch/x86/kernel/irq.c |   24 +++++++++++++++++-------
> >>  1 file changed, 17 insertions(+), 7 deletions(-)
> >>
> >> Index: linux-2.6/arch/x86/kernel/irq.c
> >> ===================================================================
> >> --- linux-2.6.orig/arch/x86/kernel/irq.c
> >> +++ linux-2.6/arch/x86/kernel/irq.c
> >> @@ -277,11 +277,18 @@ int check_irq_vectors_for_cpu_disable(vo
> >>  	unsigned int this_cpu, vector, this_count, count;
> >>  	struct irq_desc *desc;
> >>  	struct irq_data *data;
> >> -	struct cpumask affinity_new, online_new;
> >> +	cpumask_var_t affinity_new, online_new;
> >> +
> >> +	if (!alloc_cpumask_var(&affinity_new, GFP_ATOMIC))
> >> +		return -ENOMEM;
> >> +	if (!alloc_cpumask_var(&online_new, GFP_ATOMIC)) {
> >> +		free_cpumask_var(affinity_new);
> >> +		return -ENOMEM;
> >> +	}
> > 
> > Atomic allocations can fail easily if the system is under duress.
> 
> Then the hotplug attempt will fail which IMO is okay. [...]

Which is not OK at all for reliable operation, if the system has 
otherwise gobs of RAM, which just don't happen to be atomic 
allocatable!

> [...]  With GFP_KERNEL we might hang the system.

Yes, that's a bug that should be fixed - but not via GFP_ATOMIC, which 
adds insult to injury.

Thanks,

	Ingo

Re: [PATCH -v2] x86: allocate cpumask during check irq vectors

[Prarit Bhargava]

On 01/26/2014 08:32 AM, Ingo Molnar wrote:
> 
> * Prarit Bhargava <prarit@redhat.com> wrote:
> 
>> On 01/25/2014 03:02 AM, Ingo Molnar wrote:
>>>
>>> * Yinghai Lu <yinghai@kernel.org> wrote:
>>>
>>>> Fix warning:
>>>> arch/x86/kernel/irq.c: In function check_irq_vectors_for_cpu_disable:
>>>> arch/x86/kernel/irq.c:337:1: warning: the frame size of 2052 bytes is larger than 2048 bytes
>>>>
>>>> when NR_CPUS=8192
>>>>
>>>> We should use zalloc_cpumask_var() instead.
>>>>
>>>> -v2: update to GFP_ATOMIC instead and free the allocated cpumask at last.
>>>>
>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>>> Cc: Prarit Bhargava <prarit@redhat.com>
>>>>
>>>> ---
>>>>  arch/x86/kernel/irq.c |   24 +++++++++++++++++-------
>>>>  1 file changed, 17 insertions(+), 7 deletions(-)
>>>>
>>>> Index: linux-2.6/arch/x86/kernel/irq.c
>>>> ===================================================================
>>>> --- linux-2.6.orig/arch/x86/kernel/irq.c
>>>> +++ linux-2.6/arch/x86/kernel/irq.c
>>>> @@ -277,11 +277,18 @@ int check_irq_vectors_for_cpu_disable(vo
>>>>  	unsigned int this_cpu, vector, this_count, count;
>>>>  	struct irq_desc *desc;
>>>>  	struct irq_data *data;
>>>> -	struct cpumask affinity_new, online_new;
>>>> +	cpumask_var_t affinity_new, online_new;
>>>> +
>>>> +	if (!alloc_cpumask_var(&affinity_new, GFP_ATOMIC))
>>>> +		return -ENOMEM;
>>>> +	if (!alloc_cpumask_var(&online_new, GFP_ATOMIC)) {
>>>> +		free_cpumask_var(affinity_new);
>>>> +		return -ENOMEM;
>>>> +	}
>>>
>>> Atomic allocations can fail easily if the system is under duress.
>>
>> Then the hotplug attempt will fail which IMO is okay. [...]
> 
> Which is not OK at all for reliable operation, if the system has 
> otherwise gobs of RAM, which just don't happen to be atomic 
> allocatable!

Ingo, I'm really not sure what other option there is here.  Care to suggest one?

P.

Re: [PATCH -v2] x86: allocate cpumask during check irq vectors

[Ingo Molnar]

* Prarit Bhargava <prarit@redhat.com> wrote:

> 
> 
> On 01/26/2014 08:32 AM, Ingo Molnar wrote:
> > 
> > * Prarit Bhargava <prarit@redhat.com> wrote:
> > 
> >> On 01/25/2014 03:02 AM, Ingo Molnar wrote:
> >>>
> >>> * Yinghai Lu <yinghai@kernel.org> wrote:
> >>>
> >>>> Fix warning:
> >>>> arch/x86/kernel/irq.c: In function check_irq_vectors_for_cpu_disable:
> >>>> arch/x86/kernel/irq.c:337:1: warning: the frame size of 2052 bytes is larger than 2048 bytes
> >>>>
> >>>> when NR_CPUS=8192
> >>>>
> >>>> We should use zalloc_cpumask_var() instead.
> >>>>
> >>>> -v2: update to GFP_ATOMIC instead and free the allocated cpumask at last.
> >>>>
> >>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >>>> Cc: Prarit Bhargava <prarit@redhat.com>
> >>>>
> >>>> ---
> >>>>  arch/x86/kernel/irq.c |   24 +++++++++++++++++-------
> >>>>  1 file changed, 17 insertions(+), 7 deletions(-)
> >>>>
> >>>> Index: linux-2.6/arch/x86/kernel/irq.c
> >>>> ===================================================================
> >>>> --- linux-2.6.orig/arch/x86/kernel/irq.c
> >>>> +++ linux-2.6/arch/x86/kernel/irq.c
> >>>> @@ -277,11 +277,18 @@ int check_irq_vectors_for_cpu_disable(vo
> >>>>  	unsigned int this_cpu, vector, this_count, count;
> >>>>  	struct irq_desc *desc;
> >>>>  	struct irq_data *data;
> >>>> -	struct cpumask affinity_new, online_new;
> >>>> +	cpumask_var_t affinity_new, online_new;
> >>>> +
> >>>> +	if (!alloc_cpumask_var(&affinity_new, GFP_ATOMIC))
> >>>> +		return -ENOMEM;
> >>>> +	if (!alloc_cpumask_var(&online_new, GFP_ATOMIC)) {
> >>>> +		free_cpumask_var(affinity_new);
> >>>> +		return -ENOMEM;
> >>>> +	}
> >>>
> >>> Atomic allocations can fail easily if the system is under duress.
> >>
> >> Then the hotplug attempt will fail which IMO is okay. [...]
> > 
> > Which is not OK at all for reliable operation, if the system has 
> > otherwise gobs of RAM, which just don't happen to be atomic 
> > allocatable!
> 
> Ingo, I'm really not sure what other option there is here.  Care to 
> suggest one?

Since only ever a single instance of this code will run, can it simply 
be a global cpumask_t variable?

Thanks,

	Ingo

Re: [PATCH -v2] x86: allocate cpumask during check irq vectors

[H. Peter Anvin]

s/global/static/, with a big loud comment why it is okay.

Ingo Molnar <mingo@kernel.org> wrote:
>
>* Prarit Bhargava <prarit@redhat.com> wrote:
>
>> 
>> 
>> On 01/26/2014 08:32 AM, Ingo Molnar wrote:
>> > 
>> > * Prarit Bhargava <prarit@redhat.com> wrote:
>> > 
>> >> On 01/25/2014 03:02 AM, Ingo Molnar wrote:
>> >>>
>> >>> * Yinghai Lu <yinghai@kernel.org> wrote:
>> >>>
>> >>>> Fix warning:
>> >>>> arch/x86/kernel/irq.c: In function
>check_irq_vectors_for_cpu_disable:
>> >>>> arch/x86/kernel/irq.c:337:1: warning: the frame size of 2052
>bytes is larger than 2048 bytes
>> >>>>
>> >>>> when NR_CPUS=8192
>> >>>>
>> >>>> We should use zalloc_cpumask_var() instead.
>> >>>>
>> >>>> -v2: update to GFP_ATOMIC instead and free the allocated cpumask
>at last.
>> >>>>
>> >>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> >>>> Cc: Prarit Bhargava <prarit@redhat.com>
>> >>>>
>> >>>> ---
>> >>>>  arch/x86/kernel/irq.c |   24 +++++++++++++++++-------
>> >>>>  1 file changed, 17 insertions(+), 7 deletions(-)
>> >>>>
>> >>>> Index: linux-2.6/arch/x86/kernel/irq.c
>> >>>>
>===================================================================
>> >>>> --- linux-2.6.orig/arch/x86/kernel/irq.c
>> >>>> +++ linux-2.6/arch/x86/kernel/irq.c
>> >>>> @@ -277,11 +277,18 @@ int check_irq_vectors_for_cpu_disable(vo
>> >>>>  	unsigned int this_cpu, vector, this_count, count;
>> >>>>  	struct irq_desc *desc;
>> >>>>  	struct irq_data *data;
>> >>>> -	struct cpumask affinity_new, online_new;
>> >>>> +	cpumask_var_t affinity_new, online_new;
>> >>>> +
>> >>>> +	if (!alloc_cpumask_var(&affinity_new, GFP_ATOMIC))
>> >>>> +		return -ENOMEM;
>> >>>> +	if (!alloc_cpumask_var(&online_new, GFP_ATOMIC)) {
>> >>>> +		free_cpumask_var(affinity_new);
>> >>>> +		return -ENOMEM;
>> >>>> +	}
>> >>>
>> >>> Atomic allocations can fail easily if the system is under duress.
>> >>
>> >> Then the hotplug attempt will fail which IMO is okay. [...]
>> > 
>> > Which is not OK at all for reliable operation, if the system has 
>> > otherwise gobs of RAM, which just don't happen to be atomic 
>> > allocatable!
>> 
>> Ingo, I'm really not sure what other option there is here.  Care to 
>> suggest one?
>
>Since only ever a single instance of this code will run, can it simply 
>be a global cpumask_t variable?
>
>Thanks,
>
>	Ingo

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Re: [PATCH -v2] x86: allocate cpumask during check irq vectors

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> s/global/static/, with a big loud comment why it is okay.

It would be a global no matter which form we use, but for 
maintainability reasons I generally prefer a static put right before 
the function that uses it:

	static cpumask_t mask;

	static func(...)
	{
	}

That makes it really apparent that it's a global - statics are easily 
missed when hiding amongst local variables.

Thanks,

	Ingo

Re: [PATCH -v2] x86: allocate cpumask during check irq vectors

[H. Peter Anvin]

I strongly disagree with putting variables in file scope when function scope will do, but I do like to see static variables before automatics.  Anyway, this is bikeshedding.

Ingo Molnar <mingo@kernel.org> wrote:
>
>* H. Peter Anvin <hpa@zytor.com> wrote:
>
>> s/global/static/, with a big loud comment why it is okay.
>
>It would be a global no matter which form we use, but for 
>maintainability reasons I generally prefer a static put right before 
>the function that uses it:
>
>	static cpumask_t mask;
>
>	static func(...)
>	{
>	}
>
>That makes it really apparent that it's a global - statics are easily 
>missed when hiding amongst local variables.
>
>Thanks,
>
>	Ingo

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Re: [PATCH -v2] x86: allocate cpumask during check irq vectors

[Prarit Bhargava]

On 01/26/2014 03:29 PM, H. Peter Anvin wrote:
> I strongly disagree with putting variables in file scope when function scope will do, but I do like to see static variables before automatics.  Anyway, this is bikeshedding.
> 
> Ingo Molnar <mingo@kernel.org> wrote:
>>
>> * H. Peter Anvin <hpa@zytor.com> wrote:
>>
>>> s/global/static/, with a big loud comment why it is okay.
>>
>> It would be a global no matter which form we use, but for 
>> maintainability reasons I generally prefer a static put right before 
>> the function that uses it:
>>
>> 	static cpumask_t mask;
>>
>> 	static func(...)
>> 	{
>> 	}
>>
>> That makes it really apparent that it's a global - statics are easily 
>> missed when hiding amongst local variables.

Okay, thanks for the input -- I'll put something together and test.

P.

>>
>> Thanks,
>>
>> 	Ingo
> 

Re: [PATCH -v2] x86: allocate cpumask during check irq vectors

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> I strongly disagree with putting variables in file scope when 
> function scope will do, [...]

Yes, you are right that single-use file scope statics 'could' be moved 
function local and are syntactically superior because in that case 
other functions cannot make use of it.

But I also have very good (and unfixable and thus stronger) reasons to 
object to statics inside local variables: more than once I personally 
missed 'hidden statics' during review, in one case it even slipped 
into a commit, so it's not a practice I want to encourage in any shape 
or form (even if the 'rule' is to have a big fat comment, people will 
just see the function local static and emulate it without the 
comment), for code I maintain.

It's not about you, it's about me and other reviewers: I've seen 
statics slipping past other reviewers as well. So it's the lesser of 
two evils. Can you accept that reasoning?

Thanks,

	Ingo