usb: gadget: dummy_hcd: Switch to hrtimers for better performance
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
Marcello Sylvester Bauer
PATCH 1/2 usb: gadget: dummy_hcd: Switch to hrtimer transfer scheduler
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
@@ -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
Re: [PATCH v1 1/2] usb: gadget: dummy_hcd: Switch to hrtimer transfer scheduler
Quoted 8 lines · Marcello Sylvester Bauer
The
dummy_hcdtransfer 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.Signed-off-by Marcello Sylvester Bauer <[email protected]>Signed-off-by Marcello Sylvester Bauer <[email protected]>---
Marcello Sylvester Bauer
PATCH 2/2 usb: gadget: dummy_hcd: Set transfer interval to 1 microframe
In reply to Marcello Sylvester BauerFrom: 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
@@ -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
Re: [PATCH v1 2/2] usb: gadget: dummy_hcd: Set transfer interval to 1 microframe
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
Signed-off-by Marcello Sylvester Bauer <[email protected]>Signed-off-by Marcello Sylvester Bauer <[email protected]>--- drivers/usb/gadget/udc/dummy_hcd.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/usb/gadget/udc/dummy_hcd.c +0
-0
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