Re: [PATCH] xen/console: do not drop serial output from the hardware domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.06.2022 13:31, Roger Pau Monné wrote:
> On Tue, Jun 14, 2022 at 11:45:54AM +0200, Jan Beulich wrote:
>> On 14.06.2022 11:38, Roger Pau Monné wrote:
>>> On Tue, Jun 14, 2022 at 11:13:07AM +0200, Jan Beulich wrote:
>>>> On 14.06.2022 10:32, Roger Pau Monné wrote:
>>>>> On Tue, Jun 14, 2022 at 10:10:03AM +0200, Jan Beulich wrote:
>>>>>> On 14.06.2022 08:52, Roger Pau Monné wrote:
>>>>>>> On Mon, Jun 13, 2022 at 03:56:54PM +0200, Jan Beulich wrote:
>>>>>>>> On 13.06.2022 14:32, Roger Pau Monné wrote:
>>>>>>>>> On Mon, Jun 13, 2022 at 11:18:49AM +0200, Jan Beulich wrote:
>>>>>>>>>> On 13.06.2022 11:04, Roger Pau Monné wrote:
>>>>>>>>>>> On Mon, Jun 13, 2022 at 10:29:43AM +0200, Jan Beulich wrote:
>>>>>>>>>>>> On 13.06.2022 10:21, Roger Pau Monné wrote:
>>>>>>>>>>>>> On Mon, Jun 13, 2022 at 09:30:06AM +0200, Jan Beulich wrote:
>>>>>>>>>>>>>> On 10.06.2022 17:06, Roger Pau Monne wrote:
>>>>>>>>>>>>>>> Prevent dropping console output from the hardware domain, since 
>>>>>>>>>>>>>>> it's
>>>>>>>>>>>>>>> likely important to have all the output if the boot fails 
>>>>>>>>>>>>>>> without
>>>>>>>>>>>>>>> having to resort to sync_console (which also affects the output 
>>>>>>>>>>>>>>> from
>>>>>>>>>>>>>>> other guests).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Do so by pairing the console_serial_puts() with
>>>>>>>>>>>>>>> serial_{start,end}_log_everything(), so that no output is 
>>>>>>>>>>>>>>> dropped.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> While I can see the goal, why would Dom0 output be (effectively) 
>>>>>>>>>>>>>> more
>>>>>>>>>>>>>> important than Xen's own one (which isn't "forced")? And with 
>>>>>>>>>>>>>> this
>>>>>>>>>>>>>> aiming at boot output only, wouldn't you want to stop the 
>>>>>>>>>>>>>> overriding
>>>>>>>>>>>>>> once boot has completed (of which, if I'm not mistaken, we don't
>>>>>>>>>>>>>> really have any signal coming from Dom0)? And even during boot 
>>>>>>>>>>>>>> I'm
>>>>>>>>>>>>>> not convinced we'd want to let through everything, but perhaps 
>>>>>>>>>>>>>> just
>>>>>>>>>>>>>> Dom0's kernel messages?
>>>>>>>>>>>>>
>>>>>>>>>>>>> I normally use sync_console on all the boxes I'm doing dev work, 
>>>>>>>>>>>>> so
>>>>>>>>>>>>> this request is something that come up internally.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Didn't realize Xen output wasn't forced, since we already have 
>>>>>>>>>>>>> rate
>>>>>>>>>>>>> limiting based on log levels I was assuming that non-ratelimited
>>>>>>>>>>>>> messages wouldn't be dropped.  But yes, I agree that Xen 
>>>>>>>>>>>>> (non-guest
>>>>>>>>>>>>> triggered) output shouldn't be rate limited either.
>>>>>>>>>>>>
>>>>>>>>>>>> Which would raise the question of why we have log levels for 
>>>>>>>>>>>> non-guest
>>>>>>>>>>>> messages.
>>>>>>>>>>>
>>>>>>>>>>> Hm, maybe I'm confused, but I don't see a direct relation between 
>>>>>>>>>>> log
>>>>>>>>>>> levels and rate limiting.  If I set log level to WARNING I would
>>>>>>>>>>> expect to not loose _any_ non-guest log messages with level WARNING 
>>>>>>>>>>> or
>>>>>>>>>>> above.  It's still useful to have log levels for non-guest messages,
>>>>>>>>>>> since user might want to filter out DEBUG non-guest messages for
>>>>>>>>>>> example.
>>>>>>>>>>
>>>>>>>>>> It was me who was confused, because of the two log-everything 
>>>>>>>>>> variants
>>>>>>>>>> we have (console and serial). You're right that your change is 
>>>>>>>>>> unrelated
>>>>>>>>>> to log levels. However, when there are e.g. many warnings or when an
>>>>>>>>>> admin has lowered the log level, what you (would) do is effectively
>>>>>>>>>> force sync_console mode transiently (for a subset of messages, but
>>>>>>>>>> that's secondary, especially because the "forced" output would still
>>>>>>>>>> be waiting for earlier output to make it out).
>>>>>>>>>
>>>>>>>>> Right, it would have to wait for any previous output on the buffer to
>>>>>>>>> go out first.  In any case we can guarantee that no more output will
>>>>>>>>> be added to the buffer while Xen waits for it to be flushed.
>>>>>>>>>
>>>>>>>>> So for the hardware domain it might make sense to wait for the TX
>>>>>>>>> buffers to be half empty (the current tx_quench logic) by preempting
>>>>>>>>> the hypercall.  That however could cause issues if guests manage to
>>>>>>>>> keep filling the buffer while the hardware domain is being preempted.
>>>>>>>>>
>>>>>>>>> Alternatively we could always reserve half of the buffer for the
>>>>>>>>> hardware domain, and allow it to be preempted while waiting for space
>>>>>>>>> (since it's guaranteed non hardware domains won't be able to steal the
>>>>>>>>> allocation from the hardware domain).
>>>>>>>>
>>>>>>>> Getting complicated it seems. I have to admit that I wonder whether we
>>>>>>>> wouldn't be better off leaving the current logic as is.
>>>>>>>
>>>>>>> Another possible solution (more like a band aid) is to increase the
>>>>>>> buffer size from 4 pages to 8 or 16.  That would likely allow to cope
>>>>>>> fine with the high throughput of boot messages.
>>>>>>
>>>>>> You mean the buffer whose size is controlled by serial_tx_buffer?
>>>>>
>>>>> Yes.
>>>>>
>>>>>> On
>>>>>> large systems one may want to simply make use of the command line
>>>>>> option then; I don't think the built-in default needs changing. Or
>>>>>> if so, then perhaps not statically at build time, but taking into
>>>>>> account system properties (like CPU count).
>>>>>
>>>>> So how about we use:
>>>>>
>>>>> min(16384, ROUNDUP(1024 * num_possible_cpus(), 4096))
>>>>
>>>> That would _reduce_ size on small systems, wouldn't it? Originally
>>>> you were after increasing the default size. But if you had meant
>>>> max(), then I'd fear on very large systems this may grow a little
>>>> too large.
>>>
>>> See previous followup about my mistake of using min() instead of
>>> max().
>>>
>>> On a system with 512 CPUs that would be 512KB, I don't think that's a
>>> lot of memory, specially taking into account that a system with 512
>>> CPUs should have a matching amount of memory I would expect.
>>>
>>> It's true however that I very much doubt we would fill a 512K buffer,
>>> so limiting to 64K might be a sensible starting point?
>>
>> Yeah, 64k could be a value to compromise on. What total size of
>> output have you observed to trigger the making of this patch? Xen
>> alone doesn't even manage to fill 16k on most of my systems ...
> 
> I've tried on one of the affected systems now, it's a 8 CPU Kaby Lake
> at 3,5GHz, and manages to fill the buffer while booting Linux.
> 
> My proposed formula won't fix this use case, so what about just
> bumping the buffer to 32K by default, which does fix it?

As said, suitably explained I could also agree with going to 64k. The
question though is in how far 32k, 64k, or ...

> Or alternatively use the proposed formula, but adjust the buffer to be
> between [32K,64K].

... this formula would cover a wide range of contemporary systems.
Without such I can't really see what good a bump would do, as then
many people may still find themselves in need of using the command
line option to put in place a larger buffer.

Jan