USB interrupt times

Commit Message

Jiri Kosina Aug. 14, 2012, 10:54 a.m. UTC

On Tue, 14 Aug 2012, Russell King wrote:

> I've been trying to track down where all the CPU time goes while
> processing USB interrupts for HID devices.  At the moment, out of
> everything on the cubox, moving the mouse or even pressing a key
> on the keyboard just once is far more expensive than processing an
> interrupt for the network interface or handling the SDHCI port.
> Handing HID actions weigh in at around 300-400us per USB interrupt.
> 
> Around two-thirds of the time for a USB HID device interrupt is
> spent in the HID layer, currently tracked down to:
> 
> 	for (a = 0; a < report->maxfield; a++)
> 		hid_input_field(hid, report->field[a], cdata, interrupt);
> 
> in hid_report_raw_event().
> 
> However, looking at hid_input_report() and its use of down_trylock(),
> I'm wondering why we're trying to run this chunk of code in IRQ
> context anyway?  Why not a tasklet or workqueue?
> 
> Any suggestions on why processing a key press or mouse movement is soo
> expensive?

Actually, Henrik (added to CC) has been doing some latency improvements 
both for input core in general, and for HID devices as well lately. I 
still have his patchset in my to-review queue, as I have just came back 
from offline vacation, but the patch below definitely can't hurt and 
should significantly lower the time spent in handling the irq for hid 
device in common situation (i.e. noone listening for debugfs events).

Could you please measure how much it helps on your system?

Offloading the processing to workqueue might actually work and make sense, 
but I will have to think a little bit more about all the consequences it'd 
have throughout the rest of the code.



From: Henrik Rydberg <[e-mail]>
Subject: HID: Only dump input if someone is listening

Going through the motions of printing the debug message information
takes a long time; using the keyboard can lead to a 160 us irqsoff
latency. This patch skips hid_dump_input() when there are no open
handles, which brings latency down to 100 us.

Signed-off-by: Henrik Rydberg <[e-mail]>
---
 drivers/hid/hid-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Oliver Neukum Aug. 14, 2012, 12:31 p.m. UTC | #1

On Tuesday 14 August 2012 12:54:00 Jiri Kosina wrote:
> Offloading the processing to workqueue might actually work and make sense, 
> but I will have to think a little bit more about all the consequences it'd 
> have throughout the rest of the code.

How would alt-sysrq be handled?

	Reagrds
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 60ea284..5b74e78 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -996,7 +996,8 @@  static void hid_process_event(struct hid_device *hid, struct hid_field *field,
 	struct hid_driver *hdrv = hid->driver;
 	int ret;
 
-	hid_dump_input(hid, usage, value);
+	if (!list_empty(&hid->debug_list))
+		hid_dump_input(hid, usage, value);
 
 	if (hdrv && hdrv->event && hid_match_usage(hid, usage)) {
 		ret = hdrv->event(hid, field, usage, value);