login
Search
Use a nested level for 'for' cycle and break long lines.
For apic_print we should countinue using KERNEL_DEBUG if
we have started to.
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
Index: linux-2.6.git/arch/x86/kernel/io_apic.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/io_apic.c 2008-09-04 22:08:06.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/io_apic.c 2008-09-04 22:08:32.000000000 +0400
@@ -1521,32 +1521,35 @@ static void __init setup_IO_APIC_irqs(vo
apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
for (apic = 0; apic < nr_ioapics; apic++) {
- for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
+ for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
- idx = find_irq_entry(apic,pin,mp_INT);
- if (idx == -1) {
- if (first_notcon) {
- apic_printk(APIC_VERBOSE, KERN_DEBUG " IO-APIC (apicid-pin) %d-%d", mp_ioapics[apic].mp_apicid, pin);
- first_notcon = 0;
- } else
- apic_printk(APIC_VERBOSE, ", %d-%d", mp_ioapics[apic].mp_apicid, pin);
- continue;
- }
- if (!first_notcon) {
- apic_printk(APIC_VERBOSE, " not connected.\n");
- first_notcon = 1;
- }
+ idx = find_irq_entry(apic, pin, mp_INT);
+ if (idx == -1) {
+ if (first_notcon) {
+ apic_printk(APIC_VERBOSE, KERN_DEBUG
+ " IO-APIC (apicid-pin) %d-%d",
+ mp_ioapics[apic].mp_apicid, pin);
+ first_notcon = 0;
+ } else
+ apic_printk(APIC_VERBOSE, KERN_DEBUG ", %d-%d",
+ mp_ioapics[apic].mp_apicid, pin);
+ continue;
+ }
+ if (!first_notcon) {
+ apic_printk(APIC_VERBOSE, " not connected.\n");
+ first_notcon = 1;
+ }
- irq = pin_2_irq(idx, apic, pin);
+ irq = pin_2_irq(idx, apic, pin);
#ifdef CONFIG_X86_32
- if (multi_timer_check(apic, irq))
- continue;
+ if (multi_timer_check(apic, irq))
+ continue;
#endif
- add_pin_to_irq(irq, apic, pin);
+ add_pin_to_irq(irq, apic, pin);
...hm, i dont really like the super-deep nesting we do here. Could you please split out the iterator into a separate function? That makes the code a lot easier to understand and saves two extra tabs as well for those ugly-looking printk lines. Ingo --
[Ingo Molnar - Fri, Sep 05, 2008 at 10:04:47AM +0200]
|
| * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
|
| > Use a nested level for 'for' cycle and break long lines.
| > For apic_print we should countinue using KERNEL_DEBUG if
| > we have started to.
|
| > @@ -1521,32 +1521,35 @@ static void __init setup_IO_APIC_irqs(vo
| > apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
| >
| > for (apic = 0; apic < nr_ioapics; apic++) {
| > - for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
| > + for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
| >
| > + idx = find_irq_entry(apic, pin, mp_INT);
| > + if (idx == -1) {
|
| hm, i dont really like the super-deep nesting we do here. Could you
| please split out the iterator into a separate function? That makes the
| code a lot easier to understand and saves two extra tabs as well for
| those ugly-looking printk lines.
|
| Ingo
|
ok Ingo, will take a look, thanks
- Cyrill -
--
[Ingo Molnar - Fri, Sep 05, 2008 at 10:04:47AM +0200]
|
| * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
|
| > Use a nested level for 'for' cycle and break long lines.
| > For apic_print we should countinue using KERNEL_DEBUG if
| > we have started to.
|
| > @@ -1521,32 +1521,35 @@ static void __init setup_IO_APIC_irqs(vo
| > apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
| >
| > for (apic = 0; apic < nr_ioapics; apic++) {
| > - for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
| > + for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
| >
| > + idx = find_irq_entry(apic, pin, mp_INT);
| > + if (idx == -1) {
|
| hm, i dont really like the super-deep nesting we do here. Could you
| please split out the iterator into a separate function? That makes the
| code a lot easier to understand and saves two extra tabs as well for
| those ugly-looking printk lines.
|
| Ingo
|
You know it seems we use such a 'cycle on every pin on io-apics'
in several places for now:
io_apic.c
---------
clear_IO_APIC
save_mask_IO_APIC_setup
restore_IO_APIC_setup
IO_APIC_irq_trigger
setup_IO_APIC_irqs
I've made a one-line macro for this (like for_all_ioapics_pins)
_but_ it looks much more ugly then this two nested for(;;) :)
If you meant me to make a separate iterator over the pins I think
it will not help a lot - this function is simple enought so the only
problem is too-long-printk-form - maybe just print them on separated
lines instead of tracking apicids? Or it was made in a sake to not
scroll screen too much?
- Cyrill -
--
hm, by iterator i meant the body itself. I.e. something like:
static void __init setup_IO_APIC_irqs(void)
{
int apic, pin, notcon = 1;
apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
for (apic = 0; apic < nr_ioapics; apic++)
for (pin = 0; pin < nr_ioapic_registers[apic]; pin++)
notcon = setup_ioapic_irq(apic, pin, notcon);
if (!notcon)
apic_printk(APIC_VERBOSE, " not connected.\n");
}
this looks quite a bit cleaner, doesnt it? We lose the 'idx' and 'irq'
variables and we lose the curly braces as well. The flow looks a lot
more trivial. And the new setup_ioapic_irq() function will be simpler as
well - it will only have 'idx' and 'irq' as a local variable, the rest
comes in as a parameter. It can 'return notcon' instead of 'continue'.
And it will be 2 levels of tabs aligned to the left, as an added bonus.
Hm?
Ingo
--
[Ingo Molnar - Fri, Sep 05, 2008 at 08:11:11PM +0200]
|
| * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
|
| > [Ingo Molnar - Fri, Sep 05, 2008 at 10:04:47AM +0200]
| > |
| > | * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| > |
| > | > Use a nested level for 'for' cycle and break long lines.
| > | > For apic_print we should countinue using KERNEL_DEBUG if
| > | > we have started to.
| > |
| > | > @@ -1521,32 +1521,35 @@ static void __init setup_IO_APIC_irqs(vo
| > | > apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
| > | >
| > | > for (apic = 0; apic < nr_ioapics; apic++) {
| > | > - for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
| > | > + for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
| > | >
| > | > + idx = find_irq_entry(apic, pin, mp_INT);
| > | > + if (idx == -1) {
| > |
| > | hm, i dont really like the super-deep nesting we do here. Could you
| > | please split out the iterator into a separate function? That makes the
| > | code a lot easier to understand and saves two extra tabs as well for
| > | those ugly-looking printk lines.
| > |
| > | Ingo
| > |
| >
| > You know it seems we use such a 'cycle on every pin on io-apics'
| > in several places for now:
| >
| > io_apic.c
| > ---------
| > clear_IO_APIC
| > save_mask_IO_APIC_setup
| > restore_IO_APIC_setup
| > IO_APIC_irq_trigger
| > setup_IO_APIC_irqs
| >
| > I've made a one-line macro for this (like for_all_ioapics_pins)
| > _but_ it looks much more ugly then this two nested for(;;) :)
| >
| > If you meant me to make a separate iterator over the pins I think
| > it will not help a lot - this function is simple enought so the only
| > problem is too-long-printk-form - maybe just print them on separated
| > lines instead of tracking apicids? Or it was made in a sake to not
| > scroll screen too much?
|
| hm, by iterator i meant the body itself. I.e. something like:
|
| static void __init setup_IO_APIC_irqs(void)
| {
| int ...the compiler will inline that single-called function just fine, as long as you declare it static. spreading printouts over several functions isnt all that bad i think. We could even drop the 'not connected' complication - it's evident enough from the fact that nothing gets printed. Ingo --
[Ingo Molnar - Fri, Sep 05, 2008 at 08:38:35PM +0200] | | * Cyrill Gorcunov <gorcunov@gmail.com> wrote: | | > So as you see it's more then enough self-solid :) So I wouldn't break | > it 'cause of printing. If we have enough memory for bit field - we | > could just mark there if pin is connected to irq and print connection | > map after. Don't get me wrong please - I just don't want to overload | > this function with additional call. | | the compiler will inline that single-called function just fine, as long | as you declare it static. | | spreading printouts over several functions isnt all that bad i think. We | could even drop the 'not connected' complication - it's evident enough | from the fact that nothing gets printed. | | Ingo | Ingo, lets leave it as is for now :) (we cshouldn't drop these messages since they show us what exactly is wrong with MP table) - Cyrill - --
[Ingo Molnar - Fri, Sep 05, 2008 at 08:38:35PM +0200]
|
| * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
|
| > So as you see it's more then enough self-solid :) So I wouldn't break
| > it 'cause of printing. If we have enough memory for bit field - we
| > could just mark there if pin is connected to irq and print connection
| > map after. Don't get me wrong please - I just don't want to overload
| > this function with additional call.
|
| the compiler will inline that single-called function just fine, as long
| as you declare it static.
|
| spreading printouts over several functions isnt all that bad i think. We
| could even drop the 'not connected' complication - it's evident enough
| from the fact that nothing gets printed.
|
| Ingo
|
Ingo, how about the following approach? We don't introduce new
functions but rather srink the code by new printout form.
- Cyrill -
---
From: Cyrill Gorcunov <gorcunov@gmail.com>
Subject: [PATCH] x86: io-apic - setup_IO_APIC_irqs code simplification
By changing printout form we are able to shrink code a bit.
Former printout example:
init IO_APIC IRQs
IO-APIC (apicid-pin) 1-1, 1-2, 1-3 not connected.
IO-APIC (apicid-pin) 2-1, 2-2, 2-3 not connected.
New printout example:
init IO_APIC IRQs
1-1 1-2 1-3 (apicid-pin) not connected
2-1 2-2 2-3 (apicid-pin) not connected
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
Index: linux-2.6.git/arch/x86/kernel/io_apic.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/io_apic.c 2008-09-06 13:46:23.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/io_apic.c 2008-09-06 14:08:13.000000000 +0400
@@ -1516,41 +1516,44 @@ static void setup_IO_APIC_irq(int apic,
static void __init setup_IO_APIC_irqs(void)
{
- int apic, pin, idx, irq, first_notcon = 1;
+ int apic, pin, idx, irq;
+ int notcon = 0;
apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
for (apic = 0; apic < ...applied to tip/irq/sparseirq - thanks Cyrill. Ingo --
got 0-16<7> 0-17<7> 0-18<7> 0-19<7> 0-20<7> 0-21<7> 0-22<7> 0-23<7> (apicid-pin) not connected 1-0<7> 1-1<7> 1-2<7> 1-3<7> 1-4<7> 1-5<7> 1-6<7> 1-7<7> 1-8<7> 1-9<7> 1-10<7> 1-11<7> 1-12<7> 1-13<7> 1-14<7> 1-15<7> 1-16<7> 1-17<7> 1-18<7> 1-19<7> 1-20<7> 1-21<7> 1-22<7> 1-23<7> (apicid-pin) not connected can you remove the extra <7>? YH --
Thanks Yinghai! The patch enveloped. I'm overzealous with KERN_DEBUG marker. Sorry. (Can't send it inlined - hope it's not a big problem) Cyrill
Btw Yinghai, do you really have a machine with that many unconnected pins? --
Yes recent change in sparseirq, we only init irq [0,16) at that point. so pins on other io apic controller all unconnected... YH --
correction: sparseirq does not change the behavoir... ENABLING IO-APIC IRQs init IO_APIC IRQs IO-APIC (apicid-pin) 0-0 not connected. IOAPIC[0]: Set routing entry (0-1 -> 0x31 -> IRQ 1 Mode:0 Active:0) IOAPIC[0]: Set routing entry (0-2 -> 0x30 -> IRQ 0 Mode:0 Active:0) IOAPIC[0]: Set routing entry (0-3 -> 0x33 -> IRQ 3 Mode:0 Active:0) IOAPIC[0]: Set routing entry (0-4 -> 0x34 -> IRQ 4 Mode:0 Active:0) IOAPIC[0]: Set routing entry (0-5 -> 0x35 -> IRQ 5 Mode:0 Active:0) IOAPIC[0]: Set routing entry (0-6 -> 0x36 -> IRQ 6 Mode:0 Active:0) IOAPIC[0]: Set routing entry (0-7 -> 0x37 -> IRQ 7 Mode:0 Active:0) IOAPIC[0]: Set routing entry (0-8 -> 0x38 -> IRQ 8 Mode:0 Active:0) IOAPIC[0]: Set routing entry (0-9 -> 0x39 -> IRQ 9 Mode:1 Active:0) IOAPIC[0]: Set routing entry (0-10 -> 0x3a -> IRQ 10 Mode:0 Active:0) IOAPIC[0]: Set routing entry (0-11 -> 0x3b -> IRQ 11 Mode:0 Active:0) IOAPIC[0]: Set routing entry (0-12 -> 0x3c -> IRQ 12 Mode:0 Active:0) IOAPIC[0]: Set routing entry (0-13 -> 0x3d -> IRQ 13 Mode:0 Active:0) IOAPIC[0]: Set routing entry (0-14 -> 0x3e -> IRQ 14 Mode:0 Active:0) IOAPIC[0]: Set routing entry (0-15 -> 0x3f -> IRQ 15 Mode:0 Active:0) IO-APIC (apicid-pin) 0-16, 0-17, 0-18, 0-19, 0-20, 0-21, 0-22, 0-23, 1-0, 1-1, 1-2, 1-3, 1-4, 1-5, 1-6, 2-0, 2-1, 2-2, 2-3, 2-4, 2-5, 2-6, 3-0, 3-1, 3-2, 3-3, 3-4, 3-5, 3-6, 3-7, 3-8, 3-9, 3-10, 3-11, 3-12, 3-13, 3-14, 3-15, 3-16, 3-17, 3-18, 3-19, 3-20, 3-21, 3-22, 3-23 not connected. there is some difference between using acpi or mptable... when mpatble is used, mp_irqs is all filled after mptable is parsed. when acpi is used, one [0-15) is filled..., later it will fill entries when acpi add entries...mp_register_gsi/mp_config_acpi_gsi YH --
thanks for explanation Yinghai, i was reffering to MP and didn't take a look on ACPI facility (that is why I've asked you). Thanks! --
Honestly, this one should probably use sprintf() or suchlike to avoid the mess of printk() calls building a line of output from pieces. It's quite easy to calculate here what the maximum size of the buffer required could be and automatic arrays can have variable size, so no need for the hassle of heap management. Calls to printk() without a trailing newline should be avoided where possible as it messes up logging priority if a message pops up from an interrupt inbetween. Maciej --
hm, is it worth the trouble? This is during very early init. Ingo --
But is it really a trouble? With an auto array the additional code is minuscule -- mostly printk() calls replaced with sprintf() plus a variable or two to maintain a pointer to the buffer which will go into registers anyway. Plus a pr_info("%s", buffer) or suchlike at the end. I know laziness is a virtue, but you have to draw a line somewhere. ;) The benefit, apart from what I wrote above, is less chance of propagating bad style to newcomers with each piece of such old code removed. Maciej --
[Ingo Molnar - Sat, Sep 06, 2008 at 08:49:14PM +0200]
|
| * Maciej W. Rozycki <macro@linux-mips.org> wrote:
|
| > On Sat, 6 Sep 2008, Cyrill Gorcunov wrote:
| >
| > > Ingo, how about the following approach? We don't introduce new
| > > functions but rather srink the code by new printout form.
| >
| > Honestly, this one should probably use sprintf() or suchlike to avoid the
| > mess of printk() calls building a line of output from pieces. It's quite
| > easy to calculate here what the maximum size of the buffer required could
| > be and automatic arrays can have variable size, so no need for the hassle
| > of heap management. Calls to printk() without a trailing newline should
| > be avoided where possible as it messes up logging priority if a message
| > pops up from an interrupt inbetween.
|
| hm, is it worth the trouble? This is during very early init.
|
| Ingo
|
Ingo, Maciej,
here is an another attempt to satisfy the requirements :)
Please review and comment if it looks better or worse now.
- Cyrill -
---
From: Cyrill Gorcunov <gorcunov@gmail.com>
Subject: [PATCH] x86: io-apic - cleanup of setup_IO_APIC_irqs v3
Use local buffer to accumulate all not connected pin info
on each io-apic and printout it if needed.
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
Index: linux-2.6.git/arch/x86/kernel/io_apic.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/io_apic.c 2008-09-07 10:10:15.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/io_apic.c 2008-09-07 13:50:46.000000000 +0400
@@ -1514,46 +1514,69 @@ static void setup_IO_APIC_irq(int apic,
ioapic_write_entry(apic, pin, entry);
}
+/* check and setup io-apic irq */
+static int __init setup_ioapic_irq(int apic, int pin)
+{
+ int idx, irq;
+
+ idx = find_irq_entry(apic, pin, mp_INT);
+ if (idx == -1)
+ return -1;
+
+ irq = pin_2_irq(idx, apic, pin);
+
+#ifdef CONFIG_X86_32
+ if (multi_timer_check(apic, ...sigh, i think the extra buffering is clearly worse. These are early init printks and we already do many other cases of multi-line printouts and even KERN_CONT printouts. We have a perfectly fine built-in buffer already: the printk buffer. Ingo --
[Ingo Molnar - Sun, Sep 07, 2008 at 05:47:21PM +0200] | | * Cyrill Gorcunov <gorcunov@gmail.com> wrote: | | > here is an another attempt to satisfy the requirements :) | > Please review and comment if it looks better or worse now. | | sigh, i think the extra buffering is clearly worse. These are early init | printks and we already do many other cases of multi-line printouts and | even KERN_CONT printouts. We have a perfectly fine built-in buffer | already: the printk buffer. | | Ingo | ok Ingo, i think we could drop this idea then. - Cyrill - --
[Maciej W. Rozycki - Sat, Sep 06, 2008 at 07:45:08PM +0100] | On Sat, 6 Sep 2008, Cyrill Gorcunov wrote: | | > Ingo, how about the following approach? We don't introduce new | > functions but rather srink the code by new printout form. | | Honestly, this one should probably use sprintf() or suchlike to avoid the | mess of printk() calls building a line of output from pieces. It's quite | easy to calculate here what the maximum size of the buffer required could | be and automatic arrays can have variable size, so no need for the hassle | of heap management. Calls to printk() without a trailing newline should | be avoided where possible as it messes up logging priority if a message | pops up from an interrupt inbetween. | | Maciej | The easiest way would be just print this info on separate lines like IO-APIC (apicid-pin) 1-1 not connected and just drop all this troubles :) I'm not sure how much memory we need for every io-apic pins - iirc there only 32 redirection entry so it could be about 32 bytes from stack would be enough. Will take a look. Thanks Maciej! Ingo? Btw, i think the problem is rather with (as Ingo mentoined) too nesting cycles - so maybe we could use an iterator for this? Maybe rcu based? As I see these global variables are not supposed to be changed during iterations and if that happens in future we could achieve rcu complains about it and this prevent us for possible bug. Opinions? (just a thoughts) - Cyrill - --
no. some system could have 3 or 4 ioapic controller, and every one have 24...(like three mcp55/io55) 4*24 or old system have 1 8111 and 7 8132. will have 32 + 7*2*7 YH --
[Yinghai Lu - Sat, Sep 06, 2008 at 12:16:28PM -0700] | On Sat, Sep 6, 2008 at 12:04 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote: | > [Maciej W. Rozycki - Sat, Sep 06, 2008 at 07:45:08PM +0100] | > | On Sat, 6 Sep 2008, Cyrill Gorcunov wrote: | > | | > | > Ingo, how about the following approach? We don't introduce new | > | > functions but rather srink the code by new printout form. | > | | > | Honestly, this one should probably use sprintf() or suchlike to avoid the | > | mess of printk() calls building a line of output from pieces. It's quite | > | easy to calculate here what the maximum size of the buffer required could | > | be and automatic arrays can have variable size, so no need for the hassle | > | of heap management. Calls to printk() without a trailing newline should | > | be avoided where possible as it messes up logging priority if a message | > | pops up from an interrupt inbetween. | > | | > | Maciej | > | | > | > The easiest way would be just print this info on separate | > lines like | > | > IO-APIC (apicid-pin) 1-1 not connected | > | > and just drop all this troubles :) | > | > I'm not sure how much memory we need for every io-apic | > pins - iirc there only 32 redirection entry so it could | > be about 32 bytes from stack would be enough. Will take | > a look. Thanks Maciej! Ingo? | | no. some system could have 3 or 4 ioapic controller, and every one | have 24...(like three mcp55/io55) | 4*24 | | or old system have 1 8111 and 7 8132. will have 32 + 7*2*7 | | YH | yep, I just calculated - we've to allocate too much for early init. - Cyrill - --
[Yinghai Lu - Sat, Sep 06, 2008 at 12:16:28PM -0700] | On Sat, Sep 6, 2008 at 12:04 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote: | > [Maciej W. Rozycki - Sat, Sep 06, 2008 at 07:45:08PM +0100] | > | On Sat, 6 Sep 2008, Cyrill Gorcunov wrote: | > | | > | > Ingo, how about the following approach? We don't introduce new | > | > functions but rather srink the code by new printout form. | > | | > | Honestly, this one should probably use sprintf() or suchlike to avoid the | > | mess of printk() calls building a line of output from pieces. It's quite | > | easy to calculate here what the maximum size of the buffer required could | > | be and automatic arrays can have variable size, so no need for the hassle | > | of heap management. Calls to printk() without a trailing newline should | > | be avoided where possible as it messes up logging priority if a message | > | pops up from an interrupt inbetween. | > | | > | Maciej | > | | > | > The easiest way would be just print this info on separate | > lines like | > | > IO-APIC (apicid-pin) 1-1 not connected | > | > and just drop all this troubles :) | > | > I'm not sure how much memory we need for every io-apic | > pins - iirc there only 32 redirection entry so it could | > be about 32 bytes from stack would be enough. Will take | > a look. Thanks Maciej! Ingo? | | no. some system could have 3 or 4 ioapic controller, and every one | have 24...(like three mcp55/io55) | 4*24 | | or old system have 1 8111 and 7 8132. will have 32 + 7*2*7 | | YH | Didn't really understand all numbers :) We have 24 io-apic route entries 128 io-apic maximum in quantity so the worst (hardly possible if ever) is when on last io-apic all pins are wrong. So for this case we would need (3 + 1) * ((10 + 1) + (13 + 1)) = 100 where 3 - strlen("127") - max io-apic number 10 - Sum(strlen(0)+...+strlen(9)) - pin numbers 13 - same sum for two-digit numbers +1 number - either "-" sign or " " 100 bytes to print not ...
[Cyrill Gorcunov - Sat, Sep 06, 2008 at 11:38:20PM +0400] | [Yinghai Lu - Sat, Sep 06, 2008 at 12:16:28PM -0700] | | On Sat, Sep 6, 2008 at 12:04 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote: | | > [Maciej W. Rozycki - Sat, Sep 06, 2008 at 07:45:08PM +0100] | | > | On Sat, 6 Sep 2008, Cyrill Gorcunov wrote: | | > | | | > | > Ingo, how about the following approach? We don't introduce new | | > | > functions but rather srink the code by new printout form. | | > | | | > | Honestly, this one should probably use sprintf() or suchlike to avoid the | | > | mess of printk() calls building a line of output from pieces. It's quite | | > | easy to calculate here what the maximum size of the buffer required could | | > | be and automatic arrays can have variable size, so no need for the hassle | | > | of heap management. Calls to printk() without a trailing newline should | | > | be avoided where possible as it messes up logging priority if a message | | > | pops up from an interrupt inbetween. | | > | | | > | Maciej | | > | | | > | | > The easiest way would be just print this info on separate | | > lines like | | > | | > IO-APIC (apicid-pin) 1-1 not connected | | > | | > and just drop all this troubles :) | | > | | > I'm not sure how much memory we need for every io-apic | | > pins - iirc there only 32 redirection entry so it could | | > be about 32 bytes from stack would be enough. Will take | | > a look. Thanks Maciej! Ingo? | | | | no. some system could have 3 or 4 ioapic controller, and every one | | have 24...(like three mcp55/io55) | | 4*24 | | | | or old system have 1 8111 and 7 8132. will have 32 + 7*2*7 | | | | YH | | | | Didn't really understand all numbers :) | We have | 24 io-apic route entries | 128 io-apic maximum in quantity | | so the worst (hardly possible if ever) is when on last io-apic | all pins are wrong. So for this case we would need | | (3 + 1) * ((10 + 1) + (13 + 1)) = 100 | | where | 3 - strlen("127") - max io-apic ...
No doubt about it. Then if the number of entries was to be huge, then lines output to the log buffer could be limited to 80 characters. With the use of sprintf() it would be rather trivial. And you would avoid any concerns about stack consumption. Please note that with MPS 1.1 systems the number of "unconnected" I/O APIC inputs can be considerable. Maciej --
[Maciej W. Rozycki - Sat, Sep 06, 2008 at 09:08:51PM +0100] | On Sat, 6 Sep 2008, Cyrill Gorcunov wrote: | | > So I think better would be just use Ingo's suggestion | > about to separate apic/pin iterator and function body. | | No doubt about it. Then if the number of entries was to be huge, then | lines output to the log buffer could be limited to 80 characters. With | the use of sprintf() it would be rather trivial. And you would avoid any | concerns about stack consumption. | | Please note that with MPS 1.1 systems the number of "unconnected" I/O | APIC inputs can be considerable. | | Maciej | Thanks Maciej, will continue tomorrow. - Cyrill - --
