[PATCH for-xen-4.5] console: increase initial conring size

[PATCH for-xen-4.5] console: increase initial conring size

[Daniel Kiper]

In general initial conring size is sufficient. However, if log
level is increased on platforms which have e.g. huge number
of memory regions (I have an IBM System x3550 M2 with 8 GiB RAM
which has more than 200 entries in EFI memory map) then some
of earlier messages in console ring are overwritten. It means
that in case of issues deeper analysis can be hindered. Sadly
conring_size argument does not help because new console buffer
is allocated late on heap. It means that it is not possible to
allocate larger ring earlier. So, in this situation initial
conring size should be increased. My experiments showed that
even on not so big machines more than 26 KiB of free space are
needed for initial messages. In theory we could increase conring
size buffer to 32 KiB. However, I think that this value could be
too small for huge machines with large number of ACPI tables and
EFI memory regions. Hence, this patch increases initial conring
size to 64 KiB.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 xen/drivers/char/console.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 2f03259..429d296 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -67,7 +67,7 @@ custom_param("console_timestamps", parse_console_timestamps);
 static uint32_t __initdata opt_conring_size;
 size_param("conring_size", opt_conring_size);
 
-#define _CONRING_SIZE 16384
+#define _CONRING_SIZE 65536
 #define CONRING_IDX_MASK(i) ((i)&(conring_size-1))
 static char __initdata _conring[_CONRING_SIZE];
 static char *__read_mostly conring = _conring;
-- 
1.7.10.4

Re: [PATCH for-xen-4.5] console: increase initial conring size

[Andrew Cooper]

On 02/12/14 14:57, Daniel Kiper wrote:
> In general initial conring size is sufficient. However, if log
> level is increased on platforms which have e.g. huge number
> of memory regions (I have an IBM System x3550 M2 with 8 GiB RAM
> which has more than 200 entries in EFI memory map) then some
> of earlier messages in console ring are overwritten. It means
> that in case of issues deeper analysis can be hindered. Sadly
> conring_size argument does not help because new console buffer
> is allocated late on heap. It means that it is not possible to
> allocate larger ring earlier. So, in this situation initial
> conring size should be increased. My experiments showed that
> even on not so big machines more than 26 KiB of free space are
> needed for initial messages. In theory we could increase conring
> size buffer to 32 KiB. However, I think that this value could be
> too small for huge machines with large number of ACPI tables and
> EFI memory regions. Hence, this patch increases initial conring
> size to 64 KiB.
>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

For what it is worth, XenServer has been been using a 64k default
console size for a very long time now.

However, a change line this must include a change to
docs/misc/xen-command-line.markdown

~Andrew

> ---
>  xen/drivers/char/console.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 2f03259..429d296 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -67,7 +67,7 @@ custom_param("console_timestamps", parse_console_timestamps);
>  static uint32_t __initdata opt_conring_size;
>  size_param("conring_size", opt_conring_size);
>  
> -#define _CONRING_SIZE 16384
> +#define _CONRING_SIZE 65536
>  #define CONRING_IDX_MASK(i) ((i)&(conring_size-1))
>  static char __initdata _conring[_CONRING_SIZE];
>  static char *__read_mostly conring = _conring;

Re: [PATCH for-xen-4.5] console: increase initial conring size

[Olaf Hering]

On Tue, Dec 02, Daniel Kiper wrote:

> Hence, this patch increases initial conring size to 64 KiB.

Instead of increasing the default it was suggested to dynamically
allocate the buffer very early.

http://lists.xenproject.org/archives/html/xen-devel/2011-07/msg00689.html

Olaf

Re: [PATCH for-xen-4.5] console: increase initial conring size

[Jan Beulich]

>>> Daniel Kiper <daniel.kiper@oracle.com> 12/02/14 3:58 PM >>>
>In general initial conring size is sufficient. However, if log
>level is increased on platforms which have e.g. huge number
>of memory regions (I have an IBM System x3550 M2 with 8 GiB RAM
>which has more than 200 entries in EFI memory map) then some
>of earlier messages in console ring are overwritten. It means
>that in case of issues deeper analysis can be hindered. Sadly
>conring_size argument does not help because new console buffer
>is allocated late on heap. It means that it is not possible to
>allocate larger ring earlier. So, in this situation initial
>conring size should be increased. My experiments showed that
>even on not so big machines more than 26 KiB of free space are
>needed for initial messages. In theory we could increase conring
>size buffer to 32 KiB. However, I think that this value could be
>too small for huge machines with large number of ACPI tables and
>EFI memory regions. Hence, this patch increases initial conring
>size to 64 KiB.
>
>Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

I think it was made clear before that just saying "for-xen-4.5" without any
further rationale is insufficient. Please explain what makes this so important
a change that it needs to go in now.

Apart from that, as Olaf validly replied, setting up a dynamically allocated
buffer earlier would seem like a much better course of action.

Jan

Re: [PATCH for-xen-4.5] console: increase initial conring size

[Daniel Kiper]

On Tue, Dec 02, 2014 at 03:58:53PM +0000, Jan Beulich wrote:
> >>> Daniel Kiper <daniel.kiper@oracle.com> 12/02/14 3:58 PM >>>
> >In general initial conring size is sufficient. However, if log
> >level is increased on platforms which have e.g. huge number
> >of memory regions (I have an IBM System x3550 M2 with 8 GiB RAM
> >which has more than 200 entries in EFI memory map) then some
> >of earlier messages in console ring are overwritten. It means
> >that in case of issues deeper analysis can be hindered. Sadly
> >conring_size argument does not help because new console buffer
> >is allocated late on heap. It means that it is not possible to
> >allocate larger ring earlier. So, in this situation initial
> >conring size should be increased. My experiments showed that
> >even on not so big machines more than 26 KiB of free space are
> >needed for initial messages. In theory we could increase conring
> >size buffer to 32 KiB. However, I think that this value could be
> >too small for huge machines with large number of ACPI tables and
> >EFI memory regions. Hence, this patch increases initial conring
> >size to 64 KiB.
> >
> >Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
>
> I think it was made clear before that just saying "for-xen-4.5" without any
> further rationale is insufficient. Please explain what makes this so important
> a change that it needs to go in now.

What is not clear in commit message? It describes a bug, how to fix it
and why in that way. Do you need anything else?

> Apart from that, as Olaf validly replied, setting up a dynamically allocated
> buffer earlier would seem like a much better course of action.

I though about that before posting this patch (I did not know beforehand about
Olaf's work made in 2011). However, I stated that it is too late to make so
intrusive changes. I think we should (sadly) apply this "band-aid" right
now because, as you can see, this bug hits more and more people. On the
other hand I agree that we should finally fix this issue in better way.
Even I am able to add this thing to my TODO list but it is quite long
so I do not know when it will happen.

Daniel

Re: [PATCH for-xen-4.5] console: increase initial conring size

[Jan Beulich]

>>> On 03.12.14 at 16:37, <daniel.kiper@oracle.com> wrote:
> On Tue, Dec 02, 2014 at 03:58:53PM +0000, Jan Beulich wrote:
>> >>> Daniel Kiper <daniel.kiper@oracle.com> 12/02/14 3:58 PM >>>
>> >In general initial conring size is sufficient. However, if log
>> >level is increased on platforms which have e.g. huge number
>> >of memory regions (I have an IBM System x3550 M2 with 8 GiB RAM
>> >which has more than 200 entries in EFI memory map) then some
>> >of earlier messages in console ring are overwritten. It means
>> >that in case of issues deeper analysis can be hindered. Sadly
>> >conring_size argument does not help because new console buffer
>> >is allocated late on heap. It means that it is not possible to
>> >allocate larger ring earlier. So, in this situation initial
>> >conring size should be increased. My experiments showed that
>> >even on not so big machines more than 26 KiB of free space are
>> >needed for initial messages. In theory we could increase conring
>> >size buffer to 32 KiB. However, I think that this value could be
>> >too small for huge machines with large number of ACPI tables and
>> >EFI memory regions. Hence, this patch increases initial conring
>> >size to 64 KiB.
>> >
>> >Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
>>
>> I think it was made clear before that just saying "for-xen-4.5" without any
>> further rationale is insufficient. Please explain what makes this so 
> important
>> a change that it needs to go in now.
> 
> What is not clear in commit message? It describes a bug, how to fix it
> and why in that way. Do you need anything else?

I didn't say the commit message is unclear. Instead I told you that
after a --- separate I would have expected justification for the
request to include this in 4.5. After all what you call a bug here
is imo merely a lack of functionality.

Jan