On larger systems, because of the numerous ACPI, Bootmem and EFI
messages, the static log buffer overflows before the larger one
specified by the log_buf_len param is allocated. Minimize the
potential for overflow by allocating the new log buffer as soon
as possible.
We do this by changing the log_buf_len from an early_param to a
_setup param. But _setup params are processed before the
alloc_bootmem is available, so this function will now just save
the requested log buf len. The real work routine (setup_log_buf)
is called after bootmem is available.
Signed-off-by: Mike Travis <[email protected]>
Reviewed-by: Jack Steiner <[email protected]>
Reviewed-by: Robin Holt <[email protected]>
---
arch/x86/kernel/setup.c | 5 +++
include/linux/printk.h | 4 ++
init/main.c | 1
kernel/printk.c | 75 ++++++++++++++++++++++++++++--------------------
4 files changed, 54 insertions(+), 31 deletions(-)
--- linux.orig/arch/x86/kernel/setup.c
+++ linux/arch/x86/kernel/setup.c
@@ -1007,6 +1007,11 @@ void __init setup_arch(char **cmdline_p)
memblock_find_dma_reserve();
dma32_reserve_bootmem();
+ /*
+ * Allocate bigger log buffer as early as possible
+ */
+ setup_log_buf();
+
#ifdef CONFIG_KVM_CLOCK
kvmclock_init();
#endif
--- linux.orig/include/linux/printk.h
+++ linux/include/linux/printk.h
@@ -1,6 +1,8 @@
#ifndef __KERNEL_PRINTK__
#define __KERNEL_PRINTK__
+#include <linux/init.h>
+
extern const char linux_banner[];
extern const char linux_proc_banner[];
@@ -89,6 +91,8 @@ int no_printk(const char *fmt, ...)
extern asmlinkage __attribute__ ((format (printf, 1, 2)))
void early_printk(const char *fmt, ...);
+void __init setup_log_buf(void);
+
extern int printk_needs_cpu(int cpu);
extern void printk_tick(void);
--- linux.orig/init/main.c
+++ linux/init/main.c
@@ -592,6 +592,7 @@ asmlinkage void __init start_kernel(void
* These use large bootmem allocations and must precede
* kmem_cache_init()
*/
+ setup_log_buf();
pidhash_init();
vfs_caches_init_early();
sort_main_extable();
--- linux.orig/kernel/printk.c
+++ linux/kernel/printk.c
@@ -162,46 +162,59 @@ void log_buf_kexec_setup(void)
}
#endif
+/* requested log_buf_len from kernel cmdline */
+static unsigned long __initdata new_log_buf_len;
+
+/* save requested log_buf_len since it's too early to process it */
static int __init log_buf_len_setup(char *str)
{
unsigned size = memparse(str, &str);
- unsigned long flags;
if (size)
size = roundup_pow_of_two(size);
- if (size > log_buf_len) {
- unsigned start, dest_idx, offset;
- char *new_log_buf;
-
- new_log_buf = alloc_bootmem(size);
- if (!new_log_buf) {
- printk(KERN_WARNING "log_buf_len: allocation failed\n");
- goto out;
- }
-
- spin_lock_irqsave(&logbuf_lock, flags);
- log_buf_len = size;
- log_buf = new_log_buf;
-
- offset = start = min(con_start, log_start);
- dest_idx = 0;
- while (start != log_end) {
- log_buf[dest_idx] = __log_buf[start & (__LOG_BUF_LEN - 1)];
- start++;
- dest_idx++;
- }
- log_start -= offset;
- con_start -= offset;
- log_end -= offset;
- spin_unlock_irqrestore(&logbuf_lock, flags);
+ if (size > log_buf_len)
+ new_log_buf_len = size;
- printk(KERN_NOTICE "log_buf_len: %d\n", log_buf_len);
- }
-out:
- return 1;
+ return 0;
}
+early_param("log_buf_len", log_buf_len_setup);
-__setup("log_buf_len=", log_buf_len_setup);
+void __init setup_log_buf(void)
+{
+ unsigned long flags;
+ unsigned start, dest_idx, offset;
+ char *new_log_buf;
+ int free;
+
+ if (!new_log_buf_len)
+ return;
+
+ new_log_buf = alloc_bootmem(new_log_buf_len);
+
+ spin_lock_irqsave(&logbuf_lock, flags);
+ log_buf_len = new_log_buf_len;
+ log_buf = new_log_buf;
+ new_log_buf_len = 0;
+ free = __LOG_BUF_LEN - log_end;
+
+ offset = start = min(con_start, log_start);
+ dest_idx = 0;
+ while (start != log_end) {
+ unsigned log_idx_mask = start & (__LOG_BUF_LEN - 1);
+
+ log_buf[dest_idx] = __log_buf[log_idx_mask];
+ start++;
+ dest_idx++;
+ }
+ log_start -= offset;
+ con_start -= offset;
+ log_end -= offset;
+ spin_unlock_irqrestore(&logbuf_lock, flags);
+
+ pr_info("log_buf_len: %d\n", log_buf_len);
+ pr_info("early log buf free: %d(%d%%)\n",
+ free, (free * 100) / __LOG_BUF_LEN);
+}
#ifdef CONFIG_BOOT_PRINTK_DELAY
--
* Mike Travis <[email protected]> wrote:
> On larger systems, because of the numerous ACPI, Bootmem and EFI
> messages, the static log buffer overflows before the larger one
> specified by the log_buf_len param is allocated. Minimize the
> potential for overflow by allocating the new log buffer as soon
> as possible.
>
> We do this by changing the log_buf_len from an early_param to a
> _setup param. But _setup params are processed before the
> alloc_bootmem is available, so this function will now just save
> the requested log buf len. The real work routine (setup_log_buf)
> is called after bootmem is available.
>
> Signed-off-by: Mike Travis <[email protected]>
> Reviewed-by: Jack Steiner <[email protected]>
> Reviewed-by: Robin Holt <[email protected]>
> ---
> arch/x86/kernel/setup.c | 5 +++
> include/linux/printk.h | 4 ++
> init/main.c | 1
> kernel/printk.c | 75 ++++++++++++++++++++++++++++--------------------
> 4 files changed, 54 insertions(+), 31 deletions(-)
Well, the modern allocation method is memblock - available on all major
architectures.
You could avoid all this ugly workaround of bootmem limitations by moving the
allocation to memblock_alloc() and desupporting the log_buf_len= boot parameter
on non-memblock architectures.
kernel log buffer size can be configured via the .config so they will not be left
without larger buffers.
Doing this should also have the advantage of getting all the early x86 messages into
the larger buffer already, reducing the pressure to apply some of the other patches
in your series.
Thanks,
Ingo
* Ingo Molnar <[email protected]> wrote:
> You could avoid all this ugly workaround of bootmem limitations by moving the
> allocation to memblock_alloc() and desupporting the log_buf_len= boot parameter on
> non-memblock architectures.
memblock_alloc() could return -ENOSYS on architectures that do not implement it -
thus enabling such optional features without ugly #ifdef CONFIG_HAVE_MEMBLOCK
conditionals.
Thanks,
Ingo
On 02/27/2011 04:15 AM, Ingo Molnar wrote:
>
> * Ingo Molnar <[email protected]> wrote:
>
>> You could avoid all this ugly workaround of bootmem limitations by moving the
>> allocation to memblock_alloc() and desupporting the log_buf_len= boot parameter on
>> non-memblock architectures.
>
> memblock_alloc() could return -ENOSYS on architectures that do not implement it -
> thus enabling such optional features without ugly #ifdef CONFIG_HAVE_MEMBLOCK
> conditionals.
Mike,
please check updated patch...
with the memblock change, you don't need to change acpi SRAT handling etc any more.
new buffer will be near high mem under 4g.
Thanks
Yinghai
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index a47fe00..69b8995 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -974,6 +974,11 @@ void __init setup_arch(char **cmdline_p)
memblock.current_limit = get_max_mapped();
/*
+ * Allocate bigger log buffer as early as possible
+ */
+ setup_log_buf();
+
+ /*
* NOTE: On x86-32, only from this point on, fixmaps are ready for use.
*/
diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 62a10c2..c3ade22 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -2,6 +2,8 @@
#define _LINUX_MEMBLOCK_H
#ifdef __KERNEL__
+#define MEMBLOCK_ERROR 0
+
#ifdef CONFIG_HAVE_MEMBLOCK
/*
* Logical memory blocks.
@@ -20,7 +22,6 @@
#include <asm/memblock.h>
#define INIT_MEMBLOCK_REGIONS 128
-#define MEMBLOCK_ERROR 0
struct memblock_region {
phys_addr_t base;
@@ -160,6 +161,12 @@ static inline unsigned long memblock_region_reserved_end_pfn(const struct memblo
#define __initdata_memblock
#endif
+#else
+static inline phys_addr_t memblock_alloc(phys_addr_t size, phys_addr_t align)
+{
+ return MEMBLOCK_ERROR;
+}
+
#endif /* CONFIG_HAVE_MEMBLOCK */
#endif /* __KERNEL__ */
diff --git a/include/linux/printk.h b/include/linux/printk.h
index ee048e7..fd266a8 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -1,6 +1,8 @@
#ifndef __KERNEL_PRINTK__
#define __KERNEL_PRINTK__
+#include <linux/init.h>
+
extern const char linux_banner[];
extern const char linux_proc_banner[];
@@ -89,6 +91,8 @@ int no_printk(const char *fmt, ...)
extern asmlinkage __attribute__ ((format (printf, 1, 2)))
void early_printk(const char *fmt, ...);
+void __init setup_log_buf(void);
+
extern int printk_needs_cpu(int cpu);
extern void printk_tick(void);
diff --git a/init/main.c b/init/main.c
index 33c37c3..2085bb3 100644
--- a/init/main.c
+++ b/init/main.c
@@ -592,6 +592,7 @@ asmlinkage void __init start_kernel(void)
* These use large bootmem allocations and must precede
* kmem_cache_init()
*/
+ setup_log_buf();
pidhash_init();
vfs_caches_init_early();
sort_main_extable();
diff --git a/kernel/printk.c b/kernel/printk.c
index 3623152..14fa4d0 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -31,6 +31,7 @@
#include <linux/smp.h>
#include <linux/security.h>
#include <linux/bootmem.h>
+#include <linux/memblock.h>
#include <linux/syscalls.h>
#include <linux/kexec.h>
#include <linux/kdb.h>
@@ -162,46 +163,64 @@ void log_buf_kexec_setup(void)
}
#endif
+/* requested log_buf_len from kernel cmdline */
+static unsigned long __initdata new_log_buf_len;
+
+/* save requested log_buf_len since it's too early to process it */
static int __init log_buf_len_setup(char *str)
{
unsigned size = memparse(str, &str);
- unsigned long flags;
if (size)
size = roundup_pow_of_two(size);
- if (size > log_buf_len) {
- unsigned start, dest_idx, offset;
- char *new_log_buf;
+ if (size > log_buf_len)
+ new_log_buf_len = size;
- new_log_buf = alloc_bootmem(size);
- if (!new_log_buf) {
- printk(KERN_WARNING "log_buf_len: allocation failed\n");
- goto out;
- }
+ return 0;
+}
+early_param("log_buf_len", log_buf_len_setup);
- spin_lock_irqsave(&logbuf_lock, flags);
- log_buf_len = size;
- log_buf = new_log_buf;
-
- offset = start = min(con_start, log_start);
- dest_idx = 0;
- while (start != log_end) {
- log_buf[dest_idx] = __log_buf[start & (__LOG_BUF_LEN - 1)];
- start++;
- dest_idx++;
- }
- log_start -= offset;
- con_start -= offset;
- log_end -= offset;
- spin_unlock_irqrestore(&logbuf_lock, flags);
+void __init setup_log_buf(void)
+{
+ unsigned long flags;
+ unsigned start, dest_idx, offset;
+ char *new_log_buf;
+ phys_addr_t new_addr;
+ int free;
+
+ if (!new_log_buf_len)
+ return;
- printk(KERN_NOTICE "log_buf_len: %d\n", log_buf_len);
+ new_addr = memblock_alloc(new_log_buf_len, PAGE_SIZE);
+ if (new_addr != MEMBLOCK_ERROR)
+ new_log_buf = __va(new_addr);
+ else
+ new_log_buf = alloc_bootmem(new_log_buf_len);
+
+ spin_lock_irqsave(&logbuf_lock, flags);
+ log_buf_len = new_log_buf_len;
+ log_buf = new_log_buf;
+ new_log_buf_len = 0;
+ free = __LOG_BUF_LEN - log_end;
+
+ offset = start = min(con_start, log_start);
+ dest_idx = 0;
+ while (start != log_end) {
+ unsigned log_idx_mask = start & (__LOG_BUF_LEN - 1);
+
+ log_buf[dest_idx] = __log_buf[log_idx_mask];
+ start++;
+ dest_idx++;
}
-out:
- return 1;
-}
+ log_start -= offset;
+ con_start -= offset;
+ log_end -= offset;
+ spin_unlock_irqrestore(&logbuf_lock, flags);
-__setup("log_buf_len=", log_buf_len_setup);
+ pr_info("log_buf_len: %d\n", log_buf_len);
+ pr_info("early log buf free: %d(%d%%)\n",
+ free, (free * 100) / __LOG_BUF_LEN);
+}
#ifdef CONFIG_BOOT_PRINTK_DELAY
* Yinghai Lu <[email protected]> wrote:
> + pr_info("log_buf_len: %d\n", log_buf_len);
> + pr_info("early log buf free: %d(%d%%)\n",
> + free, (free * 100) / __LOG_BUF_LEN);
Such debug printks should be removed from the final version of the patch ...
Thanks,
Ingo
* Yinghai Lu <[email protected]> wrote:
> + new_addr = memblock_alloc(new_log_buf_len, PAGE_SIZE);
> + if (new_addr != MEMBLOCK_ERROR)
> + new_log_buf = __va(new_addr);
> + else
> + new_log_buf = alloc_bootmem(new_log_buf_len);
alloc_bootmem() can fail, especially if someone uses a too large boot parameter
value - and your code does not check for failure.
Thanks,
Ingo
Ingo Molnar wrote:
> * Mike Travis <[email protected]> wrote:
>
>> On larger systems, because of the numerous ACPI, Bootmem and EFI
>> messages, the static log buffer overflows before the larger one
>> specified by the log_buf_len param is allocated. Minimize the
>> potential for overflow by allocating the new log buffer as soon
>> as possible.
>>
>> We do this by changing the log_buf_len from an early_param to a
>> _setup param. But _setup params are processed before the
>> alloc_bootmem is available, so this function will now just save
>> the requested log buf len. The real work routine (setup_log_buf)
>> is called after bootmem is available.
>>
>> Signed-off-by: Mike Travis <[email protected]>
>> Reviewed-by: Jack Steiner <[email protected]>
>> Reviewed-by: Robin Holt <[email protected]>
>> ---
>> arch/x86/kernel/setup.c | 5 +++
>> include/linux/printk.h | 4 ++
>> init/main.c | 1
>> kernel/printk.c | 75 ++++++++++++++++++++++++++++--------------------
>> 4 files changed, 54 insertions(+), 31 deletions(-)
>
> Well, the modern allocation method is memblock - available on all major
> architectures.
>
> You could avoid all this ugly workaround of bootmem limitations by moving the
> allocation to memblock_alloc() and desupporting the log_buf_len= boot parameter
> on non-memblock architectures.
Is it really that ugly? I thought in some ways it cleaned it up.
I'm also hesitant to change code for other arch's when I can't test them. This
approach seemed to be the safest.
> kernel log buffer size can be configured via the .config so they will not be left
> without larger buffers.
We have asked about this, but distros are reluctant to increase memory usage
for their entire installed base. I think we're lucky they bumped it up to 256k
from the default 128k.
>
> Doing this should also have the advantage of getting all the early x86 messages into
> the larger buffer already, reducing the pressure to apply some of the other patches
> in your series.
There are only two and both remove only redundant information.
>
> Thanks,
>
> Ingo
On Mon, Feb 28, 2011 at 12:06 AM, Ingo Molnar <[email protected]> wrote:
>
> * Yinghai Lu <[email protected]> wrote:
>
>> + ? ? new_addr = memblock_alloc(new_log_buf_len, PAGE_SIZE);
>> + ? ? if (new_addr != MEMBLOCK_ERROR)
>> + ? ? ? ? ? ? new_log_buf = __va(new_addr);
>> + ? ? else
>> + ? ? ? ? ? ? new_log_buf = alloc_bootmem(new_log_buf_len);
>
> alloc_bootmem() can fail, especially if someone uses a too large boot parameter
> value - and your code does not check for failure.
alloc_bootmem will panic if it fails.
static void * __init ___alloc_bootmem(unsigned long size, unsigned long align,
unsigned long goal, unsigned long limit)
{
void *mem = ___alloc_bootmem_nopanic(size, align, goal, limit);
if (mem)
return mem;
/*
* Whoops, we cannot satisfy the allocation request.
*/
printk(KERN_ALERT "bootmem alloc of %lu bytes failed!\n", size);
panic("Out of memory");
return NULL;
}
so it should be ok.
Yinghai Lu wrote:
> On 02/27/2011 04:15 AM, Ingo Molnar wrote:
>> * Ingo Molnar <[email protected]> wrote:
>>
>>> You could avoid all this ugly workaround of bootmem limitations by moving the
>>> allocation to memblock_alloc() and desupporting the log_buf_len= boot parameter on
>>> non-memblock architectures.
>> memblock_alloc() could return -ENOSYS on architectures that do not implement it -
>> thus enabling such optional features without ugly #ifdef CONFIG_HAVE_MEMBLOCK
>> conditionals.
>
> Mike,
>
> please check updated patch...
>
> with the memblock change, you don't need to change acpi SRAT handling etc any more.
I had to debug a weird ACPI -> Node mapping last week and the
"improved" SRAT messages helped that considerably. It was
far easier to spot which Node didn't have the correct assignments.
I'd submit that patch even without needing fewer (like 512 lines
max instead of 4096 lines max) bytes in the log buffer.
>
> new buffer will be near high mem under 4g.
Is this really that important? The patchset is ridiculously
simple as it is. Do I really need to spend more time on it?
I've already done 4 revisions of it and responded to all
objections.
I have a far more important patchset that I've been trying to
prepare that actually fixes broken code, instead of trying to
reduce a few thousand bytes in the log buffer.
And everyone that has experienced it here on our lab systems
say they like the improvements (even the ones that I had to
toss because of "that's not the way it was before" kind of
objections.)
Let's move on to far more important problems.
Thanks,
Mike
>
> Thanks
>
> Yinghai
>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index a47fe00..69b8995 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -974,6 +974,11 @@ void __init setup_arch(char **cmdline_p)
> memblock.current_limit = get_max_mapped();
>
> /*
> + * Allocate bigger log buffer as early as possible
> + */
> + setup_log_buf();
> +
> + /*
> * NOTE: On x86-32, only from this point on, fixmaps are ready for use.
> */
>
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> index 62a10c2..c3ade22 100644
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -2,6 +2,8 @@
> #define _LINUX_MEMBLOCK_H
> #ifdef __KERNEL__
>
> +#define MEMBLOCK_ERROR 0
> +
> #ifdef CONFIG_HAVE_MEMBLOCK
> /*
> * Logical memory blocks.
> @@ -20,7 +22,6 @@
> #include <asm/memblock.h>
>
> #define INIT_MEMBLOCK_REGIONS 128
> -#define MEMBLOCK_ERROR 0
>
> struct memblock_region {
> phys_addr_t base;
> @@ -160,6 +161,12 @@ static inline unsigned long memblock_region_reserved_end_pfn(const struct memblo
> #define __initdata_memblock
> #endif
>
> +#else
> +static inline phys_addr_t memblock_alloc(phys_addr_t size, phys_addr_t align)
> +{
> + return MEMBLOCK_ERROR;
> +}
> +
> #endif /* CONFIG_HAVE_MEMBLOCK */
>
> #endif /* __KERNEL__ */
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index ee048e7..fd266a8 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -1,6 +1,8 @@
> #ifndef __KERNEL_PRINTK__
> #define __KERNEL_PRINTK__
>
> +#include <linux/init.h>
> +
> extern const char linux_banner[];
> extern const char linux_proc_banner[];
>
> @@ -89,6 +91,8 @@ int no_printk(const char *fmt, ...)
> extern asmlinkage __attribute__ ((format (printf, 1, 2)))
> void early_printk(const char *fmt, ...);
>
> +void __init setup_log_buf(void);
> +
> extern int printk_needs_cpu(int cpu);
> extern void printk_tick(void);
>
> diff --git a/init/main.c b/init/main.c
> index 33c37c3..2085bb3 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -592,6 +592,7 @@ asmlinkage void __init start_kernel(void)
> * These use large bootmem allocations and must precede
> * kmem_cache_init()
> */
> + setup_log_buf();
> pidhash_init();
> vfs_caches_init_early();
> sort_main_extable();
> diff --git a/kernel/printk.c b/kernel/printk.c
> index 3623152..14fa4d0 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -31,6 +31,7 @@
> #include <linux/smp.h>
> #include <linux/security.h>
> #include <linux/bootmem.h>
> +#include <linux/memblock.h>
> #include <linux/syscalls.h>
> #include <linux/kexec.h>
> #include <linux/kdb.h>
> @@ -162,46 +163,64 @@ void log_buf_kexec_setup(void)
> }
> #endif
>
> +/* requested log_buf_len from kernel cmdline */
> +static unsigned long __initdata new_log_buf_len;
> +
> +/* save requested log_buf_len since it's too early to process it */
> static int __init log_buf_len_setup(char *str)
> {
> unsigned size = memparse(str, &str);
> - unsigned long flags;
>
> if (size)
> size = roundup_pow_of_two(size);
> - if (size > log_buf_len) {
> - unsigned start, dest_idx, offset;
> - char *new_log_buf;
> + if (size > log_buf_len)
> + new_log_buf_len = size;
>
> - new_log_buf = alloc_bootmem(size);
> - if (!new_log_buf) {
> - printk(KERN_WARNING "log_buf_len: allocation failed\n");
> - goto out;
> - }
> + return 0;
> +}
> +early_param("log_buf_len", log_buf_len_setup);
>
> - spin_lock_irqsave(&logbuf_lock, flags);
> - log_buf_len = size;
> - log_buf = new_log_buf;
> -
> - offset = start = min(con_start, log_start);
> - dest_idx = 0;
> - while (start != log_end) {
> - log_buf[dest_idx] = __log_buf[start & (__LOG_BUF_LEN - 1)];
> - start++;
> - dest_idx++;
> - }
> - log_start -= offset;
> - con_start -= offset;
> - log_end -= offset;
> - spin_unlock_irqrestore(&logbuf_lock, flags);
> +void __init setup_log_buf(void)
> +{
> + unsigned long flags;
> + unsigned start, dest_idx, offset;
> + char *new_log_buf;
> + phys_addr_t new_addr;
> + int free;
> +
> + if (!new_log_buf_len)
> + return;
>
> - printk(KERN_NOTICE "log_buf_len: %d\n", log_buf_len);
> + new_addr = memblock_alloc(new_log_buf_len, PAGE_SIZE);
> + if (new_addr != MEMBLOCK_ERROR)
> + new_log_buf = __va(new_addr);
> + else
> + new_log_buf = alloc_bootmem(new_log_buf_len);
> +
> + spin_lock_irqsave(&logbuf_lock, flags);
> + log_buf_len = new_log_buf_len;
> + log_buf = new_log_buf;
> + new_log_buf_len = 0;
> + free = __LOG_BUF_LEN - log_end;
> +
> + offset = start = min(con_start, log_start);
> + dest_idx = 0;
> + while (start != log_end) {
> + unsigned log_idx_mask = start & (__LOG_BUF_LEN - 1);
> +
> + log_buf[dest_idx] = __log_buf[log_idx_mask];
> + start++;
> + dest_idx++;
> }
> -out:
> - return 1;
> -}
> + log_start -= offset;
> + con_start -= offset;
> + log_end -= offset;
> + spin_unlock_irqrestore(&logbuf_lock, flags);
>
> -__setup("log_buf_len=", log_buf_len_setup);
> + pr_info("log_buf_len: %d\n", log_buf_len);
> + pr_info("early log buf free: %d(%d%%)\n",
> + free, (free * 100) / __LOG_BUF_LEN);
> +}
>
> #ifdef CONFIG_BOOT_PRINTK_DELAY
>
>
Because I haven't been able to test on a fully configured
system, this might be crucial info to figure out how to
fix this when it happens on a customer system. Are you
saying this small line is any less important than the
other thousands and thousands of seemingly meaningless
lines?
Ingo Molnar wrote:
> * Yinghai Lu <[email protected]> wrote:
>
>> + pr_info("log_buf_len: %d\n", log_buf_len);
>> + pr_info("early log buf free: %d(%d%%)\n",
>> + free, (free * 100) / __LOG_BUF_LEN);
>
> Such debug printks should be removed from the final version of the patch ...
>
> Thanks,
>
> Ingo
Ingo Molnar wrote:
> * Yinghai Lu <[email protected]> wrote:
>
>> + new_addr = memblock_alloc(new_log_buf_len, PAGE_SIZE);
>> + if (new_addr != MEMBLOCK_ERROR)
>> + new_log_buf = __va(new_addr);
>> + else
>> + new_log_buf = alloc_bootmem(new_log_buf_len);
>
> alloc_bootmem() can fail, especially if someone uses a too large boot parameter
> value - and your code does not check for failure.
alloc_bootmem does panic when it can't allocate memory.
Ingo, we have a "uvconfig" script that sets up the boot parameters (there
are many that are needed to be very specific). It sets up the log_buf_len
to be 8M. We will never overflow memory with that.
And if someone is stupid enough to try and allocate a log buffer that
consumes more memory than they have, then they have a different kind of
problem.
>
> Thanks,
>
> Ingo
On Mon, Feb 28, 2011 at 11:23 AM, Mike Travis <[email protected]> wrote:
>
>
> Yinghai Lu wrote:
>>
>> On 02/27/2011 04:15 AM, Ingo Molnar wrote:
>>>
>>> * Ingo Molnar <[email protected]> wrote:
>>>
>>>> You could avoid all this ugly workaround of bootmem limitations by
>>>> moving the allocation to memblock_alloc() and desupporting the log_buf_len=
>>>> boot parameter on non-memblock architectures.
>>>
>>> memblock_alloc() could return -ENOSYS on architectures that do not
>>> implement it - thus enabling such optional features without ugly #ifdef
>>> CONFIG_HAVE_MEMBLOCK conditionals.
>>
>> Mike,
>>
>> please check updated patch...
>>
>> with the memblock change, you don't need to change acpi SRAT handling etc
>> any more.
>
> I had to debug a weird ACPI -> Node mapping last week and the
> "improved" SRAT messages helped that considerably. ?It was
> far easier to spot which Node didn't have the correct assignments.
> I'd submit that patch even without needing fewer (like 512 lines
> max instead of 4096 lines max) bytes in the log buffer.
Your current change to ACPI srat is not complete yet.
you only handle x2apic entries.
According to ACPI 4.0 spec, We should have mixed entries with apic
entries and x2apic entries.
apic entries are for apic id < 255.
x2apic entries are for apic id > 255.
Yinghai
Yinghai Lu wrote:
> On Mon, Feb 28, 2011 at 11:23 AM, Mike Travis <[email protected]> wrote:
>>
>> Yinghai Lu wrote:
>>> On 02/27/2011 04:15 AM, Ingo Molnar wrote:
>>>> * Ingo Molnar <[email protected]> wrote:
>>>>
>>>>> You could avoid all this ugly workaround of bootmem limitations by
>>>>> moving the allocation to memblock_alloc() and desupporting the log_buf_len=
>>>>> boot parameter on non-memblock architectures.
>>>> memblock_alloc() could return -ENOSYS on architectures that do not
>>>> implement it - thus enabling such optional features without ugly #ifdef
>>>> CONFIG_HAVE_MEMBLOCK conditionals.
>>> Mike,
>>>
>>> please check updated patch...
>>>
>>> with the memblock change, you don't need to change acpi SRAT handling etc
>>> any more.
>> I had to debug a weird ACPI -> Node mapping last week and the
>> "improved" SRAT messages helped that considerably. It was
>> far easier to spot which Node didn't have the correct assignments.
>> I'd submit that patch even without needing fewer (like 512 lines
>> max instead of 4096 lines max) bytes in the log buffer.
>
> Your current change to ACPI srat is not complete yet.
>
> you only handle x2apic entries.
>
> According to ACPI 4.0 spec, We should have mixed entries with apic
> entries and x2apic entries.
> apic entries are for apic id < 255.
> x2apic entries are for apic id > 255.
>
> Yinghai
Are you sure you can run both "legacy" and "x2" apic modes in
the same SSI under the Intel or AMD rules?
(And it's highly probable that you cannot overflow the log
buffer with less than 256 CPU's.)
Thanks,
Mike
On Mon, Feb 28, 2011 at 12:02 PM, Mike Travis <[email protected]> wrote:
>
>
> Yinghai Lu wrote:
>>
>> On Mon, Feb 28, 2011 at 11:23 AM, Mike Travis <[email protected]> wrote:
>>>
>>> Yinghai Lu wrote:
>>>>
>>>> On 02/27/2011 04:15 AM, Ingo Molnar wrote:
>>>>>
>>>>> * Ingo Molnar <[email protected]> wrote:
>>>>>
>>>>>> You could avoid all this ugly workaround of bootmem limitations by
>>>>>> moving the allocation to memblock_alloc() and desupporting the
>>>>>> log_buf_len=
>>>>>> boot parameter on non-memblock architectures.
>>>>>
>>>>> memblock_alloc() could return -ENOSYS on architectures that do not
>>>>> implement it - thus enabling such optional features without ugly #ifdef
>>>>> CONFIG_HAVE_MEMBLOCK conditionals.
>>>>
>>>> Mike,
>>>>
>>>> please check updated patch...
>>>>
>>>> with the memblock change, you don't need to change acpi SRAT handling
>>>> etc
>>>> any more.
>>>
>>> I had to debug a weird ACPI -> Node mapping last week and the
>>> "improved" SRAT messages helped that considerably. ?It was
>>> far easier to spot which Node didn't have the correct assignments.
>>> I'd submit that patch even without needing fewer (like 512 lines
>>> max instead of 4096 lines max) bytes in the log buffer.
>>
>> Your current change to ACPI srat is not complete yet.
>>
>> you only handle x2apic entries.
>>
>> According to ACPI 4.0 spec, We should have mixed entries with apic
>> entries and x2apic entries.
>> apic entries are for apic id < 255.
>> x2apic entries are for apic id > 255.
>>
>> Yinghai
>
> Are you sure you can run both "legacy" and "x2" apic modes in
> the same SSI under the Intel or AMD rules?
>
No.
According to ACPI 4.0 Spec:
even the CPUs are with x2apic mode (aka pre-enabled in BIOS),
if their apic id < 255, BIOS still need to use xapic entries for them.
Yinghai
* Mike Travis <[email protected]> wrote:
> Ingo Molnar wrote:
> >* Yinghai Lu <[email protected]> wrote:
> >
> >>+ pr_info("log_buf_len: %d\n", log_buf_len);
> >>+ pr_info("early log buf free: %d(%d%%)\n",
> >>+ free, (free * 100) / __LOG_BUF_LEN);
> >
> > Such debug printks should be removed from the final version of the patch ...
>
> Because I haven't been able to test on a fully configured
> system, this might be crucial info to figure out how to
> fix this when it happens on a customer system. Are you
> saying this small line is any less important than the
> other thousands and thousands of seemingly meaningless
> lines?
I think you are thinking about this the wrong way.
The real fix is to never even have to think about 'how much early buffer space do we
have left'. I.e. via the memblock allocation the buffer can be enlargened before the
big printks happen.
btw., a sidenote, it would make sense to record cases when we 'lose' printk output,
in the printk buffer itself. (it can be done by reserving a bit and updating the
'lost this many bytes' portion of the buffer, or so.)
Thanks,
Ingo
* Mike Travis <[email protected]> wrote:
> Yinghai Lu wrote:
> > please check updated patch...
> >
> > with the memblock change, you don't need to change acpi SRAT handling etc any
> > more.
>
> I had to debug a weird ACPI -> Node mapping last week and the
> "improved" SRAT messages helped that considerably. It was
> far easier to spot which Node didn't have the correct assignments.
> I'd submit that patch even without needing fewer (like 512 lines
> max instead of 4096 lines max) bytes in the log buffer.
I agree that better compressed output generally makes sense - you just need to solve
the ugliness aspect of it. (or get Len's Acked-by to add that code to drivers/acpi/)
Nevertheless doing this via memblock is obviously more important, as it solves the
early printk log overrun problem once and for all.
> Let's move on to far more important problems.
That's not the threshold for upstream inclusion though. (at least for patches that
we process via the -tip tree)
If you add crap to a single hardware driver then only that hardware is affected, but
if you change the way the printk buffer is allocated it is *very important*, because
like every single kernel message is affected by it.
So we scale up our review threshold with the importance of the piece of code
affected.
Thanks,
Ingo
Subject: memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set
Author: Yinghai Lu <[email protected]>
Add an error return if CONFIG_HAVE_MEMBLOCK is not set instead
of having to add #ifdef CONFIG_HAVE_MEMBLOCK around blocks of
code calling that function.
Signed-off-by: Mike Travis <[email protected]>
---
include/linux/memblock.h | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
--- linux.orig/include/linux/memblock.h
+++ linux/include/linux/memblock.h
@@ -2,6 +2,8 @@
#define _LINUX_MEMBLOCK_H
#ifdef __KERNEL__
+#define MEMBLOCK_ERROR 0
+
#ifdef CONFIG_HAVE_MEMBLOCK
/*
* Logical memory blocks.
@@ -20,7 +22,6 @@
#include <asm/memblock.h>
#define INIT_MEMBLOCK_REGIONS 128
-#define MEMBLOCK_ERROR 0
struct memblock_region {
phys_addr_t base;
@@ -160,6 +161,12 @@ static inline unsigned long memblock_reg
#define __initdata_memblock
#endif
+#else
+static inline phys_addr_t memblock_alloc(phys_addr_t size, phys_addr_t align)
+{
+ return MEMBLOCK_ERROR;
+}
+
#endif /* CONFIG_HAVE_MEMBLOCK */
#endif /* __KERNEL__ */
Subject: printk: Allocate kernel log buffer earlier v2
On larger systems, because of the numerous ACPI, Bootmem and EFI
messages, the static log buffer overflows before the larger one
specified by the log_buf_len param is allocated. Minimize the
overflow by allocating the new log buffer as soon as possible.
The allocation method is passed in as an argument to make
backporting to "pre-memblock" kernels easier.
Signed-off-by: Mike Travis <[email protected]>
Reviewed-by: Jack Steiner <[email protected]>
Reviewed-by: Robin Holt <[email protected]>
---
arch/x86/kernel/setup.c | 13 +++++++
include/linux/kernel.h | 1
include/linux/printk.h | 5 ++
init/main.c | 1
kernel/printk.c | 83 ++++++++++++++++++++++++++++++------------------
5 files changed, 72 insertions(+), 31 deletions(-)
--- linux.orig/arch/x86/kernel/setup.c
+++ linux/arch/x86/kernel/setup.c
@@ -313,6 +313,17 @@ static void __init reserve_brk(void)
_brk_start = 0;
}
+static unsigned long __init reserve_log_buf(unsigned long len)
+{
+ unsigned long mem;
+
+ mem = memblock_alloc(len, PAGE_SIZE);
+ if (mem == MEMBLOCK_ERROR)
+ return 0ULL;
+
+ return (unsigned long)__va(mem);
+}
+
#ifdef CONFIG_BLK_DEV_INITRD
#define MAX_MAP_CHUNK (NR_FIX_BTMAPS << PAGE_SHIFT)
@@ -948,6 +959,8 @@ void __init setup_arch(char **cmdline_p)
if (init_ohci1394_dma_early)
init_ohci1394_dma_on_all_controllers();
#endif
+ /* Allocate bigger log buffer as early as possible */
+ setup_log_buf(reserve_log_buf);
reserve_initrd();
--- linux.orig/include/linux/kernel.h
+++ linux/include/linux/kernel.h
@@ -19,6 +19,7 @@
#include <linux/typecheck.h>
#include <linux/printk.h>
#include <linux/dynamic_debug.h>
+#include <linux/init.h>
#include <asm/byteorder.h>
#include <asm/bug.h>
--- linux.orig/include/linux/printk.h
+++ linux/include/linux/printk.h
@@ -1,6 +1,8 @@
#ifndef __KERNEL_PRINTK__
#define __KERNEL_PRINTK__
+#include <linux/init.h>
+
extern const char linux_banner[];
extern const char linux_proc_banner[];
@@ -89,6 +91,9 @@ int no_printk(const char *fmt, ...)
extern asmlinkage __attribute__ ((format (printf, 1, 2)))
void early_printk(const char *fmt, ...);
+typedef unsigned long (alloc_method)(unsigned long len);
+void __init setup_log_buf(alloc_method *alloc);
+
extern int printk_needs_cpu(int cpu);
extern void printk_tick(void);
--- linux.orig/init/main.c
+++ linux/init/main.c
@@ -504,6 +504,7 @@ asmlinkage void __init start_kernel(void
* These use large bootmem allocations and must precede
* kmem_cache_init()
*/
+ setup_log_buf(NULL);
pidhash_init();
vfs_caches_init_early();
sort_main_extable();
--- linux.orig/kernel/printk.c
+++ linux/kernel/printk.c
@@ -167,46 +167,67 @@ void log_buf_kexec_setup(void)
}
#endif
+/* requested log_buf_len from kernel cmdline */
+static unsigned long __initdata new_log_buf_len;
+
+/* save requested log_buf_len since it's too early to process it */
static int __init log_buf_len_setup(char *str)
{
unsigned size = memparse(str, &str);
- unsigned long flags;
if (size)
size = roundup_pow_of_two(size);
- if (size > log_buf_len) {
- unsigned start, dest_idx, offset;
- char *new_log_buf;
-
- new_log_buf = alloc_bootmem(size);
- if (!new_log_buf) {
- printk(KERN_WARNING "log_buf_len: allocation failed\n");
- goto out;
- }
-
- spin_lock_irqsave(&logbuf_lock, flags);
- log_buf_len = size;
- log_buf = new_log_buf;
-
- offset = start = min(con_start, log_start);
- dest_idx = 0;
- while (start != log_end) {
- log_buf[dest_idx] = __log_buf[start & (__LOG_BUF_LEN - 1)];
- start++;
- dest_idx++;
- }
- log_start -= offset;
- con_start -= offset;
- log_end -= offset;
- spin_unlock_irqrestore(&logbuf_lock, flags);
+ if (size > log_buf_len)
+ new_log_buf_len = size;
- printk(KERN_NOTICE "log_buf_len: %d\n", log_buf_len);
- }
-out:
- return 1;
+ return 0;
}
+early_param("log_buf_len", log_buf_len_setup);
-__setup("log_buf_len=", log_buf_len_setup);
+void __init setup_log_buf(alloc_method *alloc)
+{
+ unsigned long flags;
+ unsigned start, dest_idx, offset;
+ char *new_log_buf;
+ int free;
+
+ if (!new_log_buf_len)
+ return;
+
+ if (alloc)
+ new_log_buf = (char *)alloc(new_log_buf_len);
+ else
+ new_log_buf = alloc_bootmem_nopanic(new_log_buf_len);
+
+ if (!new_log_buf) {
+ pr_err("log_buf_len: %d bytes not available\n", log_buf_len);
+ return;
+ }
+
+ spin_lock_irqsave(&logbuf_lock, flags);
+ log_buf_len = new_log_buf_len;
+ log_buf = new_log_buf;
+ new_log_buf_len = 0;
+ free = __LOG_BUF_LEN - log_end;
+
+ offset = start = min(con_start, log_start);
+ dest_idx = 0;
+ while (start != log_end) {
+ unsigned log_idx_mask = start & (__LOG_BUF_LEN - 1);
+
+ log_buf[dest_idx] = __log_buf[log_idx_mask];
+ start++;
+ dest_idx++;
+ }
+ log_start -= offset;
+ con_start -= offset;
+ log_end -= offset;
+ spin_unlock_irqrestore(&logbuf_lock, flags);
+
+ pr_info("log_buf_len: %d\n", log_buf_len);
+ pr_info("early log buf free: %d(%d%%)\n",
+ free, (free * 100) / __LOG_BUF_LEN);
+}
#ifdef CONFIG_BOOT_PRINTK_DELAY
On Wed, Mar 30, 2011 at 5:41 PM, Mike Travis <[email protected]> wrote:
> Subject: memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set
> Author: Yinghai Lu <[email protected]>
>
> ? ? ? ?Add an error return if CONFIG_HAVE_MEMBLOCK is not set instead
> ? ? ? ?of having to add #ifdef CONFIG_HAVE_MEMBLOCK around blocks of
> ? ? ? ?code calling that function.
>
> Signed-off-by: Mike Travis <[email protected]>
> ---
> include/linux/memblock.h | ? ?9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> --- linux.orig/include/linux/memblock.h
> +++ linux/include/linux/memblock.h
> @@ -2,6 +2,8 @@
> #define _LINUX_MEMBLOCK_H
> #ifdef __KERNEL__
>
> +#define MEMBLOCK_ERROR 0
> +
> #ifdef CONFIG_HAVE_MEMBLOCK
> /*
> ?* Logical memory blocks.
> @@ -20,7 +22,6 @@
> #include <asm/memblock.h>
>
> #define INIT_MEMBLOCK_REGIONS ? 128
> -#define MEMBLOCK_ERROR ? ? ? ? 0
>
> struct memblock_region {
> ? ? ? ?phys_addr_t base;
> @@ -160,6 +161,12 @@ static inline unsigned long memblock_reg
> #define __initdata_memblock
> #endif
>
> +#else
> +static inline phys_addr_t memblock_alloc(phys_addr_t size, phys_addr_t
> align)
> +{
> + ? ? ? return MEMBLOCK_ERROR;
> +}
> +
> #endif /* CONFIG_HAVE_MEMBLOCK */
>
> #endif /* __KERNEL__ */
>
setup_log_buf will pass function pointer, So this one is not needed, right?
Thanks
Yinghai
* Mike Travis <[email protected]> wrote:
> Subject: printk: Allocate kernel log buffer earlier v2
>
> On larger systems, because of the numerous ACPI, Bootmem and EFI
> messages, the static log buffer overflows before the larger one
> specified by the log_buf_len param is allocated. Minimize the
> overflow by allocating the new log buffer as soon as possible.
>
> The allocation method is passed in as an argument to make
> backporting to "pre-memblock" kernels easier.
Hm, all that allocation pointer magic looks a tad too complex and non-obvious.
Why not just make it as simple and obvious as possible for the current kernel -
and we can still mark it -stable and you backport it to the non-memblock
kernel? That keeps compatibility complexity out of upstream ...
Thanks,
Ingo
Yinghai Lu wrote:
> On Wed, Mar 30, 2011 at 5:41 PM, Mike Travis <[email protected]> wrote:
>> Subject: memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set
>> Author: Yinghai Lu <[email protected]>
>>
>> Add an error return if CONFIG_HAVE_MEMBLOCK is not set instead
>> of having to add #ifdef CONFIG_HAVE_MEMBLOCK around blocks of
>> code calling that function.
>>
>> Signed-off-by: Mike Travis <[email protected]>
>> ---
>> include/linux/memblock.h | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> --- linux.orig/include/linux/memblock.h
>> +++ linux/include/linux/memblock.h
>> @@ -2,6 +2,8 @@
>> #define _LINUX_MEMBLOCK_H
>> #ifdef __KERNEL__
>>
>> +#define MEMBLOCK_ERROR 0
>> +
>> #ifdef CONFIG_HAVE_MEMBLOCK
>> /*
>> * Logical memory blocks.
>> @@ -20,7 +22,6 @@
>> #include <asm/memblock.h>
>>
>> #define INIT_MEMBLOCK_REGIONS 128
>> -#define MEMBLOCK_ERROR 0
>>
>> struct memblock_region {
>> phys_addr_t base;
>> @@ -160,6 +161,12 @@ static inline unsigned long memblock_reg
>> #define __initdata_memblock
>> #endif
>>
>> +#else
>> +static inline phys_addr_t memblock_alloc(phys_addr_t size, phys_addr_t
>> align)
>> +{
>> + return MEMBLOCK_ERROR;
>> +}
>> +
>> #endif /* CONFIG_HAVE_MEMBLOCK */
>>
>> #endif /* __KERNEL__ */
>>
>
> setup_log_buf will pass function pointer, So this one is not needed, right?
The other function would need the #ifdef CONFIG_HAVE_MEMBLOCK before calling
memblock_alloc which I thought was the point of this patch? Note we still
have the last fallback of using alloc_boot_mem in kernel/init.c.
Thanks,
Mike
Ingo Molnar wrote:
> * Mike Travis <[email protected]> wrote:
>
>> Subject: printk: Allocate kernel log buffer earlier v2
>>
>> On larger systems, because of the numerous ACPI, Bootmem and EFI
>> messages, the static log buffer overflows before the larger one
>> specified by the log_buf_len param is allocated. Minimize the
>> overflow by allocating the new log buffer as soon as possible.
>>
>> The allocation method is passed in as an argument to make
>> backporting to "pre-memblock" kernels easier.
>
> Hm, all that allocation pointer magic looks a tad too complex and non-obvious.
>
> Why not just make it as simple and obvious as possible for the current kernel -
> and we can still mark it -stable and you backport it to the non-memblock
> kernel? That keeps compatibility complexity out of upstream ...
>
> Thanks,
>
> Ingo
Sure, I can do that.
THanks,
Mike
On Thu, Mar 31, 2011 at 8:23 AM, Mike Travis <[email protected]> wrote:
>
>>
>> setup_log_buf will pass function pointer, So this one is not needed,
>> right?
>
> The other function would need the #ifdef CONFIG_HAVE_MEMBLOCK before calling
> memblock_alloc which I thought was the point of this patch? ?Note we still
> have the last fallback of using alloc_boot_mem in kernel/init.c.
before that patch, it already use bootmem allocation.
So should be ok to drop this one.
Thanks
Yinghai
Was there any further objections to these patches?
Mike Travis wrote:
> Subject: memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set
> Author: Yinghai Lu <[email protected]>
>
> Add an error return if CONFIG_HAVE_MEMBLOCK is not set instead
> of having to add #ifdef CONFIG_HAVE_MEMBLOCK around blocks of
> code calling that function.
>
> Signed-off-by: Mike Travis <[email protected]>
> ---
> include/linux/memblock.h | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> --- linux.orig/include/linux/memblock.h
> +++ linux/include/linux/memblock.h
> @@ -2,6 +2,8 @@
> #define _LINUX_MEMBLOCK_H
> #ifdef __KERNEL__
>
> +#define MEMBLOCK_ERROR 0
> +
> #ifdef CONFIG_HAVE_MEMBLOCK
> /*
> * Logical memory blocks.
> @@ -20,7 +22,6 @@
> #include <asm/memblock.h>
>
> #define INIT_MEMBLOCK_REGIONS 128
> -#define MEMBLOCK_ERROR 0
>
> struct memblock_region {
> phys_addr_t base;
> @@ -160,6 +161,12 @@ static inline unsigned long memblock_reg
> #define __initdata_memblock
> #endif
>
> +#else
> +static inline phys_addr_t memblock_alloc(phys_addr_t size, phys_addr_t
> align)
> +{
> + return MEMBLOCK_ERROR;
> +}
> +
> #endif /* CONFIG_HAVE_MEMBLOCK */
>
> #endif /* __KERNEL__ */
>
>
* Mike Travis <[email protected]> wrote:
> Was there any further objections to these patches?
No big objections that i remember - so assuming review feedback has been
addressed please send the latest and greatest in a new thread, this one is
getting a bit deep :-)
Thanks,
Ingo