[PATCH 0/2] console/serial: adjust default TX buffer size

[PATCH 0/2] console/serial: adjust default TX buffer size

[Roger Pau Monne]

Hello,

First patch moves the setting of the default TX buffer size to Kconfig,
and shouldn't be controversial, second patch increases the buffer to
32K.

Thanks, Roger.

Roger Pau Monne (2):
  console/serial: set the default transmit buffer size in Kconfig
  console/serial: bump buffer from 16K to 32K

 xen/drivers/char/Kconfig  | 10 ++++++++++
 xen/drivers/char/serial.c |  3 ++-
 2 files changed, 12 insertions(+), 1 deletion(-)

-- 
2.36.1

[PATCH 1/2] console/serial: set the default transmit buffer size in Kconfig

[Roger Pau Monne]

Take the opportunity to convert the variable to read-only after init.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/char/Kconfig  | 10 ++++++++++
 xen/drivers/char/serial.c |  3 ++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index e5f7b1d8eb..a349d55f18 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -74,3 +74,13 @@ config HAS_EHCI
 	help
 	  This selects the USB based EHCI debug port to be used as a UART. If
 	  you have an x86 based system with USB, say Y.
+
+config SERIAL_TX_BUFSIZE
+	int "Size of the transmit serial buffer"
+	default 16384
+	help
+	  Controls the default size of the transmit buffer (in bytes) used by
+	  the serial driver.  Note the value provided will be rounder up to
+	  PAGE_SIZE.
+
+	  Default value is 16384 (16KB).
diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c
index 5ecba0af33..8d375a41e3 100644
--- a/xen/drivers/char/serial.c
+++ b/xen/drivers/char/serial.c
@@ -16,7 +16,8 @@
 /* Never drop characters, even if the async transmit buffer fills. */
 /* #define SERIAL_NEVER_DROP_CHARS 1 */
 
-unsigned int __read_mostly serial_txbufsz = 16384;
+unsigned int __ro_after_init serial_txbufsz = ROUNDUP(CONFIG_SERIAL_TX_BUFSIZE,
+                                                      PAGE_SIZE);
 size_param("serial_tx_buffer", serial_txbufsz);
 
 #define mask_serial_rxbuf_idx(_i) ((_i)&(serial_rxbufsz-1))
-- 
2.36.1

[PATCH 2/2] console/serial: bump buffer from 16K to 32K

[Roger Pau Monne]

Testing on a Kaby Lake box with 8 CPUs leads to the serial buffer
being filled halfway during dom0 boot, and thus a non-trivial chunk of
Linux boot messages are dropped.

Increasing the buffer to 32K does fix the issue and Linux boot
messages are no longer dropped.  There's no justification either on
why 16K was chosen, and hence bumping to 32K in order to cope with
current systems generating output faster does seem appropriate to have
a better user experience with the provided defaults.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/char/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index a349d55f18..a8ac667ba2 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -77,10 +77,10 @@ config HAS_EHCI
 
 config SERIAL_TX_BUFSIZE
 	int "Size of the transmit serial buffer"
-	default 16384
+	default 32768
 	help
 	  Controls the default size of the transmit buffer (in bytes) used by
 	  the serial driver.  Note the value provided will be rounder up to
 	  PAGE_SIZE.
 
-	  Default value is 16384 (16KB).
+	  Default value is 32768 (32KB).
-- 
2.36.1

Re: [PATCH 1/2] console/serial: set the default transmit buffer size in Kconfig

[Jan Beulich]

On 23.06.2022 11:08, Roger Pau Monne wrote:
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -74,3 +74,13 @@ config HAS_EHCI
>  	help
>  	  This selects the USB based EHCI debug port to be used as a UART. If
>  	  you have an x86 based system with USB, say Y.
> +
> +config SERIAL_TX_BUFSIZE
> +	int "Size of the transmit serial buffer"
> +	default 16384
> +	help
> +	  Controls the default size of the transmit buffer (in bytes) used by
> +	  the serial driver.  Note the value provided will be rounder up to
> +	  PAGE_SIZE.

I first wanted to point out the spelling mistake (rounded), but I
wonder what good that rounding does and whether this description
isn't really misleading: serial_async_transmit() rounds down to a
power of two. So likely the value here would better be a log-2
one.

> +	  Default value is 16384 (16KB).

Perhaps (16kiB) (albeit the default value would change anyway if
the above is followed)?

Jan

Re: [PATCH 2/2] console/serial: bump buffer from 16K to 32K

[Jan Beulich]

On 23.06.2022 11:08, Roger Pau Monne wrote:
> Testing on a Kaby Lake box with 8 CPUs leads to the serial buffer
> being filled halfway during dom0 boot, and thus a non-trivial chunk of
> Linux boot messages are dropped.
> 
> Increasing the buffer to 32K does fix the issue and Linux boot
> messages are no longer dropped.  There's no justification either on
> why 16K was chosen, and hence bumping to 32K in order to cope with
> current systems generating output faster does seem appropriate to have
> a better user experience with the provided defaults.

Just to record what was part of an earlier discussion: I'm not going
to nak such a change, but I think the justification is insufficient:
On this same basis someone else could come a few days later and bump
to 64k, then 128k, etc.

Jan

Re: [PATCH 2/2] console/serial: bump buffer from 16K to 32K

[Roger Pau Monné]

On Thu, Jun 23, 2022 at 03:32:30PM +0200, Jan Beulich wrote:
> On 23.06.2022 11:08, Roger Pau Monne wrote:
> > Testing on a Kaby Lake box with 8 CPUs leads to the serial buffer
> > being filled halfway during dom0 boot, and thus a non-trivial chunk of
> > Linux boot messages are dropped.
> > 
> > Increasing the buffer to 32K does fix the issue and Linux boot
> > messages are no longer dropped.  There's no justification either on
> > why 16K was chosen, and hence bumping to 32K in order to cope with
> > current systems generating output faster does seem appropriate to have
> > a better user experience with the provided defaults.
> 
> Just to record what was part of an earlier discussion: I'm not going
> to nak such a change, but I think the justification is insufficient:
> On this same basis someone else could come a few days later and bump
> to 64k, then 128k, etc.

Indeed, and that would be fine IMO.  We should aim to provide defaults
that work fine for most situations, and here I don't see what drawback
it has to increase the default buffer size from 16kiB to 32kiB, and
I would be fine with increasing to 128kiB if that's required for some
use case, albeit I have a hard time seeing how we could fill that
buffer.

If I can ask, what kind of justification you would see fit for
granting an increase to the default buffer size?

Thanks, Roger.

Re: [PATCH 2/2] console/serial: bump buffer from 16K to 32K

[Jan Beulich]

On 29.06.2022 17:23, Roger Pau Monné wrote:
> On Thu, Jun 23, 2022 at 03:32:30PM +0200, Jan Beulich wrote:
>> On 23.06.2022 11:08, Roger Pau Monne wrote:
>>> Testing on a Kaby Lake box with 8 CPUs leads to the serial buffer
>>> being filled halfway during dom0 boot, and thus a non-trivial chunk of
>>> Linux boot messages are dropped.
>>>
>>> Increasing the buffer to 32K does fix the issue and Linux boot
>>> messages are no longer dropped.  There's no justification either on
>>> why 16K was chosen, and hence bumping to 32K in order to cope with
>>> current systems generating output faster does seem appropriate to have
>>> a better user experience with the provided defaults.
>>
>> Just to record what was part of an earlier discussion: I'm not going
>> to nak such a change, but I think the justification is insufficient:
>> On this same basis someone else could come a few days later and bump
>> to 64k, then 128k, etc.
> 
> Indeed, and that would be fine IMO.  We should aim to provide defaults
> that work fine for most situations, and here I don't see what drawback
> it has to increase the default buffer size from 16kiB to 32kiB, and
> I would be fine with increasing to 128kiB if that's required for some
> use case, albeit I have a hard time seeing how we could fill that
> buffer.
> 
> If I can ask, what kind of justification you would see fit for
> granting an increase to the default buffer size?

Making plausible that for a majority of contemporary systems the buffer
is not large enough would be one aspect. But then there's imo always
going to be an issue: What if non-Linux Dom0 would be far more chatty?
What if Linux, down the road, was made less verbose (by default)? What
if people expect large enough a buffer to also suffice when Linux runs
in e.g. ignore_loglevel mode? We simply can't fit all use cases and at
the same time also not go overboard with the default size. That's why
there's a way to control this via command line option.

Jan

Re: [PATCH 2/2] console/serial: bump buffer from 16K to 32K

[Roger Pau Monné]

On Wed, Jun 29, 2022 at 06:03:34PM +0200, Jan Beulich wrote:
> On 29.06.2022 17:23, Roger Pau Monné wrote:
> > On Thu, Jun 23, 2022 at 03:32:30PM +0200, Jan Beulich wrote:
> >> On 23.06.2022 11:08, Roger Pau Monne wrote:
> >>> Testing on a Kaby Lake box with 8 CPUs leads to the serial buffer
> >>> being filled halfway during dom0 boot, and thus a non-trivial chunk of
> >>> Linux boot messages are dropped.
> >>>
> >>> Increasing the buffer to 32K does fix the issue and Linux boot
> >>> messages are no longer dropped.  There's no justification either on
> >>> why 16K was chosen, and hence bumping to 32K in order to cope with
> >>> current systems generating output faster does seem appropriate to have
> >>> a better user experience with the provided defaults.
> >>
> >> Just to record what was part of an earlier discussion: I'm not going
> >> to nak such a change, but I think the justification is insufficient:
> >> On this same basis someone else could come a few days later and bump
> >> to 64k, then 128k, etc.
> > 
> > Indeed, and that would be fine IMO.  We should aim to provide defaults
> > that work fine for most situations, and here I don't see what drawback
> > it has to increase the default buffer size from 16kiB to 32kiB, and
> > I would be fine with increasing to 128kiB if that's required for some
> > use case, albeit I have a hard time seeing how we could fill that
> > buffer.
> > 
> > If I can ask, what kind of justification you would see fit for
> > granting an increase to the default buffer size?
> 
> Making plausible that for a majority of contemporary systems the buffer
> is not large enough would be one aspect. But then there's imo always
> going to be an issue: What if non-Linux Dom0 would be far more chatty?
> What if Linux, down the road, was made less verbose (by default)? What
> if people expect large enough a buffer to also suffice when Linux runs
> in e.g. ignore_loglevel mode? We simply can't fit all use cases and at
> the same time also not go overboard with the default size. That's why
> there's a way to control this via command line option.

Maybe I should clarify that the current buffer size is not enough on
this system with the default Linux log level. I think we can expect
someone that changes the default Linux log level to also consider
changing the Xen buffer size.  OTOH when using the default Linux log
level the default Xen serial buffer should be enough.

I haven't tested with FreeBSD on that system, other systems I have
seem to be fine when booting FreeBSD with the default Xen serial
buffer size.

I think we should exercise caution if someone proposed to increase to
1M for example, but I don't see why so much controversy for going from
16K to 32K, it's IMO a reasonable value and has proven to prevent
serial log loss when using the default Linux log level.

Thanks, Roger.

Re: [PATCH 2/2] console/serial: bump buffer from 16K to 32K

[Jan Beulich]

On 29.06.2022 18:19, Roger Pau Monné wrote:
> On Wed, Jun 29, 2022 at 06:03:34PM +0200, Jan Beulich wrote:
>> On 29.06.2022 17:23, Roger Pau Monné wrote:
>>> On Thu, Jun 23, 2022 at 03:32:30PM +0200, Jan Beulich wrote:
>>>> On 23.06.2022 11:08, Roger Pau Monne wrote:
>>>>> Testing on a Kaby Lake box with 8 CPUs leads to the serial buffer
>>>>> being filled halfway during dom0 boot, and thus a non-trivial chunk of
>>>>> Linux boot messages are dropped.
>>>>>
>>>>> Increasing the buffer to 32K does fix the issue and Linux boot
>>>>> messages are no longer dropped.  There's no justification either on
>>>>> why 16K was chosen, and hence bumping to 32K in order to cope with
>>>>> current systems generating output faster does seem appropriate to have
>>>>> a better user experience with the provided defaults.
>>>>
>>>> Just to record what was part of an earlier discussion: I'm not going
>>>> to nak such a change, but I think the justification is insufficient:
>>>> On this same basis someone else could come a few days later and bump
>>>> to 64k, then 128k, etc.
>>>
>>> Indeed, and that would be fine IMO.  We should aim to provide defaults
>>> that work fine for most situations, and here I don't see what drawback
>>> it has to increase the default buffer size from 16kiB to 32kiB, and
>>> I would be fine with increasing to 128kiB if that's required for some
>>> use case, albeit I have a hard time seeing how we could fill that
>>> buffer.
>>>
>>> If I can ask, what kind of justification you would see fit for
>>> granting an increase to the default buffer size?
>>
>> Making plausible that for a majority of contemporary systems the buffer
>> is not large enough would be one aspect. But then there's imo always
>> going to be an issue: What if non-Linux Dom0 would be far more chatty?
>> What if Linux, down the road, was made less verbose (by default)? What
>> if people expect large enough a buffer to also suffice when Linux runs
>> in e.g. ignore_loglevel mode? We simply can't fit all use cases and at
>> the same time also not go overboard with the default size. That's why
>> there's a way to control this via command line option.
> 
> Maybe I should clarify that the current buffer size is not enough on
> this system with the default Linux log level. I think we can expect
> someone that changes the default Linux log level to also consider
> changing the Xen buffer size.  OTOH when using the default Linux log
> level the default Xen serial buffer should be enough.
> 
> I haven't tested with FreeBSD on that system, other systems I have
> seem to be fine when booting FreeBSD with the default Xen serial
> buffer size.
> 
> I think we should exercise caution if someone proposed to increase to
> 1M for example, but I don't see why so much controversy for going from
> 16K to 32K, it's IMO a reasonable value and has proven to prevent
> serial log loss when using the default Linux log level.

But see - that's exactly my point. Where do you draw the line between
"we should accept" and "exercise caution"? Is it 256k? Or 512k?

Certainly I'm also aware of the common "memory is cheap" attitude,
but that doesn't make me like it. That's both because of having
started with 64k total (or maybe even less; too long ago meanwhile),
and because of my general observation that everything only ever
(fast) growing makes many things slower than they would need to be.

As to controversy - I did make clear before that I don't mean to nak
the change. But given my views, you'll need to find someone else to
ack it despite being aware of my opinion.

Jan

Re: [PATCH 2/2] console/serial: bump buffer from 16K to 32K

[Roger Pau Monné]

On Wed, Jun 29, 2022 at 06:30:18PM +0200, Jan Beulich wrote:
> On 29.06.2022 18:19, Roger Pau Monné wrote:
> > On Wed, Jun 29, 2022 at 06:03:34PM +0200, Jan Beulich wrote:
> >> On 29.06.2022 17:23, Roger Pau Monné wrote:
> >>> On Thu, Jun 23, 2022 at 03:32:30PM +0200, Jan Beulich wrote:
> >>>> On 23.06.2022 11:08, Roger Pau Monne wrote:
> >>>>> Testing on a Kaby Lake box with 8 CPUs leads to the serial buffer
> >>>>> being filled halfway during dom0 boot, and thus a non-trivial chunk of
> >>>>> Linux boot messages are dropped.
> >>>>>
> >>>>> Increasing the buffer to 32K does fix the issue and Linux boot
> >>>>> messages are no longer dropped.  There's no justification either on
> >>>>> why 16K was chosen, and hence bumping to 32K in order to cope with
> >>>>> current systems generating output faster does seem appropriate to have
> >>>>> a better user experience with the provided defaults.
> >>>>
> >>>> Just to record what was part of an earlier discussion: I'm not going
> >>>> to nak such a change, but I think the justification is insufficient:
> >>>> On this same basis someone else could come a few days later and bump
> >>>> to 64k, then 128k, etc.
> >>>
> >>> Indeed, and that would be fine IMO.  We should aim to provide defaults
> >>> that work fine for most situations, and here I don't see what drawback
> >>> it has to increase the default buffer size from 16kiB to 32kiB, and
> >>> I would be fine with increasing to 128kiB if that's required for some
> >>> use case, albeit I have a hard time seeing how we could fill that
> >>> buffer.
> >>>
> >>> If I can ask, what kind of justification you would see fit for
> >>> granting an increase to the default buffer size?
> >>
> >> Making plausible that for a majority of contemporary systems the buffer
> >> is not large enough would be one aspect. But then there's imo always
> >> going to be an issue: What if non-Linux Dom0 would be far more chatty?
> >> What if Linux, down the road, was made less verbose (by default)? What
> >> if people expect large enough a buffer to also suffice when Linux runs
> >> in e.g. ignore_loglevel mode? We simply can't fit all use cases and at
> >> the same time also not go overboard with the default size. That's why
> >> there's a way to control this via command line option.
> > 
> > Maybe I should clarify that the current buffer size is not enough on
> > this system with the default Linux log level. I think we can expect
> > someone that changes the default Linux log level to also consider
> > changing the Xen buffer size.  OTOH when using the default Linux log
> > level the default Xen serial buffer should be enough.
> > 
> > I haven't tested with FreeBSD on that system, other systems I have
> > seem to be fine when booting FreeBSD with the default Xen serial
> > buffer size.
> > 
> > I think we should exercise caution if someone proposed to increase to
> > 1M for example, but I don't see why so much controversy for going from
> > 16K to 32K, it's IMO a reasonable value and has proven to prevent
> > serial log loss when using the default Linux log level.
> 
> But see - that's exactly my point. Where do you draw the line between
> "we should accept" and "exercise caution"? Is it 256k? Or 512k?

Hard to tell, it would depend on the justification/use case for needed
those buffer sizes.

To be fair 16K seems equally random to me, I tried to backtrack to the
commit it was introduced, but I haven't been able to find any
justification.

I think we can at least agree that making the buffer size configurable
from Kconfig is a desirable move.

Thanks, Roger.