Skip to contentSkip to conversation

usb: gadget: dummy_hcd: Switch to hrtimers for better performance

PATCH New Marcello Sylvester Bauer 3 participants 5 messages Apr 11, 2024 USB

lore
Share & tools
lore
#3 lore raw

PATCH 2/2 usb: gadget: dummy_hcd: Set transfer interval to 1 microframe

In reply to Marcello Sylvester Bauer

From: Marcello Sylvester Bauer <[email protected]>

Currently, the transfer polling interval is set to 1ms, which is the frame rate of full-speed and low-speed USB. The USB 2.0 specification introduces microframes (125 microseconds) to improve the timing precision of data transfers.

Reducing the transfer interval to 1 microframe increases data throughput for high-speed and super-speed USB communication

---
 drivers/usb/gadget/udc/dummy_hcd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/usb/gadget/udc/dummy_hcd.c +4 -2
index dab559d8ee8c..f37b0d8386c1 100644
@@ -50,6 +50,8 @@
 #define POWER_BUDGET	500	/* in mA; use 8 for low-power port testing */
 #define POWER_BUDGET_3	900	/* in mA */
 
+#define DUMMY_TIMER_INT_NSECS	125000 /* 1 microframe */
+
 static const char	driver_name[] = "dummy_hcd";
 static const char	driver_desc[] = "USB Host+Gadget Emulator";
 
@@ -1302,7 +1304,7 @@ static int dummy_urb_enqueue(
 
 	/* kick the scheduler, it'll do the rest */
 	if (!hrtimer_active(&dum_hcd->timer))
-		hrtimer_start(&dum_hcd->timer, ms_to_ktime(1), HRTIMER_MODE_REL);
+		hrtimer_start(&dum_hcd->timer, ns_to_ktime(DUMMY_TIMER_INT_NSECS), HRTIMER_MODE_REL);
 
  done:
 	spin_unlock_irqrestore(&dum_hcd->dum->lock, flags);
@@ -1993,7 +1995,7 @@ static enum hrtimer_restart dummy_timer(struct hrtimer *t)
 		dum_hcd->udev = NULL;
 	} else if (dum_hcd->rh_state == DUMMY_RH_RUNNING) {
 		/* want a 1 msec delay here */
-		hrtimer_start(&dum_hcd->timer, ms_to_ktime(1), HRTIMER_MODE_REL);
+		hrtimer_start(&dum_hcd->timer, ns_to_ktime(DUMMY_TIMER_INT_NSECS), HRTIMER_MODE_REL);
 	}
 
 	spin_unlock_irqrestore(&dum->lock, flags);
Signature · 1 line
2.44.0
#1 lore raw

PATCH 1/2 usb: gadget: dummy_hcd: Switch to hrtimer transfer scheduler

In reply to Marcello Sylvester Bauer

From: Marcello Sylvester Bauer <[email protected]>

The dummy_hcd transfer scheduler assumes that the internal kernel timer frequency is set to 1000Hz to give a polling interval of 1ms. Reducing the timer frequency will result in an anti-proportional reduction in transfer performance. Switch to a hrtimer to decouple this association.

---
 drivers/usb/gadget/udc/dummy_hcd.c | 35 +++++++++++++++++-------------
 1 file changed, 20 insertions(+), 15 deletions(-)
drivers/usb/gadget/udc/dummy_hcd.c +20 -15
index 0953e1b5c030..dab559d8ee8c 100644
@@ -30,7 +30,7 @@
 #include <linux/slab.h>
 #include <linux/errno.h>
 #include <linux/init.h>
-#include <linux/timer.h>
+#include <linux/hrtimer.h>
 #include <linux/list.h>
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
@@ -240,7 +240,7 @@ enum dummy_rh_state {
 struct dummy_hcd {
 	struct dummy			*dum;
 	enum dummy_rh_state		rh_state;
-	struct timer_list		timer;
+	struct hrtimer			timer;
 	u32				port_status;
 	u32				old_status;
 	unsigned long			re_timeout;
@@ -1301,8 +1301,8 @@ static int dummy_urb_enqueue(
 		urb->error_count = 1;		/* mark as a new urb */
 
 	/* kick the scheduler, it'll do the rest */
-	if (!timer_pending(&dum_hcd->timer))
-		mod_timer(&dum_hcd->timer, jiffies + 1);
+	if (!hrtimer_active(&dum_hcd->timer))
+		hrtimer_start(&dum_hcd->timer, ms_to_ktime(1), HRTIMER_MODE_REL);
 
  done:
 	spin_unlock_irqrestore(&dum_hcd->dum->lock, flags);
@@ -1323,7 +1323,7 @@ static int dummy_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 	rc = usb_hcd_check_unlink_urb(hcd, urb, status);
 	if (!rc && dum_hcd->rh_state != DUMMY_RH_RUNNING &&
 			!list_empty(&dum_hcd->urbp_list))
-		mod_timer(&dum_hcd->timer, jiffies);
+		hrtimer_start(&dum_hcd->timer, ns_to_ktime(0), HRTIMER_MODE_REL);
 
 	spin_unlock_irqrestore(&dum_hcd->dum->lock, flags);
 	return rc;
@@ -1777,7 +1777,7 @@ static int handle_control_request(struct dummy_hcd *dum_hcd, struct urb *urb,
  * drivers except that the callbacks are invoked from soft interrupt
  * context.
  */
-static void dummy_timer(struct timer_list *t)
+static enum hrtimer_restart dummy_timer(struct hrtimer *t)
 {
 	struct dummy_hcd	*dum_hcd = from_timer(dum_hcd, t, timer);
 	struct dummy		*dum = dum_hcd->dum;
@@ -1808,8 +1808,6 @@ static void dummy_timer(struct timer_list *t)
 		break;
 	}
 
-	/* FIXME if HZ != 1000 this will probably misbehave ... */
-
 	/* look at each urb queued by the host side driver */
 	spin_lock_irqsave(&dum->lock, flags);
 
@@ -1817,7 +1815,7 @@ static void dummy_timer(struct timer_list *t)
 		dev_err(dummy_dev(dum_hcd),
 				"timer fired with no URBs pending?\n");
 		spin_unlock_irqrestore(&dum->lock, flags);
-		return;
+		return HRTIMER_NORESTART;
 	}
 	dum_hcd->next_frame_urbp = NULL;
 
@@ -1995,10 +1993,12 @@ static void dummy_timer(struct timer_list *t)
 		dum_hcd->udev = NULL;
 	} else if (dum_hcd->rh_state == DUMMY_RH_RUNNING) {
 		/* want a 1 msec delay here */
-		mod_timer(&dum_hcd->timer, jiffies + msecs_to_jiffies(1));
+		hrtimer_start(&dum_hcd->timer, ms_to_ktime(1), HRTIMER_MODE_REL);
 	}
 
 	spin_unlock_irqrestore(&dum->lock, flags);
+
+	return HRTIMER_NORESTART;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -2387,7 +2387,7 @@ static int dummy_bus_resume(struct usb_hcd *hcd)
 		dum_hcd->rh_state = DUMMY_RH_RUNNING;
 		set_link_state(dum_hcd);
 		if (!list_empty(&dum_hcd->urbp_list))
-			mod_timer(&dum_hcd->timer, jiffies);
+			hrtimer_start(&dum_hcd->timer, ns_to_ktime(0), HRTIMER_MODE_REL);
 		hcd->state = HC_STATE_RUNNING;
 	}
 	spin_unlock_irq(&dum_hcd->dum->lock);
@@ -2465,7 +2465,8 @@ static DEVICE_ATTR_RO(urbs);
 
 static int dummy_start_ss(struct dummy_hcd *dum_hcd)
 {
-	timer_setup(&dum_hcd->timer, dummy_timer, 0);
+	hrtimer_init(&dum_hcd->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	dum_hcd->timer.function = dummy_timer;
 	dum_hcd->rh_state = DUMMY_RH_RUNNING;
 	dum_hcd->stream_en_ep = 0;
 	INIT_LIST_HEAD(&dum_hcd->urbp_list);
@@ -2494,7 +2495,8 @@ static int dummy_start(struct usb_hcd *hcd)
 		return dummy_start_ss(dum_hcd);
 
 	spin_lock_init(&dum_hcd->dum->lock);
-	timer_setup(&dum_hcd->timer, dummy_timer, 0);
+	hrtimer_init(&dum_hcd->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	dum_hcd->timer.function = dummy_timer;
 	dum_hcd->rh_state = DUMMY_RH_RUNNING;
 
 	INIT_LIST_HEAD(&dum_hcd->urbp_list);
@@ -2513,8 +2515,11 @@ static int dummy_start(struct usb_hcd *hcd)
 
 static void dummy_stop(struct usb_hcd *hcd)
 {
-	device_remove_file(dummy_dev(hcd_to_dummy_hcd(hcd)), &dev_attr_urbs);
-	dev_info(dummy_dev(hcd_to_dummy_hcd(hcd)), "stopped\n");
+	struct dummy_hcd	*dum_hcd = hcd_to_dummy_hcd(hcd);
+
+	hrtimer_cancel(&dum_hcd->timer);
+	device_remove_file(dummy_dev(dum_hcd), &dev_attr_urbs);
+	dev_info(dummy_dev(dum_hcd), "stopped\n");
 }
 
 /*-------------------------------------------------------------------------*/
Signature · 1 line
2.44.0
#0 lore raw

cover usb: gadget: dummy_hcd: Switch to hrtimers for better performance

About 10 years ago, there was a discussion about the poor performance of dummy_hcd due to the use of jiffies instead of hrtimers [1]. There was a proposal to switch to hrtimers, but the patch never made it into the mainline, and I cannot figure out why.

This patch set brings the idea back to the mailing list with the intention of improving the overall performance of this driver.

[1](https://linux-usb.vger.kernel.narkive.com/Fdt5oBR8/dummy-hcd-performance-correctness)

Marcello Sylvester Bauer (2):
usb: gadget: dummy_hcd: Switch to hrtimer transfer scheduler
usb: gadget: dummy_hcd: Set transfer interval to 1 microframe

 drivers/usb/gadget/udc/dummy_hcd.c | 37 ++++++++++++++++++------------
 1 file changed, 22 insertions(+), 15 deletions(-)
Signature · 1 line
2.44.0
#4 lore raw

Re: [PATCH v1 2/2] usb: gadget: dummy_hcd: Set transfer interval to 1 microframe

In reply to Marcello Sylvester Bauer
Quoted 19 lines · Marcello Sylvester Bauer

Currently, the transfer polling interval is set to 1ms, which is the frame rate of full-speed and low-speed USB. The USB 2.0 specification introduces microframes (125 microseconds) to improve the timing precision of data transfers.

Reducing the transfer interval to 1 microframe increases data throughput for high-speed and super-speed USB communication

---
 drivers/usb/gadget/udc/dummy_hcd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/usb/gadget/udc/dummy_hcd.c +0 -0
index dab559d8ee8c..f37b0d8386c1 100644

drivers/usb/gadget/udc/dummy_hcd.c line 53

Quoted 4 lines
drivers/usb/gadget/udc/dummy_hcd.c +1 -0
@@ -50,6 +50,8 @@
 #define POWER_BUDGET	500	/* in mA; use 8 for low-power port testing */
 #define POWER_BUDGET_3	900	/* in mA */
 
+#define DUMMY_TIMER_INT_NSECS	125000 /* 1 microframe */

If the is_super_speed and is_high_speed module parameters are both false then the timer should run at 1-ms intervals. So this needs to be something that can be adjusted at runtime, not a constant.

Alan Stern

Reviewed-by on v1 1/2
#2 lore raw

Re: [PATCH v1 1/2] usb: gadget: dummy_hcd: Switch to hrtimer transfer scheduler

In reply to Marcello Sylvester Bauer
Quoted 8 lines · Marcello Sylvester Bauer

The dummy_hcd transfer scheduler assumes that the internal kernel timer frequency is set to 1000Hz to give a polling interval of 1ms. Reducing the timer frequency will result in an anti-proportional reduction in transfer performance. Switch to a hrtimer to decouple this association.

---