Re: performance-improvement-of-serial-console-via-virtual.patch added to -mm tree

Re: performance-improvement-of-serial-console-via-virtual.patch added to -mm tree

[Taku Izumi]

Dear Russell:

>I don't think we want this.  With early serial console, tx_loadsz is
>not guaranteed to be initialised, and may in fact be zero.

>Plus there's no guarantee that the FIFOs will actually be enabled, so
>I think it's better that this patch doesn't go to mainline.

Our server has a virtual serial port, but its performance seems to be poor.
It takes 10 seconds to output 4000 characters (from kernel) to serial
console. By applying my patch, its peformance could be improved. ( 0.4
seconds / 4000 characters output), so I think it is useful to use FIFO at
serial8250_console_write function like transmit_chars function. Where
should I correct in order to use FIFO?

Taku Izumi <izumi2005@soft.fujitsu.com>

Re: performance-improvement-of-serial-console-via-virtual.patch added to -mm tree

[Russell King]

On Thu, Sep 08, 2005 at 04:47:32PM +0900, Taku Izumi wrote:
> >I don't think we want this.  With early serial console, tx_loadsz is
> >not guaranteed to be initialised, and may in fact be zero.
> 
> >Plus there's no guarantee that the FIFOs will actually be enabled, so
> >I think it's better that this patch doesn't go to mainline.
> 
> Our server has a virtual serial port, but its performance seems to be poor.
> It takes 10 seconds to output 4000 characters (from kernel) to serial
> console. By applying my patch, its peformance could be improved. ( 0.4
> seconds / 4000 characters output), so I think it is useful to use FIFO at
> serial8250_console_write function like transmit_chars function. Where
> should I correct in order to use FIFO?

The problem is that we don't know:

* if there is a FIFO
* what size the FIFO is
* if it has been initialised
* how much data is already contained in the FIFO

So we can't really blindly initialise the FIFO in the console write
method.  Neither can we initialise it in the console setup.  If we
could initialise it, we can't blindly load 16 bytes into the FIFO
at a time.

I don't think it's technically practical to use the FIFO for the
console and still have a reliable serial port.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: performance-improvement-of-serial-console-via-virtual.patch added to -mm tree

[Hironobu Ishii]

Hi Russel,

I am working with Taku,

> On Thu, Sep 08, 2005 at 04:47:32PM +0900, Taku Izumi wrote:
>> >I don't think we want this.  With early serial console, tx_loadsz is
>> >not guaranteed to be initialised, and may in fact be zero.
>> 
>> >Plus there's no guarantee that the FIFOs will actually be enabled, so
>> >I think it's better that this patch doesn't go to mainline.
>> 
>> Our server has a virtual serial port, but its performance seems to be poor.
>> It takes 10 seconds to output 4000 characters (from kernel) to serial
>> console. By applying my patch, its peformance could be improved. ( 0.4
>> seconds / 4000 characters output), so I think it is useful to use FIFO at
>> serial8250_console_write function like transmit_chars function. Where
>> should I correct in order to use FIFO?
> 
> The problem is that we don't know:
> 
> * if there is a FIFO
> * what size the FIFO is

I understand tx_loadsz is practical TX FIFO size. 
If there is no FIFO, tx_loadsz becomes 1.
Is it wrong?
  
 - tx_loadsz is properly initilized in autoconfig().
 - FIFO is enabled in serial8250_clear_fifo() called from autoconfig(),
   if FIFO exist.
 - autoconfig() is called from serial8250_isa_init_ports().
 - serial8250_isa_init_ports() is called from serial8250_console_init() etc.
 
I can't find the problem you are pointing out.

> * if it has been initialised
> * how much data is already contained in the FIFO

Right, we can't know how many byte exist in the FIFO.
So this patch is waiting the FIFO becomes empty at first
by calling "wait_for_xmitr(up)".
(This is the same logic with original.)

After TX FIFO become empty, we can decide the available 
TX FIFO depth by up->tx_loadsize.

>        for (i = 0; i < count; ) {
>                int     fifo;
>
>                wait_for_xmitr(up);
>                fifo = up->tx_loadsz;
>                /*
>                 *      Send the character out using FIFO.
>                 *      If a LF, also do CR...
>                 */
>                do {
>                        serial_out(up, UART_TX, *s);
>                        fifo--;
>                        if (*s == 10) {
>                                if (fifo > 0) {
>                                        serial_out(up, UART_TX, 13);
>                                        fifo--;
>                                } else {
>                                        /* No room to add CR */
>                                        wait_for_xmitr(up);
>                                        fifo = up->tx_loadsz;
>                                        serial_out(up, UART_TX, 13);
>                                        fifo--;
>                                }
>                        }
>                        i++;
>                        s++;
>                } while (fifo > 0 && i < count );
>        }


> 
> So we can't really blindly initialise the FIFO in the console write
> method.  Neither can we initialise it in the console setup.  If we
> could initialise it, we can't blindly load 16 bytes into the FIFO
> at a time.
> 
> I don't think it's technically practical to use the FIFO for the
> console and still have a reliable serial port.
> 
> -- 
> Russell King
> Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
> maintainer of:  2.6 Serial core
> -

Best regards,
Hironobu Ishii

Re: performance-improvement-of-serial-console-via-virtual.patch added to -mm tree

[Russell King]

On Tue, Sep 13, 2005 at 08:44:37PM +0900, Hironobu Ishii wrote:
> Hi Russel,
> 
> I am working with Taku,
> 
> > On Thu, Sep 08, 2005 at 04:47:32PM +0900, Taku Izumi wrote:
> >> >I don't think we want this.  With early serial console, tx_loadsz is
> >> >not guaranteed to be initialised, and may in fact be zero.
> >> 
> >> >Plus there's no guarantee that the FIFOs will actually be enabled, so
> >> >I think it's better that this patch doesn't go to mainline.
> >> 
> >> Our server has a virtual serial port, but its performance seems to be poor.
> >> It takes 10 seconds to output 4000 characters (from kernel) to serial
> >> console. By applying my patch, its peformance could be improved. ( 0.4
> >> seconds / 4000 characters output), so I think it is useful to use FIFO at
> >> serial8250_console_write function like transmit_chars function. Where
> >> should I correct in order to use FIFO?
> > 
> > The problem is that we don't know:
> > 
> > * if there is a FIFO
> > * what size the FIFO is
> 
> I understand tx_loadsz is practical TX FIFO size. 
> If there is no FIFO, tx_loadsz becomes 1.
> Is it wrong?
>   
>  - tx_loadsz is properly initilized in autoconfig().
>  - FIFO is enabled in serial8250_clear_fifo() called from autoconfig(),
>    if FIFO exist.
>  - autoconfig() is called from serial8250_isa_init_ports().
>  - serial8250_isa_init_ports() is called from serial8250_console_init() etc.
>  
> I can't find the problem you are pointing out.

autoconfig() is _not_ called from serial8250_isa_init_ports().

The problem is that the console write method may be called prior to
autoconfig() being run for the port in question, so tx_loadsz may be
uninitialised.

> > * if it has been initialised
> > * how much data is already contained in the FIFO
> 
> Right, we can't know how many byte exist in the FIFO.
> So this patch is waiting the FIFO becomes empty at first
> by calling "wait_for_xmitr(up)".
> (This is the same logic with original.)
> 
> After TX FIFO become empty, we can decide the available 
> TX FIFO depth by up->tx_loadsize.

Only if you ignore the fact that tx_loadsz may not be initialised.

The only things which the console code can rely on being initialised
is the port address description (iobase / membase / iotype / regshift),
the flow control (UPF_CONS_FLOW) in flags, and in the case of Xscale
systems, the capabilities.  Everything else will be in an indeterminent
state as far as the serial console code is concerned, and therefore can
not be relied upon.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: performance-improvement-of-serial-console-via-virtual.patch added to -mm tree

[Russell King]

On Tue, Sep 13, 2005 at 12:53:26PM +0100, Russell King wrote:
> The only things which the console code can rely on being initialised
> is the port address description (iobase / membase / iotype / regshift),
> the flow control (UPF_CONS_FLOW) in flags, and in the case of Xscale
> systems, the capabilities.  Everything else will be in an indeterminent
> state as far as the serial console code is concerned, and therefore can
> not be relied upon.

Additionally, once all architectures convert to initialising their
serial ports via platform devices (which means include/asm-*/serial.h
becomes essentially empty) and we eliminate serial8250_console_init(),
the 8250 console code can start assuming that more of the uart_port
structure will be initialised.

At that point, we can start to think about using FIFOs for the
console.

However, this is an exercise for architecture maintainers to do
who in theory know the requirements for their platforms.  So far
I've seen little progress on this though.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: performance-improvement-of-serial-console-via-virtual.patch added to -mm tree

[Hironobu Ishii]

Hi Russell,

> Additionally, once all architectures convert to initialising their
> serial ports via platform devices (which means include/asm-*/serial.h
> becomes essentially empty) and we eliminate serial8250_console_init(),
> the 8250 console code can start assuming that more of the uart_port
> structure will be initialised.
> 
> At that point, we can start to think about using FIFOs for the
> console.

Thank you for FIFO consideration.

me> Before initialization, does tx_loadsz left 0?
me> If so, we can easily solve the problem:

I confirmed this assumption is OK in current code,
because seiral8250_ports[] is static variable.

We will release revised patch later,
please apply our patch until your serial driver 
re-organization completes.

Thank you.
Hironobu Ishii

Re: performance-improvement-of-serial-console-via-virtual.patch added to -mm tree

[Taku Izumi]

[-- Attachment #1: Type: text/plain, Size: 370 bytes --]

Dear Russel:

I change my patch based on the result of discussion.

The change is as follows:

  - add a member to uart_8250_port structure in order to check if FIFO is
    enable or not.
  - use FIFO at serial8250_console_write function only when FIFO is enable.

I tested my patch on i386 and ia64 architecture.

signed-off-by: Taku Izumi <izumi2005@soft.fujitsu.com>

[-- Attachment #2: serial.patch --]
[-- Type: text/plain, Size: 2779 bytes --]

diff -urNp linux-2.6.14-rc1.org/drivers/serial/8250.c linux-2.6.14-rc1/drivers/serial/8250.c
--- linux-2.6.14-rc1.org/drivers/serial/8250.c	2005-09-13 12:12:09.000000000 +0900
+++ linux-2.6.14-rc1/drivers/serial/8250.c	2005-09-15 09:38:46.000000000 +0900
@@ -128,6 +128,7 @@ struct uart_8250_port {
 	unsigned char		mcr_mask;	/* mask of user bits */
 	unsigned char		mcr_force;	/* mask of forced bits */
 	unsigned char		lsr_break_flag;
+	unsigned char		fcr;
 
 	/*
 	 * We provide a per-port pm hook.
@@ -332,6 +333,7 @@ static unsigned int serial_icr_read(stru
 static inline void serial8250_clear_fifos(struct uart_8250_port *p)
 {
 	if (p->capabilities & UART_CAP_FIFO) {
+		p->fcr = 0;
 		serial_outp(p, UART_FCR, UART_FCR_ENABLE_FIFO);
 		serial_outp(p, UART_FCR, UART_FCR_ENABLE_FIFO |
 			       UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
@@ -1809,8 +1811,10 @@ serial8250_set_termios(struct uart_port 
 	 * LCR DLAB must be set to enable 64-byte FIFO mode. If the FCR
 	 * is written without DLAB set, this mode will be disabled.
 	 */
-	if (up->port.type == PORT_16750)
+	if (up->port.type == PORT_16750) {
 		serial_outp(up, UART_FCR, fcr);
+		up->fcr = fcr;
+	}
 
 	serial_outp(up, UART_LCR, cval);		/* reset DLAB */
 	up->lcr = cval;					/* Save LCR */
@@ -1820,6 +1824,7 @@ serial8250_set_termios(struct uart_port 
 			serial_outp(up, UART_FCR, UART_FCR_ENABLE_FIFO);
 		}
 		serial_outp(up, UART_FCR, fcr);		/* set fcr */
+		up->fcr = fcr;
 	}
 	serial8250_set_mctrl(&up->port, up->port.mctrl);
 	spin_unlock_irqrestore(&up->port.lock, flags);
@@ -2140,6 +2145,7 @@ serial8250_console_write(struct console 
 	struct uart_8250_port *up = &serial8250_ports[co->index];
 	unsigned int ier;
 	int i;
+	int tx_loadsz;
 
 	/*
 	 *	First save the UER then disable the interrupts
@@ -2152,20 +2158,39 @@ serial8250_console_write(struct console 
 		serial_out(up, UART_IER, 0);
 
 	/*
+	 *	Check whether FIFO is enabled
+	 */ 
+	tx_loadsz = (up->fcr & UART_FCR_ENABLE_FIFO ? up->tx_loadsz : 1);
+	/*
 	 *	Now, do each character
 	 */
-	for (i = 0; i < count; i++, s++) {
-		wait_for_xmitr(up);
+	for (i = 0; i < count; ) {
+		int	fifo;
 
+		wait_for_xmitr(up);
+		fifo = tx_loadsz;
 		/*
-		 *	Send the character out.
+		 *	Send the character out using FIFO.
 		 *	If a LF, also do CR...
 		 */
-		serial_out(up, UART_TX, *s);
-		if (*s == 10) {
-			wait_for_xmitr(up);
-			serial_out(up, UART_TX, 13);
-		}
+		do {
+			serial_out(up, UART_TX, *s);
+			fifo--;
+			if (*s == 10) {
+				if (fifo > 0) {
+					serial_out(up, UART_TX, 13);
+					fifo--;
+				} else {
+					/* No room to add CR */
+					wait_for_xmitr(up);
+					fifo = tx_loadsz;
+					serial_out(up, UART_TX, 13);
+					fifo--;
+				}
+			}
+			i++;
+			s++;
+		} while (fifo > 0 && i < count);
 	}
 
 	/*

Re: performance-improvement-of-serial-console-via-virtual.patch added to -mm tree

[Hironobu Ishii]

Hi Russel,
> 
> The problem is that the console write method may be called prior to
> autoconfig() being run for the port in question, so tx_loadsz may be
> uninitialised.

Thank you for explanation.

>> > * if it has been initialised
>> > * how much data is already contained in the FIFO
>> 
>> Right, we can't know how many byte exist in the FIFO.
>> So this patch is waiting the FIFO becomes empty at first
>> by calling "wait_for_xmitr(up)".
>> (This is the same logic with original.)
>> 
>> After TX FIFO become empty, we can decide the available 
>> TX FIFO depth by up->tx_loadsize.
> 
> Only if you ignore the fact that tx_loadsz may not be initialised.

OK.
Before initialization, does tx_loadsz left 0?
If so, we can easily solve the problem:

   tx_loadsz = (up->tx_loadsz ? up->tx_loadsz : 1); <-----
   for (i = 0; i < count; ) {
           int     fifo;

           wait_for_xmitr(up);
           fifo = tx_loadsz;        <------
            .
            .
            .


Best regards,
Hironobu Ishii