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 <[email protected]>
Cc: Prarit Bhargava <[email protected]>
---
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)
* Yinghai Lu <[email protected]> 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 <[email protected]>
> Cc: Prarit Bhargava <[email protected]>
>
> ---
> 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
On 01/25/2014 03:02 AM, Ingo Molnar wrote:
>
> * Yinghai Lu <[email protected]> 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 <[email protected]>
>> Cc: Prarit Bhargava <[email protected]>
>>
>> ---
>> 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
* Prarit Bhargava <[email protected]> wrote:
> On 01/25/2014 03:02 AM, Ingo Molnar wrote:
> >
> > * Yinghai Lu <[email protected]> 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 <[email protected]>
> >> Cc: Prarit Bhargava <[email protected]>
> >>
> >> ---
> >> 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
On 01/26/2014 08:32 AM, Ingo Molnar wrote:
>
> * Prarit Bhargava <[email protected]> wrote:
>
>> On 01/25/2014 03:02 AM, Ingo Molnar wrote:
>>>
>>> * Yinghai Lu <[email protected]> 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 <[email protected]>
>>>> Cc: Prarit Bhargava <[email protected]>
>>>>
>>>> ---
>>>> 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.
* Prarit Bhargava <[email protected]> wrote:
>
>
> On 01/26/2014 08:32 AM, Ingo Molnar wrote:
> >
> > * Prarit Bhargava <[email protected]> wrote:
> >
> >> On 01/25/2014 03:02 AM, Ingo Molnar wrote:
> >>>
> >>> * Yinghai Lu <[email protected]> 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 <[email protected]>
> >>>> Cc: Prarit Bhargava <[email protected]>
> >>>>
> >>>> ---
> >>>> 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
s/global/static/, with a big loud comment why it is okay.
Ingo Molnar <[email protected]> wrote:
>
>* Prarit Bhargava <[email protected]> wrote:
>
>>
>>
>> On 01/26/2014 08:32 AM, Ingo Molnar wrote:
>> >
>> > * Prarit Bhargava <[email protected]> wrote:
>> >
>> >> On 01/25/2014 03:02 AM, Ingo Molnar wrote:
>> >>>
>> >>> * Yinghai Lu <[email protected]> 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 <[email protected]>
>> >>>> Cc: Prarit Bhargava <[email protected]>
>> >>>>
>> >>>> ---
>> >>>> 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.
* H. Peter Anvin <[email protected]> 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
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 <[email protected]> wrote:
>
>* H. Peter Anvin <[email protected]> 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.
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 <[email protected]> wrote:
>>
>> * H. Peter Anvin <[email protected]> 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
>
* H. Peter Anvin <[email protected]> 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