console: allocate ring buffer earlier

PATCH Superseded Jan Beulich 3 participants 7 messages Dec 5, 2014 – Dec 8, 2014

Share & tools

Jan Beulich

author

2014-12-05 16:55 UTC #0 lore raw

PATCH 1/1 console: allocate ring buffer earlier

... when "conring_size=" was specified on the command line. We can't really do this as early as we would want to when the option was not specified, as the default depends on knowing the system CPU count. Yet the parsing of the ACPI tables is one of the things that generates a lot of output especially on large systems.

I didn't change ARM, as I wasn't sure how far ahead this call could be pulled.

Signed-off-by Jan Beulich <[email protected]>

xen/arch/x86/setup.c +1 -0

@@ -1187,6 +1187,7 @@ void __init noreturn __start_xen(unsigne
     }
 
     vm_init();
+    console_init_mem();
     vesa_init();
 
     softirq_init();

xen/drivers/char/console.c +16 -6

@@ -744,15 +744,14 @@ void __init console_init_preirq(void)
     }
 }
 
-void __init console_init_postirq(void)
+void __init console_init_mem(void)
 {
     char *ring;
     unsigned int i, order, memflags;
-
-    serial_init_postirq();
+    unsigned long flags;
 
     if ( !opt_conring_size )
-        opt_conring_size = num_present_cpus() << (9 + xenlog_lower_thresh);
+        return;
 
     order = get_order_from_bytes(max(opt_conring_size, conring_size));
     memflags = MEMF_bits(crashinfo_maxaddr_bits);

@@ -763,17 +762,28 @@ void __init console_init_postirq(void)
     }
     opt_conring_size = PAGE_SIZE << order;
 
-    spin_lock_irq(&console_lock);
+    spin_lock_irqsave(&console_lock, flags);
     for ( i = conringc ; i != conringp; i++ )
         ring[i & (opt_conring_size - 1)] = conring[i & (conring_size - 1)];
     conring = ring;
     smp_wmb(); /* Allow users of console_force_unlock() to see larger buffer. */
     conring_size = opt_conring_size;
-    spin_unlock_irq(&console_lock);
+    spin_unlock_irqrestore(&console_lock, flags);
 
     printk("Allocated console ring of %u KiB.\n", opt_conring_size >> 10);
 }
 
+void __init console_init_postirq(void)
+{
+    serial_init_postirq();
+
+    if ( !opt_conring_size )
+        opt_conring_size = num_present_cpus() << (9 + xenlog_lower_thresh);
+
+    if ( conring == _conring )
+        console_init_mem();
+}
+
 void __init console_endboot(void)
 {
     int i, j;

xen/include/xen/console.h +1 -0

@@ -14,6 +14,7 @@ struct xen_sysctl_readconsole;
 long read_console_ring(struct xen_sysctl_readconsole *op);
 
 void console_init_preirq(void);
+void console_init_mem(void);
 void console_init_postirq(void);
 void console_endboot(void);
 int console_has(const char *device);

console: allocate ring buffer earlier

I didn't change ARM, as I wasn't sure how far ahead this call could be pulled.

Signed-off-by Jan Beulich <[email protected]>

xen/arch/x86/setup.c +1 -0

@@ -1187,6 +1187,7 @@ void __init noreturn __start_xen(unsigne
     }
 
     vm_init();
+    console_init_mem();
     vesa_init();
 
     softirq_init();

xen/drivers/char/console.c +16 -6

@@ -744,15 +744,14 @@ void __init console_init_preirq(void)
     }
 }
 
-void __init console_init_postirq(void)
+void __init console_init_mem(void)
 {
     char *ring;
     unsigned int i, order, memflags;
-
-    serial_init_postirq();
+    unsigned long flags;
 
     if ( !opt_conring_size )
-        opt_conring_size = num_present_cpus() << (9 + xenlog_lower_thresh);
+        return;
 
     order = get_order_from_bytes(max(opt_conring_size, conring_size));
     memflags = MEMF_bits(crashinfo_maxaddr_bits);

@@ -763,17 +762,28 @@ void __init console_init_postirq(void)
     }
     opt_conring_size = PAGE_SIZE << order;
 
-    spin_lock_irq(&console_lock);
+    spin_lock_irqsave(&console_lock, flags);
     for ( i = conringc ; i != conringp; i++ )
         ring[i & (opt_conring_size - 1)] = conring[i & (conring_size - 1)];
     conring = ring;
     smp_wmb(); /* Allow users of console_force_unlock() to see larger buffer. */
     conring_size = opt_conring_size;
-    spin_unlock_irq(&console_lock);
+    spin_unlock_irqrestore(&console_lock, flags);
 
     printk("Allocated console ring of %u KiB.\n", opt_conring_size >> 10);
 }
 
+void __init console_init_postirq(void)
+{
+    serial_init_postirq();
+
+    if ( !opt_conring_size )
+        opt_conring_size = num_present_cpus() << (9 + xenlog_lower_thresh);
+
+    if ( conring == _conring )
+        console_init_mem();
+}
+
 void __init console_endboot(void)
 {
     int i, j;

xen/include/xen/console.h +1 -0

@@ -14,6 +14,7 @@ struct xen_sysctl_readconsole;
 long read_console_ring(struct xen_sysctl_readconsole *op);
 
 void console_init_preirq(void);
+void console_init_mem(void);
 void console_init_postirq(void);
 void console_endboot(void);
 int console_has(const char *device);

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Daniel Kiper

2014-12-05 17:23 UTC #1 lore raw

Quoted 10 lines · Jan Beulich

... when "conring_size=" was specified on the command line. We can't really do this as early as we would want to when the option was not specified, as the default depends on knowing the system CPU count. Yet the parsing of the ACPI tables is one of the things that generates a lot of output especially on large systems.
I didn't change ARM, as I wasn't sure how far ahead this call could be pulled.
Signed-off-by Jan Beulich <[email protected]>

Make sense for me but I think that we should have the same thing for ARM too.

Quoted 32 lines

xen/arch/x86/setup.c +1 -0

@@ -1187,6 +1187,7 @@ void __init noreturn __start_xen(unsigne
     }
 
     vm_init();
+    console_init_mem();
     vesa_init();
 
     softirq_init();

xen/drivers/char/console.c +3 -4

@@ -744,15 +744,14 @@ void __init console_init_preirq(void)
     }
 }
 
-void __init console_init_postirq(void)
+void __init console_init_mem(void)
 {
     char *ring;
     unsigned int i, order, memflags;
-
-    serial_init_postirq();
+    unsigned long flags;
 
     if ( !opt_conring_size )
-        opt_conring_size = num_present_cpus() << (9 + xenlog_lower_thresh);
+        return;
 
     order = get_order_from_bytes(max(opt_conring_size, conring_size));
     memflags = MEMF_bits(crashinfo_maxaddr_bits);

xen/drivers/char/console.c void __init console_init_postirq(void) · line 765

Quoted 5 lines

xen/drivers/char/console.c +1 -1

@@ -763,17 +762,28 @@ void __init console_init_postirq(void)
     }
     opt_conring_size = PAGE_SIZE << order;
 
-    spin_lock_irq(&console_lock);
+    spin_lock_irqsave(&console_lock, flags);

I am not sure why are you change spin_lock_irq() to spin_lock_irqsave() here. Could you explain this in commit message?

xen/drivers/char/console.c void __init console_init_postirq(void) · line 771

Quoted 7 lines

xen/drivers/char/console.c +1 -1

     for ( i = conringc ; i != conringp; i++ )
         ring[i & (opt_conring_size - 1)] = conring[i & (conring_size - 1)];
     conring = ring;
     smp_wmb(); /* Allow users of console_force_unlock() to see larger buffer. */
     conring_size = opt_conring_size;
-    spin_unlock_irq(&console_lock);
+    spin_unlock_irqrestore(&console_lock, flags);

Ditto.

Daniel

Daniel Kiper

2014-12-07 20:15 UTC #2 lore raw

Quoted 7 lines · Daniel Kiper

Quoted 6 lines · Jan Beulich
... when "conring_size=" was specified on the command line. We can't really do this as early as we would want to when the option was not specified, as the default depends on knowing the system CPU count. Yet the parsing of the ACPI tables is one of the things that generates a lot of output especially on large systems.

Quoted 6 lines

Quoted 4 lines
I didn't change ARM, as I wasn't sure how far ahead this call could be pulled.
Signed-off-by Jan Beulich <[email protected]>
Make sense for me but I think that we should have the same thing for ARM too.

Hmmm... Even it is better I still think that your proposal is too intrusive at this stage of 4.5 development. However, I think that Konrad has the last word in that case.

Daniel

Jan Beulich

author

2014-12-08 08:54 UTC #3 lore raw

In reply to Daniel Kiper

Quoted 6 lines

Quoted 1 line
Quoted 1 line · On 05.12.14 at 18:22, <[email protected]> wrote:
Quoted 4 lines · Jan Beulich
xen/drivers/char/console.c +0 -0
@@ -763,17 +762,28 @@ void __init console_init_postirq(void)
     }
     opt_conring_size = PAGE_SIZE << order;
 

Quoted 6 lines

Review void __init console_init_postirq(void) · line 765
Quoted 2 lines
(quoted diff) +1 -1
-    spin_lock_irq(&console_lock);
+    spin_lock_irqsave(&console_lock, flags);
I am not sure why are you change spin_lock_irq() to spin_lock_irqsave() here.
Could you explain this in commit message?

I think this is quite obvious in the context here: Previously this was guaranteed to be called after interrupts were already enabled (hence the _postirq name). That's not the case anymore, and thus we can't blindly enable interrupts when releasing the lock.

Jan

Julien Grall

2014-12-06 00:05 UTC #4 lore raw

In reply to Jan Beulich

Hi Jan,

Quoted 2 lines · Jan Beulich

I didn't change ARM, as I wasn't sure how far ahead this call could be pulled.

AFAIU, the new function only requires that the page table are setup (because of the alloc_xenheap_pages).

So console_init_mem could be called right after console_init_preirq.

Regards,

Signature · 1 line

Julien Grall

Jan Beulich

author

2014-12-08 08:52 UTC #5 lore raw

Quoted 9 lines

Quoted 1 line
Quoted 1 line · On 06.12.14 at 01:05, <[email protected]> wrote:
Quoted 2 lines · Jan Beulich
I didn't change ARM, as I wasn't sure how far ahead this call could be pulled.
AFAIU, the new function only requires that the page table are setup (because of the alloc_xenheap_pages).
So console_init_mem could be called right after console_init_preirq.

No, it requires that alloc_xenheap_pages() works, i.e. it surely can't be placed before the end_boot_allocator() invocation.

Jan

Julien Grall

2014-12-08 11:50 UTC #6 lore raw

Quoted 5 lines · Jan Beulich

Quoted 5 lines
Quoted 1 line
Quoted 1 line · On 06.12.14 at 01:05, <[email protected]> wrote:
Quoted 2 lines · Jan Beulich
I didn't change ARM, as I wasn't sure how far ahead this call could be pulled.

Quoted 7 lines

Quoted 4 lines
AFAIU, the new function only requires that the page table are setup (because of the alloc_xenheap_pages).
So console_init_mem could be called right after console_init_preirq.
No, it requires that alloc_xenheap_pages() works, i.e. it surely can't be placed before the end_boot_allocator() invocation.

Which is the case with the place I was suggesting...

On ARM, end_boot_allocator is called in setup_mm which is called before console_init_preirq.

Regards,

Signature · 1 line

Julien Grall