On Fri, 30 May 2014 pktoss@xxxxxxxxx wrote:
> From: Pantelis Koukousoulas <pktoss@xxxxxxxxx>
>
> dummy_hcd uses jiffies and seems to assume that HZ=1000 and no
> tickless behavior. This makes for some horrible performance in
> ordinary desktop kernels (tickless / HZ=250) as found in distros.
>
> This patch ports dummy_hcd to use hrtimers instead, which allows
> for reasonable performance in distro kernels as well.
> (Around 25MB/s for g_mass_storage, around 100MB/s for UAS).
>
> Implementation uses the tasklet_hrtimer framework. This means
> dummy_timer callback is executed in softirq context (as it was
> with wheel timers) which keeps required changes to a minimum.
Good work.
> -static void dummy_timer(unsigned long _dum_hcd)
> +static enum hrtimer_restart dummy_timer(struct hrtimer *timer)
> {
> - struct dummy_hcd *dum_hcd = (struct dummy_hcd *) _dum_hcd;
> + struct tasklet_hrtimer *ttimer = container_of(timer,
> + struct tasklet_hrtimer,
> + timer);
> + struct dummy_hcd *dum_hcd = container_of(ttimer,
> + struct dummy_hcd,
> + ttimer);
You don't need two statements like this. Just do:
+ struct dummy_hcd *dum_hcd = container_of(timer,
struct dummy_hcd, ttimer.timer);
By the way, the style used in this file is that continuation lines are
indented by two tab stops beyond the original line. (Sometimes four
spaces, if two tab stops would be too much.) They don't line up with
opening parens or anything like this.
> @@ -2316,9 +2337,7 @@ static DEVICE_ATTR_RO(urbs);
>
> static int dummy_start_ss(struct dummy_hcd *dum_hcd)
> {
> - init_timer(&dum_hcd->timer);
> - dum_hcd->timer.function = dummy_timer;
> - dum_hcd->timer.data = (unsigned long)dum_hcd;
> + tasklet_hrtimer_init(&dum_hcd->ttimer, dummy_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
> dum_hcd->rh_state = DUMMY_RH_RUNNING;
> dum_hcd->stream_en_ep = 0;
> INIT_LIST_HEAD(&dum_hcd->urbp_list);
> @@ -2347,9 +2366,7 @@ static int dummy_start(struct usb_hcd *hcd)
> return dummy_start_ss(dum_hcd);
>
> spin_lock_init(&dum_hcd->dum->lock);
> - init_timer(&dum_hcd->timer);
> - dum_hcd->timer.function = dummy_timer;
> - dum_hcd->timer.data = (unsigned long)dum_hcd;
> + tasklet_hrtimer_init(&dum_hcd->ttimer, dummy_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
> dum_hcd->rh_state = DUMMY_RH_RUNNING;
This looks weird, doesn't it? As far as I can see, the only things
that are different between dummy_start() and dummy_start_ss() are the
spin_lock_init, stream_en_ep, and device_create_file. I'd like to see
this code refactored. Maybe in a third patch?
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
