[PATCH 0/3] Fix sysrq caused USB performance regressions and leak
|
View:
New views
10 Messages
—
Rating Filter:
Alert me
|
 |
[PATCH 0/3] Fix sysrq caused USB performance regressions and leak
by Alan Cox
::
Rate this Message:
---
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@... More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |
|
|
[PATCH 1/3] tty: Sort out the USB sysrq changes that wrecked performance
by Alan Cox
::
Rate this Message:
From: Alan Cox <alan@...>
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@...> --- 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); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@... More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |
|
|
[PATCH 2/3] tty: Fix USB kref leak
by Alan Cox
::
Rate this Message:
From: Alan Cox <alan@...>
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@...> --- 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); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@... More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |
|
|
[PATCH 3/3] tty: Fix the PL2303 private methods for sysrq
by Alan Cox
::
Rate this Message:
From: Alan Cox <alan@...>
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@...> --- 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 */ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@... 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
by Anders Larsen
::
Rate this Message:
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@... 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
by Fengwei Yin-2
::
Rate this Message:
Hi Alan,
On Thu, Jul 9, 2009 at 8:35 PM, Alan Cox<alan@...> wrote: > From: Alan Cox <alan@...> > > 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@...> > --- > > 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@... > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@... 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
by Fengwei Yin-2
::
Rate this Message:
On Thu, Jul 9, 2009 at 11:49 PM, Fengwei Yin<yfw.kernel@...> wrote:
> Hi Alan, > > On Thu, Jul 9, 2009 at 8:35 PM, Alan Cox<alan@...> wrote: >> From: Alan Cox <alan@...> >> >> 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@...> >> --- >> >> 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 > > Regards > Yin, Fengwei > >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >> the body of a message to majordomo@... >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/ >> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@... 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
by Alan Cox
::
Rate this Message:
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@... 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
by Anders Larsen
::
Rate this Message:
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@... 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
by Alan Cox
::
Rate this Message:
On Fri, 10 Jul 2009 11:07:53 +0200
Anders Larsen <al@...> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@... More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |
| Free embeddable forum powered by Nabble | Forum Help |
