[PATCH 0/3] Fix sysrq caused USB performance regressions and leak

[PATCH 0/3] Fix sysrq caused USB performance regressions and leak

[Alan Cox]

---

Alan Cox (3):
      tty: Fix the PL2303 private methods for sysrq
      tty: Fix USB kref leak
      tty: Sort out the USB sysrq changes that wrecked performance


 drivers/usb/serial/ftdi_sio.c |    2 +
 drivers/usb/serial/generic.c  |   20 ++++++++++----
 drivers/usb/serial/pl2303.c   |   58 +++++++++++++++++++++++------------------
 include/linux/usb/serial.h    |    3 +-
 4 files changed, 50 insertions(+), 33 deletions(-)

-- 
	"That was said by Eric Raymond who belongs to another movement" 
			- Richard Stallman

[PATCH 1/3] tty: Sort out the USB sysrq changes that wrecked performance

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

We can't go around calling all sorts of magic per character functions at
full rate 3G data speed.

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/usb/serial/generic.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)


diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 932d624..e9aa7a4 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -424,10 +424,17 @@ static void flush_and_resubmit_read_urb(struct usb_serial_port *port)
 	if (!tty)
 		goto done;
 
-	/* Push data to tty */
-	for (i = 0; i < urb->actual_length; i++, ch++) {
-		if (!usb_serial_handle_sysrq_char(port, *ch))
-			tty_insert_flip_char(tty, *ch, TTY_NORMAL);
+	/* The per character mucking around with sysrq path it too slow for
+	   stuff like 3G modems, so shortcircuit it in the 99.9999999% of cases
+	   where the USB serial is not a console anyway */	
+	if (!port->console || !port->sysrq)
+		tty_insert_flip_string(tty, ch, urb->actual_length);
+	else {	
+		/* Push data to tty */
+		for (i = 0; i < urb->actual_length; i++, ch++) {
+			if (!usb_serial_handle_sysrq_char(port, *ch))
+				tty_insert_flip_char(tty, *ch, TTY_NORMAL);
+		}
 	}
 	tty_flip_buffer_push(tty);
 	tty_kref_put(tty);

[PATCH 2/3] tty: Fix USB kref leak

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

The sysrq code acquired a kref leak. Fix it by passing the tty separately
from the caller (thus effectively using the callers kref which all the
callers hold anyway)

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/usb/serial/ftdi_sio.c |    2 +-
 drivers/usb/serial/generic.c  |    7 ++++---
 drivers/usb/serial/pl2303.c   |    2 +-
 include/linux/usb/serial.h    |    3 ++-
 4 files changed, 8 insertions(+), 6 deletions(-)


diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 3dc3768..5f08702 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -2121,7 +2121,7 @@ static void ftdi_process_read(struct work_struct *work)
 				/* Note that the error flag is duplicated for
 				   every character received since we don't know
 				   which character it applied to */
-				if (!usb_serial_handle_sysrq_char(port,
+				if (!usb_serial_handle_sysrq_char(tty, port,
 						data[packet_offset + i]))
 					tty_insert_flip_char(tty,
 						data[packet_offset + i],
diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index e9aa7a4..21dd6e7 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -432,7 +432,7 @@ static void flush_and_resubmit_read_urb(struct usb_serial_port *port)
 	else {	
 		/* Push data to tty */
 		for (i = 0; i < urb->actual_length; i++, ch++) {
-			if (!usb_serial_handle_sysrq_char(port, *ch))
+			if (!usb_serial_handle_sysrq_char(tty, port, *ch))
 				tty_insert_flip_char(tty, *ch, TTY_NORMAL);
 		}
 	}
@@ -534,11 +534,12 @@ void usb_serial_generic_unthrottle(struct tty_struct *tty)
 	}
 }
 
-int usb_serial_handle_sysrq_char(struct usb_serial_port *port, unsigned int ch)
+int usb_serial_handle_sysrq_char(struct tty_struct *tty,
+			struct usb_serial_port *port, unsigned int ch)
 {
 	if (port->sysrq && port->console) {
 		if (ch && time_before(jiffies, port->sysrq)) {
-			handle_sysrq(ch, tty_port_tty_get(&port->port));
+			handle_sysrq(ch, tty);
 			port->sysrq = 0;
 			return 1;
 		}
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index ec6c132..8835802 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -1038,7 +1038,7 @@ static void pl2303_read_bulk_callback(struct urb *urb)
 		if (line_status & UART_OVERRUN_ERROR)
 			tty_insert_flip_char(tty, 0, TTY_OVERRUN);
 		for (i = 0; i < urb->actual_length; ++i)
-			if (!usb_serial_handle_sysrq_char(port, data[i]))
+			if (!usb_serial_handle_sysrq_char(tty, port, data[i]))
 				tty_insert_flip_char(tty, data[i], tty_flag);
 		tty_flip_buffer_push(tty);
 	}
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index 44801d2..0ec50ba 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -317,7 +317,8 @@ extern int usb_serial_generic_register(int debug);
 extern void usb_serial_generic_deregister(void);
 extern void usb_serial_generic_resubmit_read_urb(struct usb_serial_port *port,
 						 gfp_t mem_flags);
-extern int usb_serial_handle_sysrq_char(struct usb_serial_port *port,
+extern int usb_serial_handle_sysrq_char(struct tty_struct *tty,
+					struct usb_serial_port *port,
 					unsigned int ch);
 extern int usb_serial_handle_break(struct usb_serial_port *port);
 

[PATCH 3/3] tty: Fix the PL2303 private methods for sysrq

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

PL2303 has private data shovelling methods that also have no fast path. Fix
them to work the same way as the default handler.

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/usb/serial/pl2303.c |   58 ++++++++++++++++++++++++-------------------
 1 files changed, 33 insertions(+), 25 deletions(-)


diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index 8835802..39cf3b4 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -971,18 +971,46 @@ exit:
 			__func__, retval);
 }
 
+static void pl2303_push_data(struct tty_struct *tty,
+		struct usb_serial_port *port, struct urb *urb,
+		u8 line_status)
+{
+	unsigned char *data = urb->transfer_buffer;
+	/* get tty_flag from status */
+	char tty_flag = TTY_NORMAL;
+	/* break takes precedence over parity, */
+	/* which takes precedence over framing errors */
+	if (line_status & UART_BREAK_ERROR)
+		tty_flag = TTY_BREAK;
+	else if (line_status & UART_PARITY_ERROR)
+		tty_flag = TTY_PARITY;
+	else if (line_status & UART_FRAME_ERROR)
+		tty_flag = TTY_FRAME;
+	dbg("%s - tty_flag = %d", __func__, tty_flag);
+	
+	tty_buffer_request_room(tty, urb->actual_length + 1);
+	/* overrun is special, not associated with a char */
+	if (line_status & UART_OVERRUN_ERROR)
+		tty_insert_flip_char(tty, 0, TTY_OVERRUN);
+	if (port->console && port->sysrq) {
+		int i;
+		for (i = 0; i < urb->actual_length; ++i)
+			if (!usb_serial_handle_sysrq_char(tty, port, data[i]))
+				tty_insert_flip_char(tty, data[i], tty_flag);
+	} else
+		tty_insert_flip_string(tty, data, urb->actual_length);
+	tty_flip_buffer_push(tty);
+}
+
 static void pl2303_read_bulk_callback(struct urb *urb)
 {
 	struct usb_serial_port *port =  urb->context;
 	struct pl2303_private *priv = usb_get_serial_port_data(port);
 	struct tty_struct *tty;
-	unsigned char *data = urb->transfer_buffer;
 	unsigned long flags;
-	int i;
 	int result;
 	int status = urb->status;
 	u8 line_status;
-	char tty_flag;
 
 	dbg("%s - port %d", __func__, port->number);
 
@@ -1010,10 +1038,7 @@ static void pl2303_read_bulk_callback(struct urb *urb)
 	}
 
 	usb_serial_debug_data(debug, &port->dev, __func__,
-			      urb->actual_length, data);
-
-	/* get tty_flag from status */
-	tty_flag = TTY_NORMAL;
+			      urb->actual_length, urb->transfer_buffer);
 
 	spin_lock_irqsave(&priv->lock, flags);
 	line_status = priv->line_status;
@@ -1021,26 +1046,9 @@ static void pl2303_read_bulk_callback(struct urb *urb)
 	spin_unlock_irqrestore(&priv->lock, flags);
 	wake_up_interruptible(&priv->delta_msr_wait);
 
-	/* break takes precedence over parity, */
-	/* which takes precedence over framing errors */
-	if (line_status & UART_BREAK_ERROR)
-		tty_flag = TTY_BREAK;
-	else if (line_status & UART_PARITY_ERROR)
-		tty_flag = TTY_PARITY;
-	else if (line_status & UART_FRAME_ERROR)
-		tty_flag = TTY_FRAME;
-	dbg("%s - tty_flag = %d", __func__, tty_flag);
-
 	tty = tty_port_tty_get(&port->port);
 	if (tty && urb->actual_length) {
-		tty_buffer_request_room(tty, urb->actual_length + 1);
-		/* overrun is special, not associated with a char */
-		if (line_status & UART_OVERRUN_ERROR)
-			tty_insert_flip_char(tty, 0, TTY_OVERRUN);
-		for (i = 0; i < urb->actual_length; ++i)
-			if (!usb_serial_handle_sysrq_char(tty, port, data[i]))
-				tty_insert_flip_char(tty, data[i], tty_flag);
-		tty_flip_buffer_push(tty);
+		pl2303_push_data(tty, port, urb, line_status);
 	}
 	tty_kref_put(tty);
 	/* Schedule the next read _if_ we are still open */

Re: [PATCH 0/3] Fix sysrq caused USB performance regressions and leak

[Anders Larsen]

Apart from very similar code being added differently,
here this way around...

>  drivers/usb/serial/generic.c |   15 +++++++++++----
> 
> +	if (!port->console || !port->sysrq)
> +		tty_insert_flip_string(tty, ch, urb->actual_length);
> +	else {	
> +		/* Push data to tty */
> +		for (i = 0; i < urb->actual_length; i++, ch++) {
> +			if (!usb_serial_handle_sysrq_char(port, *ch))
> +				tty_insert_flip_char(tty, *ch, TTY_NORMAL);
> +		}

...and there the other way around,

>  drivers/usb/serial/pl2303.c |   58 ++++++++++++++++++++++++-------------------
> 
> +	if (port->console && port->sysrq) {
> +		int i;
> +		for (i = 0; i < urb->actual_length; ++i)
> +			if (!usb_serial_handle_sysrq_char(tty, port, data[i]))
> +				tty_insert_flip_char(tty, data[i], tty_flag);
> +	} else
> +		tty_insert_flip_string(tty, data, urb->actual_length);

shouldn't it be
+	if (likely(!port->console || !port->sysrq))
respectively
+	if (unlikely(port->console && port->sysrq)) {

at least for clarity?

Cheers
Anders

Re: [PATCH 1/3] tty: Sort out the USB sysrq changes that wrecked performance

[Fengwei Yin]

Hi Alan,

On Thu, Jul 9, 2009 at 8:35 PM, Alan Cox<alan@lxorguk.ukuu.org.uk> wrote:
> From: Alan Cox <alan@linux.intel.com>
>
> We can't go around calling all sorts of magic per character functions at
> full rate 3G data speed.
>
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> ---
>
>  drivers/usb/serial/generic.c |   15 +++++++++++----
>  1 files changed, 11 insertions(+), 4 deletions(-)
>
>
> diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
> index 932d624..e9aa7a4 100644
> --- a/drivers/usb/serial/generic.c
> +++ b/drivers/usb/serial/generic.c
> @@ -424,10 +424,17 @@ static void flush_and_resubmit_read_urb(struct usb_serial_port *port)
>        if (!tty)
>                goto done;
>
> -       /* Push data to tty */
> -       for (i = 0; i < urb->actual_length; i++, ch++) {
> -               if (!usb_serial_handle_sysrq_char(port, *ch))
> -                       tty_insert_flip_char(tty, *ch, TTY_NORMAL);
> +       /* The per character mucking around with sysrq path it too slow for
> +          stuff like 3G modems, so shortcircuit it in the 99.9999999% of cases
> +          where the USB serial is not a console anyway */
> +       if (!port->console || !port->sysrq)
> +               tty_insert_flip_string(tty, ch, urb->actual_length);
> +       else {
> +               /* Push data to tty */
> +               for (i = 0; i < urb->actual_length; i++, ch++) {
> +                       if (!usb_serial_handle_sysrq_char(port, *ch))
> +                               tty_insert_flip_char(tty, *ch, TTY_NORMAL);
> +               }
>        }
>        tty_flip_buffer_push(tty);
>        tty_kref_put(tty);
>

What about the change like this:

diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 932d624..b4fd33b 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -424,10 +424,26 @@ static void flush_and_resubmit_read_urb(struct
usb_serial_port *port)
 	if (!tty)
 		goto done;

-	/* Push data to tty */
-	for (i = 0; i < urb->actual_length; i++, ch++) {
-		if (!usb_serial_handle_sysrq_char(port, *ch))
-			tty_insert_flip_char(tty, *ch, TTY_NORMAL);
+        /* The per character mucking around with sysrq path it too slow for
+           stuff like 3G modems, so shortcircuit it in the 99.9999999% of cases
+           where the USB serial is not a console anyway */
+        if (!port->console || !port->sysrq)
+                tty_insert_flip_string(tty, ch, urb->actual_length);
+        else {
+		/*
+		 * Most of data shouldn't be sysrq request even when
+		 * tty is a printk console.
+		 */
+		if (time_before(jiffies, port->sysrq)) {
+			/* Push data to tty */
+			for (i = 0; i < urb->actual_length; i++, ch++) {
+				if (!usb_serial_handle_sysrq_char(port, *ch))
+					tty_insert_flip_char(tty, *ch,
+							TTY_NORMAL);
+			}
+		} else {
+			tty_insert_flip_string(tty, ch, urb->actual_length);
+		}
 	}
 	tty_flip_buffer_push(tty);
 	tty_kref_put(tty);


Regards
Yin, Fengwei

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 1/3] tty: Sort out the USB sysrq changes that wrecked performance

[Fengwei Yin]

On Thu, Jul 9, 2009 at 11:49 PM, Fengwei Yin<yfw.kernel@gmail.com> wrote:
> Hi Alan,
>
> On Thu, Jul 9, 2009 at 8:35 PM, Alan Cox<alan@lxorguk.ukuu.org.uk> wrote:
>> From: Alan Cox <alan@linux.intel.com>
>>
>> We can't go around calling all sorts of magic per character functions at
>> full rate 3G data speed.
>>
>> Signed-off-by: Alan Cox <alan@linux.intel.com>
>> ---
>>
>>  drivers/usb/serial/generic.c |   15 +++++++++++----
>>  1 files changed, 11 insertions(+), 4 deletions(-)
>>
>>
>> diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
>> index 932d624..e9aa7a4 100644
>> --- a/drivers/usb/serial/generic.c
>> +++ b/drivers/usb/serial/generic.c
>> @@ -424,10 +424,17 @@ static void flush_and_resubmit_read_urb(struct usb_serial_port *port)
>>        if (!tty)
>>                goto done;
>>
>> -       /* Push data to tty */
>> -       for (i = 0; i < urb->actual_length; i++, ch++) {
>> -               if (!usb_serial_handle_sysrq_char(port, *ch))
>> -                       tty_insert_flip_char(tty, *ch, TTY_NORMAL);
>> +       /* The per character mucking around with sysrq path it too slow for
>> +          stuff like 3G modems, so shortcircuit it in the 99.9999999% of cases
>> +          where the USB serial is not a console anyway */
>> +       if (!port->console || !port->sysrq)
>> +               tty_insert_flip_string(tty, ch, urb->actual_length);
>> +       else {
>> +               /* Push data to tty */
>> +               for (i = 0; i < urb->actual_length; i++, ch++) {
>> +                       if (!usb_serial_handle_sysrq_char(port, *ch))
>> +                               tty_insert_flip_char(tty, *ch, TTY_NORMAL);
>> +               }
>>        }
>>        tty_flip_buffer_push(tty);
>>        tty_kref_put(tty);
>>
>
> What about the change like this:
>
> diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
> index 932d624..b4fd33b 100644
> --- a/drivers/usb/serial/generic.c
> +++ b/drivers/usb/serial/generic.c
> @@ -424,10 +424,26 @@ static void flush_and_resubmit_read_urb(struct
> usb_serial_port *port)
>        if (!tty)
>                goto done;
>
> -       /* Push data to tty */
> -       for (i = 0; i < urb->actual_length; i++, ch++) {
> -               if (!usb_serial_handle_sysrq_char(port, *ch))
> -                       tty_insert_flip_char(tty, *ch, TTY_NORMAL);
> +        /* The per character mucking around with sysrq path it too slow for
> +           stuff like 3G modems, so shortcircuit it in the 99.9999999% of cases
> +           where the USB serial is not a console anyway */
> +        if (!port->console || !port->sysrq)
> +                tty_insert_flip_string(tty, ch, urb->actual_length);
> +        else {
> +               /*
> +                * Most of data shouldn't be sysrq request even when
> +                * tty is a printk console.
> +                */
> +               if (time_before(jiffies, port->sysrq)) {
> +                       /* Push data to tty */
> +                       for (i = 0; i < urb->actual_length; i++, ch++) {
> +                               if (!usb_serial_handle_sysrq_char(port, *ch))
> +                                       tty_insert_flip_char(tty, *ch,
> +                                                       TTY_NORMAL);
> +                       }
> +               } else {
> +                       tty_insert_flip_string(tty, ch, urb->actual_length);
> +               }
>        }
>        tty_flip_buffer_push(tty);
>        tty_kref_put(tty);
>
Oh. Just ignore my change. If it's console, we don't need this "optimization".

Regards
Yin, Fengwei

>
> Regards
> Yin, Fengwei
>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>

Re: [PATCH 0/3] Fix sysrq caused USB performance regressions and leak

[Alan Cox]

> shouldn't it be
> +	if (likely(!port->console || !port->sysrq))
> respectively
> +	if (unlikely(port->console && port->sysrq)) {
> 
> at least for clarity?

It'll get predicted by the CPU just fine I suspect.

Alan

Re: [PATCH 0/3] Fix sysrq caused USB performance regressions and leak

[Anders Larsen]

On 2009-07-10 02:01:37, Alan Cox wrote:
> > shouldn't it be
> > +	if (likely(!port->console || !port->sysrq))
> > respectively
> > +	if (unlikely(port->console && port->sysrq)) {
> > 
> > at least for clarity?
> 
> It'll get predicted by the CPU just fine I suspect.

I thought likely() / unlikely() were for the _compiler_ to arrange the
blocks more efficiently?

Anders

Re: [PATCH 0/3] Fix sysrq caused USB performance regressions and leak

[Alan Cox]

On Fri, 10 Jul 2009 11:07:53 +0200
Anders Larsen <al@alarsen.net> wrote:

> On 2009-07-10 02:01:37, Alan Cox wrote:
> > > shouldn't it be
> > > +	if (likely(!port->console || !port->sysrq))
> > > respectively
> > > +	if (unlikely(port->console && port->sysrq)) {
> > > 
> > > at least for clarity?
> > 
> > It'll get predicted by the CPU just fine I suspect.
> 
> I thought likely() / unlikely() were for the _compiler_ to arrange the
> blocks more efficiently?

Bit of both. Some systems have indicators for whether jumps are likely to
be taken, others you do things like conditionally jump forward to a jump
table which jumps back.

gcc itself says "programmers are notoriously bad at predicting how their
programs actually perform." 8)

Benchmarking I've not had any luck with unlikely() on a modern CPU
(the CPU is dynamically predicting anyway). The pentium iv does add
branch prediction hints (DS: and CS: overrides get reused) but they seem
to make things slower in the case where the CPU will get it right anyway.

So I'd like to see benchmarking evidence for an unlikely on such a hot
and predictible path.