---
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
From: Alan Cox <[email protected]>
We can't go around calling all sorts of magic per character functions at
full rate 3G data speed.
Signed-off-by: Alan Cox <[email protected]>
---
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);
From: Alan Cox <[email protected]>
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 <[email protected]>
---
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);
From: Alan Cox <[email protected]>
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 <[email protected]>
---
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 */
Hi Alan,
On Thu, Jul 9, 2009 at 8:35 PM, Alan Cox<[email protected]> wrote:
> From: Alan Cox <[email protected]>
>
> We can't go around calling all sorts of magic per character functions at
> full rate 3G data speed.
>
> Signed-off-by: Alan Cox <[email protected]>
> ---
>
> ?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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
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
On Thu, Jul 9, 2009 at 11:49 PM, Fengwei Yin<[email protected]> wrote:
> Hi Alan,
>
> On Thu, Jul 9, 2009 at 8:35 PM, Alan Cox<[email protected]> wrote:
>> From: Alan Cox <[email protected]>
>>
>> We can't go around calling all sorts of magic per character functions at
>> full rate 3G data speed.
>>
>> Signed-off-by: Alan Cox <[email protected]>
>> ---
>>
>> ?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 [email protected]
>> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at ?http://www.tux.org/lkml/
>>
>
> 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
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
On Fri, 10 Jul 2009 11:07:53 +0200
Anders Larsen <[email protected]> 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.