[PATCH v8 1/4] printk: make dynamic kernel ring buffer alignment explicit

[PATCH v8 1/4] printk: make dynamic kernel ring buffer alignment explicit

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

We have to consider alignment for the ring buffer both for the
default static size, and then also for when an dynamic allocation
is made when the log_buf_len=n kernel parameter is passed to set
the size specifically to a size larger than the default size set
by the architecture through CONFIG_LOG_BUF_SHIFT.

The default static kernel ring buffer can be aligned properly if
architectures set CONFIG_LOG_BUF_SHIFT properly, we provide ranges
for the size though so even if CONFIG_LOG_BUF_SHIFT has a sensible
aligned value it can be reduced to a non aligned value. Commit
6ebb017de9 by Andrew ensures the static buffer is always aligned
and the decision of alignment is done by the compiler by using
__alignof__(struct log).

When log_buf_len=n is used we allocate the ring buffer dynamically.
Dynamic allocation varies, for the early allocation called
before setup_arch() memblock_virt_alloc() requests a page aligment
and for the default kernel allocation memblock_virt_alloc_nopanic()
requests no special alignment, which in turn ends up aligning the
allocation to SMP_CACHE_BYTES, which is L1 cache aligned.

Since we already have the required alignment for the kernel ring
buffer though we can do better and request explicit alignment for
LOG_ALIGN. This does that to be safe and make dynamic allocation
alignment explicit.

Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: Petr Mladek <pmladek@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Joe Perches <joe@perches.com>
Cc: Arun KS <arunks.linux@gmail.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Davidlohr Bueso <davidlohr@hp.com>
Cc: Chris Metcalf <cmetcalf@tilera.com>
Cc: linux-kernel@vger.kernel.org
Acked-by: Petr Mladek <pmladek@suse.cz>
Tested-by: Petr Mladek <pmladek@suse.cz>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 kernel/printk/printk.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ea2d5f6..78ceccf 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -853,9 +853,10 @@ void __init setup_log_buf(int early)
 
 	if (early) {
 		new_log_buf =
-			memblock_virt_alloc(new_log_buf_len, PAGE_SIZE);
+			memblock_virt_alloc(new_log_buf_len, LOG_ALIGN);
 	} else {
-		new_log_buf = memblock_virt_alloc_nopanic(new_log_buf_len, 0);
+		new_log_buf = memblock_virt_alloc_nopanic(new_log_buf_len,
+							  LOG_ALIGN);
 	}
 
 	if (unlikely(!new_log_buf)) {
-- 
1.9.3

[PATCH v8 2/4] printk: move power of 2 practice of ring buffer size to a helper

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

In practice the power of 2 practice of the size of the kernel ring
buffer remains purely historical but not a requirement, specially
now that we have LOG_ALIGN and use it for both static and dynamic
allocations. It could have helped with implicit alignment back in
the days given the even the dynamically sized ring buffer was
guaranteed to be aligned so long as CONFIG_LOG_BUF_SHIFT was set to
produce a __LOG_BUF_LEN which is architecture aligned, since
log_buf_len=n would be allowed only if it was > __LOG_BUF_LEN and
we always ended up rounding the log_buf_len=n to the next power
of 2 with roundup_pow_of_two(), any multiple of 2 then should be also
architecture aligned. These assumptions of course relied heavily
on CONFIG_LOG_BUF_SHIFT producing an aligned value but users can
always change this.

We now have precise alignment requirements set for the log buffer
size for both static and dynamic allocations, but lets upkeep the
old practice of using powers of 2 for its size to help with easy
expected scalable values and the allocators for dynamic allocations.
We'll reuse this later so move this into a helper.

Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: Petr Mladek <pmladek@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Joe Perches <joe@perches.com>
Cc: Arun KS <arunks.linux@gmail.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Davidlohr Bueso <davidlohr@hp.com>
Cc: Chris Metcalf <cmetcalf@tilera.com>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 kernel/printk/printk.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 78ceccf..af164a7 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -828,15 +828,21 @@ void log_buf_kexec_setup(void)
 /* 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)
+/* we practice scaling the ring buffer by powers of 2 */
+static void __init log_buf_len_update(unsigned size)
 {
-	unsigned size = memparse(str, &str);
-
 	if (size)
 		size = roundup_pow_of_two(size);
 	if (size > log_buf_len)
 		new_log_buf_len = size;
+}
+
+/* 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);
+
+	log_buf_len_update(size);
 
 	return 0;
 }
-- 
1.9.3

[PATCH v8 3/4] printk: make dynamic units clear for the kernel ring buffer

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: Petr Mladek <pmladek@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Joe Perches <joe@perches.com>
Cc: Arun KS <arunks.linux@gmail.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Davidlohr Bueso <davidlohr@hp.com>
Cc: Chris Metcalf <cmetcalf@tilera.com>
Cc: linux-kernel@vger.kernel.org
Suggested-by: Davidlohr Bueso <davidlohr@hp.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 kernel/printk/printk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index af164a7..7bf1365 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -879,7 +879,7 @@ void __init setup_log_buf(int early)
 	memcpy(log_buf, __log_buf, __LOG_BUF_LEN);
 	raw_spin_unlock_irqrestore(&logbuf_lock, flags);
 
-	pr_info("log_buf_len: %d\n", log_buf_len);
+	pr_info("log_buf_len: %d bytes\n", log_buf_len);
 	pr_info("early log buf free: %d(%d%%)\n",
 		free, (free * 100) / __LOG_BUF_LEN);
 }
-- 
1.9.3

[PATCH v8 4/4] printk: allow increasing the ring buffer depending on the number of CPUs

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

The default size of the ring buffer is too small for machines
with a large amount of CPUs under heavy load. What ends up
happening when debugging is the ring buffer overlaps and chews
up old messages making debugging impossible unless the size is
passed as a kernel parameter. An idle system upon boot up will
on average spew out only about one or two extra lines but where
this really matters is on heavy load and that will vary widely
depending on the system and environment.

There are mechanisms to help increase the kernel ring buffer
for tracing through debugfs, and those interfaces even allow growing
the kernel ring buffer per CPU. We also have a static value which
can be passed upon boot. Relying on debugfs however is not ideal
for production, and relying on the value passed upon bootup is
can only used *after* an issue has creeped up. Instead of being
reactive this adds a proactive measure which lets you scale the
amount of contributions you'd expect to the kernel ring buffer
under load by each CPU in the worst case scenario.

We use num_possible_cpus() to avoid complexities which could be
introduced by dynamically changing the ring buffer size at run
time, num_possible_cpus() lets us use the upper limit on possible
number of CPUs therefore avoiding having to deal with hotplugging
CPUs on and off. This introduces the kernel configuration option
LOG_CPU_MAX_BUF_SHIFT which is used to specify the maximum amount
of contributions to the kernel ring buffer in the worst case before
the kernel ring buffer flips over, the size is specified as a power
of 2. The total amount of contributions made by each CPU must be
greater than half of the default kernel ring buffer size
(1 << LOG_BUF_SHIFT bytes) in order to trigger an increase upon
bootup. The kernel ring buffer is increased to the next power of
two that would fit the required minimum kernel ring buffer size
plus the additional CPU contribution. For example if LOG_BUF_SHIFT
is 18 (256 KB) you'd require at least 128 KB contributions by
other CPUs in order to trigger an increase of the kernel ring buffer.
With a LOG_CPU_BUF_SHIFT of 12 (4 KB) you'd require at least
anything over > 64 possible CPUs to trigger an increase. If you
had 128 possible CPUs the amount of minimum required kernel ring
buffer bumps to:

   ((1 << 18) + ((128 - 1) * (1 << 12))) / 1024 = 764 KB

Since we require the ring buffer to be a power of two the new
required size would be 1024 KB.

This CPU contributions are ignored when the "log_buf_len" kernel parameter
is used as it forces the exact size of the ring buffer to an expected power
of two value.

Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: Petr Mladek <pmladek@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Joe Perches <joe@perches.com>
Cc: Arun KS <arunks.linux@gmail.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Davidlohr Bueso <davidlohr@hp.com>
Cc: Chris Metcalf <cmetcalf@tilera.com>
Cc: linux-kernel@vger.kernel.org
Reviewed-by: Davidlohr Bueso <davidlohr@hp.com>
Tested-by: Davidlohr Bueso <davidlohr@hp.com>
Tested-by: Petr Mladek <pmladek@suse.cz>
Signed-off-by: Petr Mladek <pmladek@suse.cz>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 Documentation/kernel-parameters.txt |  8 ++++--
 init/Kconfig                        | 53 ++++++++++++++++++++++++++++++++++++-
 kernel/printk/printk.c              | 34 ++++++++++++++++++++++++
 3 files changed, 92 insertions(+), 3 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 6eaa9cd..4cf08f5 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1685,8 +1685,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			7 (KERN_DEBUG)		debug-level messages
 
 	log_buf_len=n[KMG]	Sets the size of the printk ring buffer,
-			in bytes.  n must be a power of two.  The default
-			size is set in the kernel config file.
+			in bytes.  n must be a power of two and greater
+			than the minimal size. The minimal size is defined
+			by LOG_BUF_SHIFT kernel config parameter. There is
+			also CONFIG_LOG_CPU_MAX_BUF_SHIFT config parameter
+			that allows to increase the default size depending on
+			the number of CPUs. See init/Kconfig for more details.
 
 	logo.nologo	[FB] Disables display of the built-in Linux logo.
 			This may be used to provide more screen space for
diff --git a/init/Kconfig b/init/Kconfig
index 9d76b99..573d3f6 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -807,7 +807,11 @@ config LOG_BUF_SHIFT
 	range 12 21
 	default 17
 	help
-	  Select kernel log buffer size as a power of 2.
+	  Select the minimal kernel log buffer size as a power of 2.
+	  The final size is affected by LOG_CPU_MAX_BUF_SHIFT config
+	  parameter, see below. Any higher size also might be forced
+	  by "log_buf_len" boot parameter.
+
 	  Examples:
 	  	     17 => 128 KB
 		     16 => 64 KB
@@ -816,6 +820,53 @@ config LOG_BUF_SHIFT
 		     13 =>  8 KB
 		     12 =>  4 KB
 
+config LOG_CPU_MAX_BUF_SHIFT
+	int "CPU kernel log buffer size contribution (13 => 8 KB, 17 => 128KB)"
+	range 0 21
+	default 12
+	depends on SMP
+	depends on !BASE_SMALL
+	help
+	  The kernel ring buffer will get additional data logged onto it
+	  when multiple CPUs are supported. Typically the contributions are
+	  only a few lines when idle however under under load this can vary
+	  and in the worst case it can mean losing logging information. You
+	  can use this to set the maximum expected amount of logging
+	  contribution under load by each CPU in the worst case scenario, as
+	  a power of 2. The total sum amount of contributions made by all CPUs
+	  must be greater than half of the default kernel ring buffer size
+	  ((1 << LOG_BUF_SHIFT / 2 bytes)) in order to trigger an increase upon
+	  bootup. If an increase is required the ring buffer is increased to
+	  the next power of 2 that can fit both the minimum kernel ring buffer
+	  (LOG_BUF_SHIFT) plus the additional worst case CPU contributions.
+	  For example if LOG_BUF_SHIFT is 18 (256 KB) you'd require at laest
+	  128 KB contributions by other CPUs in order to trigger an increase.
+	  With a LOG_CPU_BUF_SHIFT of 12 (4 KB) you'd require at least anything
+	  over > 64 possible CPUs to trigger an increase. If you had 128
+	  possible CPUs the new minimum required kernel ring buffer size
+	  would be:
+
+	     ((1 << 18) + ((128 - 1) * (1 << 12))) / 1024 = 764 KB
+
+	  Since we only allow powers of two for the kernel ring buffer size the
+	  new kernel ring buffer size would be 1024 KB.
+
+	  CPU contributions are ignored when "log_buf_len" kernel parameter is
+	  used as it forces an exact (power of two) size of the ring buffer to
+	  an expected value.
+
+	  The number of possible CPUs is used for this computation ignoring
+	  hotplugging making the compuation optimal for the the worst case
+	  scenerio while allowing a simple algorithm to be used from bootup.
+
+	  Examples shift values and their meaning:
+		     17 => 128 KB for each CPU
+		     16 =>  64 KB for each CPU
+		     15 =>  32 KB for each CPU
+		     14 =>  16 KB for each CPU
+		     13 =>   8 KB for each CPU
+		     12 =>   4 KB for each CPU
+
 #
 # Architectures with an unreliable sched_clock() should select this:
 #
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 7bf1365..83f7a95 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -266,6 +266,7 @@ static u32 clear_idx;
 #define LOG_ALIGN __alignof__(struct printk_log)
 #endif
 #define __LOG_BUF_LEN (1 << CONFIG_LOG_BUF_SHIFT)
+#define __LOG_CPU_MAX_BUF_LEN (1 << CONFIG_LOG_CPU_MAX_BUF_SHIFT)
 static char __log_buf[__LOG_BUF_LEN] __aligned(LOG_ALIGN);
 static char *log_buf = __log_buf;
 static u32 log_buf_len = __LOG_BUF_LEN;
@@ -848,12 +849,45 @@ static int __init log_buf_len_setup(char *str)
 }
 early_param("log_buf_len", log_buf_len_setup);
 
+static void __init log_buf_add_cpu(void)
+{
+	unsigned int cpu_extra;
+
+	/*
+	 * archs should set up cpu_possible_bits properly with
+	 * set_cpu_possible() after setup_arch() but just in
+	 * case lets ensure this is valid.
+	 */
+	if (num_possible_cpus() == 1)
+		return;
+
+	cpu_extra = (num_possible_cpus() - 1) * __LOG_CPU_MAX_BUF_LEN;
+
+	/* by default this will only continue through for large > 64 CPUs */
+	if (cpu_extra <= __LOG_BUF_LEN / 2)
+		return;
+
+	pr_info("log_buf_len individual max cpu contribution: %d bytes\n",
+		__LOG_CPU_MAX_BUF_LEN);
+	pr_info("log_buf_len total cpu_extra contributions: %d bytes\n",
+		cpu_extra);
+	pr_info("log_buf_len min size: %d bytes\n", __LOG_BUF_LEN);
+
+	log_buf_len_update(cpu_extra + __LOG_BUF_LEN);
+}
+
 void __init setup_log_buf(int early)
 {
 	unsigned long flags;
 	char *new_log_buf;
 	int free;
 
+	if (log_buf != __log_buf)
+		return;
+
+	if (!early && !new_log_buf_len)
+		log_buf_add_cpu();
+
 	if (!new_log_buf_len)
 		return;
 
-- 
1.9.3

Re: [PATCH v8 4/4] printk: allow increasing the ring buffer depending on the number of CPUs

[Andrew Morton]

On Wed, 18 Jun 2014 13:45:37 -0700 "Luis R. Rodriguez" <mcgrof@do-not-panic.com> wrote:

> ...
>
> If an increase is required the ring buffer is increased to
> +	  the next power of 2 that can fit both the minimum kernel ring buffer
> +	  (LOG_BUF_SHIFT) plus the additional worst case CPU contributions.
>
> ...
>
> +	log_buf_len_update(cpu_extra + __LOG_BUF_LEN);
> +}

I'd have expected

  total_cpu_space = minimum-per-cpu-len * nr_possible_cpus;
  log_buf_len = max(__LOG_BUF_LEN, total_cpu_space)

but here you added __LOG_BUF_LEN to total_cpu_space and I cannot work
out why.  

Re: [PATCH v8 4/4] printk: allow increasing the ring buffer depending on the number of CPUs

[Luis R. Rodriguez]

On Mon, Jun 23, 2014 at 03:41:34PM -0700, Andrew Morton wrote:
> On Wed, 18 Jun 2014 13:45:37 -0700 "Luis R. Rodriguez" <mcgrof@do-not-panic.com> wrote:
> 
> > ...
> >
> > If an increase is required the ring buffer is increased to
> > +	  the next power of 2 that can fit both the minimum kernel ring buffer
> > +	  (LOG_BUF_SHIFT) plus the additional worst case CPU contributions.
> >
> > ...
> >
> > +	log_buf_len_update(cpu_extra + __LOG_BUF_LEN);
> > +}
> 
> I'd have expected
> 
>   total_cpu_space = minimum-per-cpu-len * nr_possible_cpus;
>   log_buf_len = max(__LOG_BUF_LEN, total_cpu_space)
> 
> but here you added __LOG_BUF_LEN to total_cpu_space and I cannot work
> out why.  
> .

Ah, because its cpu_extra, not total_cpu_space that is being
computed, the goal was to see how much extra junk on the
worst case a CPU might contribute. The __LOG_BUF_LEN is the
default size, so we combine both.

  Luis

Re: [PATCH v8 4/4] printk: allow increasing the ring buffer depending on the number of CPUs

[Andrew Morton]

On Tue, 24 Jun 2014 02:20:50 +0200 "Luis R. Rodriguez" <mcgrof@suse.com> wrote:

> On Mon, Jun 23, 2014 at 03:41:34PM -0700, Andrew Morton wrote:
> > On Wed, 18 Jun 2014 13:45:37 -0700 "Luis R. Rodriguez" <mcgrof@do-not-panic.com> wrote:
> > 
> > > ...
> > >
> > > If an increase is required the ring buffer is increased to
> > > +	  the next power of 2 that can fit both the minimum kernel ring buffer
> > > +	  (LOG_BUF_SHIFT) plus the additional worst case CPU contributions.
> > >
> > > ...
> > >
> > > +	log_buf_len_update(cpu_extra + __LOG_BUF_LEN);
> > > +}
> > 
> > I'd have expected
> > 
> >   total_cpu_space = minimum-per-cpu-len * nr_possible_cpus;
> >   log_buf_len = max(__LOG_BUF_LEN, total_cpu_space)
> > 
> > but here you added __LOG_BUF_LEN to total_cpu_space and I cannot work
> > out why.  
> > .
> 
> Ah, because its cpu_extra, not total_cpu_space that is being
> computed, the goal was to see how much extra junk on the
> worst case a CPU might contribute. The __LOG_BUF_LEN is the
> default size, so we combine both.

Well...  why?  Isn't it simpler and more direct to say "I want at least
32k per CPU"?

Re: [PATCH v8 4/4] printk: allow increasing the ring buffer depending on the number of CPUs

[Luis R. Rodriguez]

On Mon, Jun 23, 2014 at 05:45:33PM -0700, Andrew Morton wrote:
> On Tue, 24 Jun 2014 02:20:50 +0200 "Luis R. Rodriguez" <mcgrof@suse.com> wrote:
> 
> > On Mon, Jun 23, 2014 at 03:41:34PM -0700, Andrew Morton wrote:
> > > On Wed, 18 Jun 2014 13:45:37 -0700 "Luis R. Rodriguez" <mcgrof@do-not-panic.com> wrote:
> > > 
> > > > ...
> > > >
> > > > If an increase is required the ring buffer is increased to
> > > > +	  the next power of 2 that can fit both the minimum kernel ring buffer
> > > > +	  (LOG_BUF_SHIFT) plus the additional worst case CPU contributions.
> > > >
> > > > ...
> > > >
> > > > +	log_buf_len_update(cpu_extra + __LOG_BUF_LEN);
> > > > +}
> > > 
> > > I'd have expected
> > > 
> > >   total_cpu_space = minimum-per-cpu-len * nr_possible_cpus;
> > >   log_buf_len = max(__LOG_BUF_LEN, total_cpu_space)
> > > 
> > > but here you added __LOG_BUF_LEN to total_cpu_space and I cannot work
> > > out why.  
> > > .
> > 
> > Ah, because its cpu_extra, not total_cpu_space that is being
> > computed, the goal was to see how much extra junk on the
> > worst case a CPU might contribute. The __LOG_BUF_LEN is the
> > default size, so we combine both.
> 
> Well...  why?  Isn't it simpler and more direct to say "I want at least
> 32k per CPU"?

That's certainly another way to go about this, but the original motivation
was trying to figure out the additional *extra* junk a CPU might spewed out,
it set out with an assumption of a base start from the first CPU booting the
system and that first CPU using the default kernel ring buffer size. The
language in the patch describes the worst case extra CPU junk contributed,
rather than a specific full split of the kernel ring buffer as that's typically
how extra junk is spewered out to the ring bufer and the concern. In general
on idle each CPU only contributes about only 1 to max 2 lines. The focus then
is the worst case on contribution.

Another note --  since this option depends on SMP and !BASE_SMALL technically 
num_possible_cpus() won't ever return something smaller than or equal to 1
but because of the default values chosen the -1 on the compuation does affect
whether or not this will trigger on > 64 CPUs or >= 64 CPUs, keeping the
-1 means we require > 64 CPUs.

This all can be changed however we like but the language and explained logic
would just need to be changed.

  Luis

Re: [PATCH v8 4/4] printk: allow increasing the ring buffer depending on the number of CPUs

[Andrew Morton]

On Tue, 24 Jun 2014 03:05:54 +0200 "Luis R. Rodriguez" <mcgrof@suse.com> wrote:

> > > Ah, because its cpu_extra, not total_cpu_space that is being
> > > computed, the goal was to see how much extra junk on the
> > > worst case a CPU might contribute. The __LOG_BUF_LEN is the
> > > default size, so we combine both.
> > 
> > Well...  why?  Isn't it simpler and more direct to say "I want at least
> > 32k per CPU"?
> 
> That's certainly another way to go about this, but the original motivation
> was trying to figure out the additional *extra* junk a CPU might spewed out,
> it set out with an assumption of a base start from the first CPU booting the
> system and that first CPU using the default kernel ring buffer size. The
> language in the patch describes the worst case extra CPU junk contributed,
> rather than a specific full split of the kernel ring buffer as that's typically
> how extra junk is spewered out to the ring bufer and the concern. In general
> on idle each CPU only contributes about only 1 to max 2 lines. The focus then
> is the worst case on contribution.

I don't think I understood all that ;)

> Another note --  since this option depends on SMP and !BASE_SMALL technically 
> num_possible_cpus() won't ever return something smaller than or equal to 1
> but because of the default values chosen the -1 on the compuation does affect
> whether or not this will trigger on > 64 CPUs or >= 64 CPUs, keeping the
> -1 means we require > 64 CPUs.

hm, that sounds like more complexity.

> This all can be changed however we like but the language and explained logic
> would just need to be changed.

Let's start out simple.  What's wrong with doing

	log buf len = max(__LOG_BUF_LEN, nr_possible_cpus * per-cpu log buf len)

?

Re: [PATCH v8 4/4] printk: allow increasing the ring buffer depending on the number of CPUs

[Luis R. Rodriguez]

On Thu, Jun 26, 2014 at 02:41:17PM -0700, Andrew Morton wrote:
> On Tue, 24 Jun 2014 03:05:54 +0200 "Luis R. Rodriguez" <mcgrof@suse.com> wrote:
> 
> > > > Ah, because its cpu_extra, not total_cpu_space that is being
> > > > computed, the goal was to see how much extra junk on the
> > > > worst case a CPU might contribute. The __LOG_BUF_LEN is the
> > > > default size, so we combine both.
> > > 
> > > Well...  why?  Isn't it simpler and more direct to say "I want at least
> > > 32k per CPU"?
> > 
> > That's certainly another way to go about this, but the original motivation
> > was trying to figure out the additional *extra* junk a CPU might spewed out,
> > it set out with an assumption of a base start from the first CPU booting the
> > system and that first CPU using the default kernel ring buffer size. The
> > language in the patch describes the worst case extra CPU junk contributed,
> > rather than a specific full split of the kernel ring buffer as that's typically
> > how extra junk is spewered out to the ring bufer and the concern. In general
> > on idle each CPU only contributes about only 1 to max 2 lines. The focus then
> > is the worst case on contribution.
> 
> I don't think I understood all that ;)

Yeah if that made *you* squint a simpler approach would be better, regardless
of how technically correct the above explanation may be.

> > Another note --  since this option depends on SMP and !BASE_SMALL technically 
> > num_possible_cpus() won't ever return something smaller than or equal to 1
> > but because of the default values chosen the -1 on the compuation does affect
> > whether or not this will trigger on > 64 CPUs or >= 64 CPUs, keeping the
> > -1 means we require > 64 CPUs.
> 
> hm, that sounds like more complexity.
> 
> > This all can be changed however we like but the language and explained logic
> > would just need to be changed.
> 
> Let's start out simple.  What's wrong with doing
> 
> 	log buf len = max(__LOG_BUF_LEN, nr_possible_cpus * per-cpu log buf len)

Sure, you already took in the patch series though so how would you like to
handle a respin, you just drop the last patch and we respin it?

  Luis

Re: [PATCH v8 4/4] printk: allow increasing the ring buffer depending on the number of CPUs

[Andrew Morton]

On Fri, 27 Jun 2014 01:16:30 +0200 "Luis R. Rodriguez" <mcgrof@suse.com> wrote:

> > > Another note --  since this option depends on SMP and !BASE_SMALL technically 
> > > num_possible_cpus() won't ever return something smaller than or equal to 1
> > > but because of the default values chosen the -1 on the compuation does affect
> > > whether or not this will trigger on > 64 CPUs or >= 64 CPUs, keeping the
> > > -1 means we require > 64 CPUs.
> > 
> > hm, that sounds like more complexity.
> > 
> > > This all can be changed however we like but the language and explained logic
> > > would just need to be changed.
> > 
> > Let's start out simple.  What's wrong with doing
> > 
> > 	log buf len = max(__LOG_BUF_LEN, nr_possible_cpus * per-cpu log buf len)
> 
> Sure, you already took in the patch series though so how would you like to
> handle a respin, you just drop the last patch and we respin it?

A fresh patch would suit.  That's if you think it is a reasonable 
approach - you've thought about this stuff more than I have!

Re: [PATCH v8 4/4] printk: allow increasing the ring buffer depending on the number of CPUs

[Luis R. Rodriguez]

On Thu, Jun 26, 2014 at 4:20 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 27 Jun 2014 01:16:30 +0200 "Luis R. Rodriguez" <mcgrof@suse.com> wrote:
>
>> > > Another note --  since this option depends on SMP and !BASE_SMALL technically
>> > > num_possible_cpus() won't ever return something smaller than or equal to 1
>> > > but because of the default values chosen the -1 on the compuation does affect
>> > > whether or not this will trigger on > 64 CPUs or >= 64 CPUs, keeping the
>> > > -1 means we require > 64 CPUs.
>> >
>> > hm, that sounds like more complexity.
>> >
>> > > This all can be changed however we like but the language and explained logic
>> > > would just need to be changed.
>> >
>> > Let's start out simple.  What's wrong with doing
>> >
>> >     log buf len = max(__LOG_BUF_LEN, nr_possible_cpus * per-cpu log buf len)
>>
>> Sure, you already took in the patch series though so how would you like to
>> handle a respin, you just drop the last patch and we respin it?
>
> A fresh patch would suit.  That's if you think it is a reasonable
> approach - you've thought about this stuff more than I have!

The way its implemented now makes more technical sense, in short it
assumes the first boot (and CPU) gets the full default kernel ring
buffer size, the extra size is for the gibberish that each extra CPU
is expected to spew out in the worst case. What you propose makes the
explanation simpler and easier to understand but sends the wrong
message about exactly how the growth of the kernel ring buffer is
expected scale with the addition of more CPUs.

  Luis

Re: [PATCH v8 4/4] printk: allow increasing the ring buffer depending on the number of CPUs

[Andrew Morton]

On Thu, 26 Jun 2014 16:32:15 -0700 "Luis R. Rodriguez" <mcgrof@suse.com> wrote:

> On Thu, Jun 26, 2014 at 4:20 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Fri, 27 Jun 2014 01:16:30 +0200 "Luis R. Rodriguez" <mcgrof@suse.com> wrote:
> >
> >> > > Another note --  since this option depends on SMP and !BASE_SMALL technically
> >> > > num_possible_cpus() won't ever return something smaller than or equal to 1
> >> > > but because of the default values chosen the -1 on the compuation does affect
> >> > > whether or not this will trigger on > 64 CPUs or >= 64 CPUs, keeping the
> >> > > -1 means we require > 64 CPUs.
> >> >
> >> > hm, that sounds like more complexity.
> >> >
> >> > > This all can be changed however we like but the language and explained logic
> >> > > would just need to be changed.
> >> >
> >> > Let's start out simple.  What's wrong with doing
> >> >
> >> >     log buf len = max(__LOG_BUF_LEN, nr_possible_cpus * per-cpu log buf len)
> >>
> >> Sure, you already took in the patch series though so how would you like to
> >> handle a respin, you just drop the last patch and we respin it?
> >
> > A fresh patch would suit.  That's if you think it is a reasonable
> > approach - you've thought about this stuff more than I have!
> 
> The way its implemented now makes more technical sense, in short it
> assumes the first boot (and CPU) gets the full default kernel ring
> buffer size, the extra size is for the gibberish that each extra CPU
> is expected to spew out in the worst case. What you propose makes the
> explanation simpler and easier to understand but sends the wrong
> message about exactly how the growth of the kernel ring buffer is
> expected scale with the addition of more CPUs.

OK, it's finally starting to sink in.  The model for the kernel-wide
printk output is "a great pile of CPU-independent stuff plus a certain
amount of per-cpu stuff".  And the code at present attempts to follow
that model.  Yes?

I'm rather internet-challenged at present - please let me take another look at
the patch on Monday.

Re: [PATCH v8 4/4] printk: allow increasing the ring buffer depending on the number of CPUs

[Luis R. Rodriguez]

On Fri, Jun 27, 2014 at 04:59:14PM -0700, Andrew Morton wrote:
> On Thu, 26 Jun 2014 16:32:15 -0700 "Luis R. Rodriguez" <mcgrof@suse.com> wrote:
> 
> > On Thu, Jun 26, 2014 at 4:20 PM, Andrew Morton
> > <akpm@linux-foundation.org> wrote:
> > > On Fri, 27 Jun 2014 01:16:30 +0200 "Luis R. Rodriguez" <mcgrof@suse.com> wrote:
> > >
> > >> > > Another note --  since this option depends on SMP and !BASE_SMALL technically
> > >> > > num_possible_cpus() won't ever return something smaller than or equal to 1
> > >> > > but because of the default values chosen the -1 on the compuation does affect
> > >> > > whether or not this will trigger on > 64 CPUs or >= 64 CPUs, keeping the
> > >> > > -1 means we require > 64 CPUs.
> > >> >
> > >> > hm, that sounds like more complexity.
> > >> >
> > >> > > This all can be changed however we like but the language and explained logic
> > >> > > would just need to be changed.
> > >> >
> > >> > Let's start out simple.  What's wrong with doing
> > >> >
> > >> >     log buf len = max(__LOG_BUF_LEN, nr_possible_cpus * per-cpu log buf len)
> > >>
> > >> Sure, you already took in the patch series though so how would you like to
> > >> handle a respin, you just drop the last patch and we respin it?
> > >
> > > A fresh patch would suit.  That's if you think it is a reasonable
> > > approach - you've thought about this stuff more than I have!
> > 
> > The way its implemented now makes more technical sense, in short it
> > assumes the first boot (and CPU) gets the full default kernel ring
> > buffer size, the extra size is for the gibberish that each extra CPU
> > is expected to spew out in the worst case. What you propose makes the
> > explanation simpler and easier to understand but sends the wrong
> > message about exactly how the growth of the kernel ring buffer is
> > expected scale with the addition of more CPUs.
> 
> OK, it's finally starting to sink in.  The model for the kernel-wide
> printk output is "a great pile of CPU-independent stuff plus a certain
> amount of per-cpu stuff".  And the code at present attempts to follow
> that model.  Yes?

Yup, exactly.

> I'm rather internet-challenged at present - please let me take another look at
> the patch on Monday.

OK!

  Luis