* [PATCH 1/3] tty: Sort out the USB sysrq changes that wrecked performance
2009-07-09 12:35 [PATCH 0/3] Fix sysrq caused USB performance regressions and leak Alan Cox
@ 2009-07-09 12:35 ` Alan Cox
2009-07-09 15:49 ` Fengwei Yin
2009-07-09 12:36 ` [PATCH 2/3] tty: Fix USB kref leak Alan Cox
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Alan Cox @ 2009-07-09 12:35 UTC (permalink / raw)
To: torvalds, linux-kernel
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);
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] tty: Sort out the USB sysrq changes that wrecked performance
2009-07-09 12:35 ` [PATCH 1/3] tty: Sort out the USB sysrq changes that wrecked performance Alan Cox
@ 2009-07-09 15:49 ` Fengwei Yin
2009-07-09 23:22 ` Fengwei Yin
0 siblings, 1 reply; 10+ messages in thread
From: Fengwei Yin @ 2009-07-09 15:49 UTC (permalink / raw)
To: Alan Cox; +Cc: torvalds, linux-kernel
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/
>
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] tty: Sort out the USB sysrq changes that wrecked performance
2009-07-09 15:49 ` Fengwei Yin
@ 2009-07-09 23:22 ` Fengwei Yin
0 siblings, 0 replies; 10+ messages in thread
From: Fengwei Yin @ 2009-07-09 23:22 UTC (permalink / raw)
To: Alan Cox; +Cc: torvalds, linux-kernel
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/
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] tty: Fix USB kref leak
2009-07-09 12:35 [PATCH 0/3] Fix sysrq caused USB performance regressions and leak Alan Cox
2009-07-09 12:35 ` [PATCH 1/3] tty: Sort out the USB sysrq changes that wrecked performance Alan Cox
@ 2009-07-09 12:36 ` Alan Cox
2009-07-09 12:36 ` [PATCH 3/3] tty: Fix the PL2303 private methods for sysrq Alan Cox
2009-07-09 15:46 ` [PATCH 0/3] Fix sysrq caused USB performance regressions and leak Anders Larsen
3 siblings, 0 replies; 10+ messages in thread
From: Alan Cox @ 2009-07-09 12:36 UTC (permalink / raw)
To: torvalds, linux-kernel
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);
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 3/3] tty: Fix the PL2303 private methods for sysrq
2009-07-09 12:35 [PATCH 0/3] Fix sysrq caused USB performance regressions and leak Alan Cox
2009-07-09 12:35 ` [PATCH 1/3] tty: Sort out the USB sysrq changes that wrecked performance Alan Cox
2009-07-09 12:36 ` [PATCH 2/3] tty: Fix USB kref leak Alan Cox
@ 2009-07-09 12:36 ` Alan Cox
2009-07-09 15:46 ` [PATCH 0/3] Fix sysrq caused USB performance regressions and leak Anders Larsen
3 siblings, 0 replies; 10+ messages in thread
From: Alan Cox @ 2009-07-09 12:36 UTC (permalink / raw)
To: torvalds, linux-kernel
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 */
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 0/3] Fix sysrq caused USB performance regressions and leak
2009-07-09 12:35 [PATCH 0/3] Fix sysrq caused USB performance regressions and leak Alan Cox
` (2 preceding siblings ...)
2009-07-09 12:36 ` [PATCH 3/3] tty: Fix the PL2303 private methods for sysrq Alan Cox
@ 2009-07-09 15:46 ` Anders Larsen
2009-07-10 0:02 ` Alan Cox
3 siblings, 1 reply; 10+ messages in thread
From: Anders Larsen @ 2009-07-09 15:46 UTC (permalink / raw)
To: Alan Cox; +Cc: torvalds, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 0/3] Fix sysrq caused USB performance regressions and leak
2009-07-09 15:46 ` [PATCH 0/3] Fix sysrq caused USB performance regressions and leak Anders Larsen
@ 2009-07-10 0:02 ` Alan Cox
2009-07-10 9:07 ` Anders Larsen
0 siblings, 1 reply; 10+ messages in thread
From: Alan Cox @ 2009-07-10 0:02 UTC (permalink / raw)
To: Anders Larsen; +Cc: torvalds, linux-kernel
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 0/3] Fix sysrq caused USB performance regressions and leak
2009-07-10 0:02 ` Alan Cox
@ 2009-07-10 9:07 ` Anders Larsen
2009-07-10 9:34 ` Alan Cox
0 siblings, 1 reply; 10+ messages in thread
From: Anders Larsen @ 2009-07-10 9:07 UTC (permalink / raw)
To: Alan Cox; +Cc: torvalds, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 0/3] Fix sysrq caused USB performance regressions and leak
2009-07-10 9:07 ` Anders Larsen
@ 2009-07-10 9:34 ` Alan Cox
0 siblings, 0 replies; 10+ messages in thread
From: Alan Cox @ 2009-07-10 9:34 UTC (permalink / raw)
To: Anders Larsen; +Cc: torvalds, linux-kernel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread