[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 24/25] xen/vpl011: buffer out chars when the backend is xen



Hi,

On 16/08/18 20:41, Stefano Stabellini wrote:
On Mon, 13 Aug 2018, Julien Grall wrote:
Hi,

On 01/08/18 00:28, Stefano Stabellini wrote:
To avoid mixing the output of different domains on the console, buffer
the output chars and print line by line. Unless the domain has input
from the serial, in which case we want to print char by char for a
smooth user experience.

The size of SBSA_UART_OUT_BUF_SIZE is arbitrary. 90 should be large
enough to accommodate the length of most lines of output (typically they
are limited to 80 characters on Unix systems), plus one extra char for
the string terminator.

How about using the same value as vuart (e.g VUART_BUT_SIZE) instead? So we
buffer the same way for the vpl011?

Yes, I can do that.


Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
---
   xen/arch/arm/vpl011.c        | 21 ++++++++++++++++++---
   xen/include/asm-arm/vpl011.h |  3 +++
   2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index f206c61..8137371 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -28,6 +28,7 @@
   #include <xen/lib.h>
   #include <xen/mm.h>
   #include <xen/sched.h>
+#include <xen/console.h>
   #include <public/domctl.h>
   #include <public/io/console.h>
   #include <asm/pl011-uart.h>
@@ -85,12 +86,26 @@ static void vpl011_write_data_xen(struct domain *d,
uint8_t data)
   {
       unsigned long flags;
       struct vpl011 *vpl011 = &d->arch.vpl011;
+    struct vpl011_xen_backend *intf = vpl011->xen;
+    struct domain *input = console_input_domain();
         VPL011_LOCK(d, flags);
   -    printk("%c", data);
-    if (data == '\n')
-        printk("DOM%u: ", d->domain_id);
+    intf->out[intf->out_prod++] = data;
+    if ( d == input && intf->out_prod == 1 )
+    {
+        printk("%c", data);
+        if ( data == '\n' )
+            printk("DOM%u: ", d->domain_id);
+        intf->out_prod = 0;

See my remark on the patch implementing vpl011_write_data_xen.

With this patch, the missing "DOM" at the beginning cannot happen
anymore due to the out buffering. Theoretically it can still happen by
switching to DOM1 before DOM1 prints anything, but it is impossible to
do in practice.

How come this is impossible? DOM1 may have print very late and therefore you have time to switch to DOM1 before any print.

Even in this theoretical scenario, the user would still
get the useful "Switching to DOM1" message, that would help clarify what
is going on. Thus, my preference is to avoid making the code more
complex to fix this issue.

I don't like the idea that in some case DOM%u: is not printed in front of the line. This is making more difficult to read the logs.



+    } else if ( d == input ||

Coding style:

}
else if
{

I'll fix


+                intf->out_prod == SBSA_UART_OUT_BUF_SIZE - 1 ||
+                data == '\n' )
+    {
+        intf->out[intf->out_prod++] = '\0';
+        printk("DOM%u: %s", d->domain_id, intf->out);
+        intf->out_prod = 0;
+    }

The code is quite difficult to read. It would be easier to differentiate
(domain == input vs domain != input) even if it means a bit of duplication.

OK, I can rearrange the code that way. For example:

     if ( d == input ){
         if ( intf->out_prod == 1 )
         {
             printk("%c", data);
             if ( data == '\n' )
                 printk("DOM%u: ", d->domain_id);
             intf->out_prod = 0;
         }
         else
         {
             if ( data != '\n' )
                 intf->out[intf->out_prod++] = '\n';
             intf->out[intf->out_prod++] = '\0';
             printk("DOM%u: %s", d->domain_id, intf->out);
             intf->out_prod = 0;
         }
     }
     else
     {
         if ( intf->out_prod == SBSA_UART_OUT_BUF_SIZE - 2 ||
              data == '\n' )
         {
             if ( data != '\n' )
                 intf->out[intf->out_prod++] = '\n';
             intf->out[intf->out_prod++] = '\0';
             printk("DOM%u: %s", d->domain_id, intf->out);
             intf->out_prod = 0;
         }
     }

Is it better?

Looks easier to read.



You also don't handle all the cases.

For the input domain, I don't think you want to print the domain in front.
Instead I would rely on the "Switch to ...".

Actually it is very convenient to know at any given time which domain
you are talking to. I couldn't find any problems with the prefix, even
using VIM, etc. I would rather keep the "DOM" string around.

Well it is convenient if you manage to put "DOM" string in front. From a look at your implementation and your own comment this is not always the case.

I am also quite surprised that this does not make VIM (or any other editor) more difficult to use as "DOM:" is printed in front of each line.



This would avoid the problem
where DomB needs to have his line printed while it is not the console input.
If this happens in the middle of DomA, then you are loosing track what's going
on.

I don't understand the example: if DOM1 has input, and DOM2 prints
something, the DOM2 output will be prepended by "DOM2", avoiding any
confusion. What am I missing?

Let's take an example:
        1) DOM1 writes "\nab"
        2) DOM2 writes "Foobar\n"
        3) DOM1 write "cde"

The output would be:

DOM1: ab DOM2: Foobar
cde

DOM1 and DOM2 has the line combined (not a big deal). However, the next few characters for DOM1 "cde" will not be prefixed with "DOM1:".

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.