[PATCH 1/2] memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set

[PATCH 1/2] memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set

[Mike Travis]

Subject: memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set
Author: Yinghai Lu <yinghai@kernel.org>

	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 <travis@sgi.com>
---
 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__ */

Re: [PATCH 1/2] memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set

[Yinghai Lu]

On Wed, Mar 30, 2011 at 5:41 PM, Mike Travis <travis@sgi.com> wrote:
> Subject: memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set
> Author: Yinghai Lu <yinghai@kernel.org>
>
>        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 <travis@sgi.com>
> ---
> 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

Re: [PATCH 1/2] memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set

[Mike Travis]

Yinghai Lu wrote:
> On Wed, Mar 30, 2011 at 5:41 PM, Mike Travis <travis@sgi.com> wrote:
>> Subject: memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set
>> Author: Yinghai Lu <yinghai@kernel.org>
>>
>>        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 <travis@sgi.com>
>> ---
>> 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

Re: [PATCH 1/2] memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set

[Yinghai Lu]

On Thu, Mar 31, 2011 at 8:23 AM, Mike Travis <travis@sgi.com> 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

Re: [PATCH 1/2] memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set

[Mike Travis]

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 <yinghai@kernel.org>
> 
>     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 <travis@sgi.com>
> ---
> 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__ */
> 
> 

Re: [PATCH 1/2] memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set

[Ingo Molnar]

* Mike Travis <travis@sgi.com> 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

[PATCH 0/2] printk: Allocate log buffer as early as possible

[Mike Travis]

On larger systems, information in the kernel log is lost because
there is so much early text printed, that it overflows the static
log buffer before the log_buf_len kernel parameter can be processed,
and a bigger log buffer allocated.

Distros are relunctant to increase memory usage by increasing the
size of the static log buffer, so minimize the problem by allocating
the new log buffer as early as possible.

-- 

[PATCH 1/2] memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set

[Mike Travis]

[-- Attachment #1: add-no-memblock --]
[-- Type: text/plain, Size: 1013 bytes --]

	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.

Authored-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Mike Travis <travis@sgi.com>
---
 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__ */

-- 

[PATCH 2/2] printk: Allocate kernel log buffer earlier v2

[Mike Travis]

[-- Attachment #1: get_log_buff_early --]
[-- Type: text/plain, Size: 4487 bytes --]

	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.

	On kernels without memblock, a later call to setup_log_buf
	from kernel/init.c is the fallback.

Signed-off-by: Mike Travis <travis@sgi.com>
---
 arch/x86/kernel/setup.c |    2 +
 include/linux/printk.h  |    4 ++
 init/main.c             |    1 
 kernel/printk.c         |   91 +++++++++++++++++++++++++++++++-----------------
 4 files changed, 67 insertions(+), 31 deletions(-)

--- linux.orig/arch/x86/kernel/setup.c
+++ linux/arch/x86/kernel/setup.c
@@ -975,6 +975,8 @@ void __init setup_arch(char **cmdline_p)
 	if (init_ohci1394_dma_early)
 		init_ohci1394_dma_on_all_controllers();
 #endif
+	/* Allocate bigger log buffer */
+	setup_log_buf(1);
 
 	reserve_initrd();
 
--- 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(int early);
+
 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(0);
 	pidhash_init();
 	vfs_caches_init_early();
 	sort_main_extable();
--- linux.orig/kernel/printk.c
+++ linux/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,74 @@ 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(int early)
+{
+	unsigned long flags;
+	unsigned start, dest_idx, offset;
+	char *new_log_buf;
+	int free;
+
+	if (!new_log_buf_len)
+		return;
+
+	if (early) {
+		unsigned long mem;
+
+		mem = memblock_alloc(new_log_buf_len, PAGE_SIZE);
+		if (mem == MEMBLOCK_ERROR)
+			return;
+		new_log_buf = __va(mem);
+	}
+	else
+		new_log_buf = alloc_bootmem_nopanic(new_log_buf_len);
+
+	if (unlikely(!new_log_buf)) {
+		pr_err("log_buf_len: %ld bytes not available\n",
+			new_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
 

-- 

Re: [PATCH 1/2] memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set

[Andrew Morton]

On Mon, 25 Apr 2011 13:11:37 -0500
Mike Travis <travis@sgi.com> wrote:

> 	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.
> 
> Authored-by: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Mike Travis <travis@sgi.com>

There is no such thing as "Authored-by:".  If this patch was written by
yinghai then it must be tagged as From:him at the top of the changelog
and preferably has his signed-off-by: at the end.

Please clarify?

Re: [PATCH 0/2] printk: Allocate log buffer as early as possible

[Ingo Molnar]

* Mike Travis <travis@sgi.com> wrote:

> 
> On larger systems, information in the kernel log is lost because
> there is so much early text printed, that it overflows the static
> log buffer before the log_buf_len kernel parameter can be processed,
> and a bigger log buffer allocated.
> 
> Distros are relunctant to increase memory usage by increasing the
> size of the static log buffer, so minimize the problem by allocating
> the new log buffer as early as possible.

I think this looks sane in principle and will solve quite a few 'the log buffer 
got too large' problems which previously resulted in somewhat complex looking 
printk-reduction patches. Andrew, if you agree with the patches, wanna carry 
them?

Thanks,

	Ingo

Re: [PATCH 1/2] memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set

[Mike Travis]

Andrew Morton wrote:
> On Mon, 25 Apr 2011 13:11:37 -0500
> Mike Travis <travis@sgi.com> wrote:
> 
>> 	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.
>>
>> Authored-by: Yinghai Lu <yinghai@kernel.org>
>> Signed-off-by: Mike Travis <travis@sgi.com>
> 
> There is no such thing as "Authored-by:".  If this patch was written by
> yinghai then it must be tagged as From:him at the top of the changelog
> and preferably has his signed-off-by: at the end.
> 
> Please clarify?

Yes, you have it correct.  I had added the From: line but when I receive
the email, it's removed.

Why not have an "Authored-by"?  That would eliminate the sendmail program
from screwing it up?

Thanks,
Mike

Re: [PATCH 1/2] memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set

[Ingo Molnar]

* Mike Travis <travis@sgi.com> wrote:

> 
> 
> Andrew Morton wrote:
> >On Mon, 25 Apr 2011 13:11:37 -0500
> >Mike Travis <travis@sgi.com> wrote:
> >
> >>	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.
> >>
> >>Authored-by: Yinghai Lu <yinghai@kernel.org>
> >>Signed-off-by: Mike Travis <travis@sgi.com>
> >
> >There is no such thing as "Authored-by:".  If this patch was written by
> >yinghai then it must be tagged as From:him at the top of the changelog
> >and preferably has his signed-off-by: at the end.
> >
> >Please clarify?
> 
> Yes, you have it correct.  I had added the From: line but when I receive
> the email, it's removed.
> 
> Why not have an "Authored-by"?  That would eliminate the sendmail program
> from screwing it up?

"From:" headers are properly recognized by Git and both git log and git 
annotate will show the right authorship. Authored-by does not get propagated.

Also, sendmail does not screw up From: headers that are in the body of the 
email - forwarding emails is one of the oldest things that can be done to 
emails.

Thanks,

	Ingo

Re: [PATCH 1/2] memblock: add error return when CONFIG_HAVE_MEMBLOCK is not set

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 829 bytes --]

On Wed, 27 Apr 2011 08:33:18 +0200, Ingo Molnar said:
> Also, sendmail does not screw up From: headers that are in the body of the 
> email - forwarding emails is one of the oldest things that can be done to 
> emails.

What some Sendmail configurations *do* eat is lines that start with 'From ' (no
colon). Those will get escaped as '>From ...'  to prevent them from looking
like a separator line for what's known as 'mbox' format - which is basically
the same info as in the Return-Path: header plus a timestamp. They look like:

>From hostmaster@prairie.net Fri Apr 19 15:50:15 2002

(If the above line has a >From, you have the issue.  Yes, it could be smarter
and do a full parse for a mbox header rather than just '^From '.  And yes, I
had to go digging to find an old archived mbox format file to snarf the example
from.


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]