| Message ID | 1372066925-23579-2-git-send-email-ming.lei@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 24, 2013 at 5:42 PM, Ming Lei <ming.lei@canonical.com> wrote: > + > +static void init_giveback_urb_bh(struct usb_hcd *hcd) > +{ > + if (!hcd_giveback_urb_in_bh(hcd)) > + return; Sorry, the above check isn't needed, and the below check isn't needed too. I will fix it in V3. > + > + __init_giveback_urb_bh(&hcd->high_prio_bh); > + __init_giveback_urb_bh(&hcd->low_prio_bh); > +} > + > +static void exit_giveback_urb_bh(struct usb_hcd *hcd) > +{ > + if (!hcd_giveback_urb_in_bh(hcd)) > + return; > + > + /* > + * tasklet_kill() isn't needed here because: > + * - driver's disconnec() called from usb_disconnect() should > + * make sure its URBs are completed during the disconnect() > + * callback > + * > + * - it is too late to run complete() here since driver may have > + * been removed already now > + * > + * Using tasklet to run URB's complete() doesn't change this > + * behavior of usbcore. > + */ > +} > + Thanks, -- Ming Lei
On Mon, 24 Jun 2013, Ming Lei wrote: > This patch implements the mechanism of giveback of URB in > tasklet context, so that hardware interrupt handling time for > usb host controller can be saved much, and HCD interrupt handling > can be simplified. Changes from v1 to v2? > +static void usb_giveback_urb_bh(unsigned long param) > +{ > + struct giveback_urb_bh *bh = (struct giveback_urb_bh *)param; > + unsigned long flags; > + struct list_head local_list; > + > + spin_lock_irqsave(&bh->lock, flags); > + bh->running = 1; > + restart: > + list_replace_init(&bh->head, &local_list); > + spin_unlock_irqrestore(&bh->lock, flags); Tasklet routines are always called with interrupts enabled, right? Therefore you don't need to use the "flags" argument here or below. > @@ -1667,25 +1721,40 @@ int usb_hcd_unlink_urb (struct urb *urb, int status) > */ > void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status) > { > - urb->hcpriv = NULL; > - if (unlikely(urb->unlinked)) > - status = urb->unlinked; > - else if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) && > - urb->actual_length < urb->transfer_buffer_length && > - !status)) > - status = -EREMOTEIO; > + struct giveback_urb_bh *bh; > + bool sched, high_prio_bh; > > - unmap_urb_for_dma(hcd, urb); > - usbmon_urb_complete(&hcd->self, urb, status); > - usb_unanchor_urb(urb); > + /* pass status to tasklet via unlinked */ > + if (likely(!urb->unlinked)) > + urb->unlinked = status; > > - /* pass ownership to the completion handler */ > - urb->status = status; > - urb->complete (urb); > - atomic_dec (&urb->use_count); > - if (unlikely(atomic_read(&urb->reject))) > - wake_up (&usb_kill_urb_queue); > - usb_put_urb (urb); > + if (!hcd_giveback_urb_in_bh(hcd) && !is_root_hub(urb->dev)) { > + __usb_hcd_giveback_urb(urb); > + return; > + } > + > + if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe)) { > + bh = &hcd->high_prio_bh; > + high_prio_bh = 1; > + } else { > + bh = &hcd->low_prio_bh; > + high_prio_bh = 0; > + } Bool values should be assigned "true" or "false", not "1" or "0". > + > + spin_lock(&bh->lock); > + list_add_tail(&urb->urb_list, &bh->head); > + if (bh->running) > + sched = 0; > + else > + sched = 1; > + spin_unlock(&bh->lock); How about calling this variable "running" instead of "sched"? Then you could just say: running = bh->running; with no "if" statement. > + > + if (!sched) > + ; > + else if (high_prio_bh) > + tasklet_hi_schedule(&bh->bh); > + else > + tasklet_schedule(&bh->bh); > } > EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb); > > @@ -2307,6 +2376,42 @@ EXPORT_SYMBOL_GPL (usb_hc_died); > > /*-------------------------------------------------------------------------*/ > > +static void __init_giveback_urb_bh(struct giveback_urb_bh *bh) > +{ > + > + spin_lock_init(&bh->lock); > + INIT_LIST_HEAD(&bh->head); > + tasklet_init(&bh->bh, usb_giveback_urb_bh, (unsigned long)bh); > +} > + > +static void init_giveback_urb_bh(struct usb_hcd *hcd) > +{ > + if (!hcd_giveback_urb_in_bh(hcd)) > + return; As you mentioned, there's not much point in skipping this code when it's not needed. In fact, you could just put the two lines below directly into usb_create_shared_hcd(), and get rid of the "__" from the beginning of the name. > + > + __init_giveback_urb_bh(&hcd->high_prio_bh); > + __init_giveback_urb_bh(&hcd->low_prio_bh); > +} > + > +static void exit_giveback_urb_bh(struct usb_hcd *hcd) > +{ > + if (!hcd_giveback_urb_in_bh(hcd)) > + return; > + > + /* > + * tasklet_kill() isn't needed here because: > + * - driver's disconnec() called from usb_disconnect() should Misspelled "disconnect". > + * make sure its URBs are completed during the disconnect() > + * callback > + * > + * - it is too late to run complete() here since driver may have > + * been removed already now > + * > + * Using tasklet to run URB's complete() doesn't change this > + * behavior of usbcore. > + */ > +} Why have a separate subroutine for doing nothing? Simply put this comment directly into usb_remove_hcd(). (And you can remove the last two lines of the comment; they don't make sense in this context.) Alan Stern
On Mon, Jun 24, 2013 at 11:17 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Mon, 24 Jun 2013, Ming Lei wrote: > >> This patch implements the mechanism of giveback of URB in >> tasklet context, so that hardware interrupt handling time for >> usb host controller can be saved much, and HCD interrupt handling >> can be simplified. > > Changes from v1 to v2? The change log is put in 0/4. > > >> +static void usb_giveback_urb_bh(unsigned long param) >> +{ >> + struct giveback_urb_bh *bh = (struct giveback_urb_bh *)param; >> + unsigned long flags; >> + struct list_head local_list; >> + >> + spin_lock_irqsave(&bh->lock, flags); >> + bh->running = 1; >> + restart: >> + list_replace_init(&bh->head, &local_list); >> + spin_unlock_irqrestore(&bh->lock, flags); > > Tasklet routines are always called with interrupts enabled, right? > Therefore you don't need to use the "flags" argument here or below. Good catch. > >> @@ -1667,25 +1721,40 @@ int usb_hcd_unlink_urb (struct urb *urb, int status) >> */ >> void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status) >> { >> - urb->hcpriv = NULL; >> - if (unlikely(urb->unlinked)) >> - status = urb->unlinked; >> - else if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) && >> - urb->actual_length < urb->transfer_buffer_length && >> - !status)) >> - status = -EREMOTEIO; >> + struct giveback_urb_bh *bh; >> + bool sched, high_prio_bh; >> >> - unmap_urb_for_dma(hcd, urb); >> - usbmon_urb_complete(&hcd->self, urb, status); >> - usb_unanchor_urb(urb); >> + /* pass status to tasklet via unlinked */ >> + if (likely(!urb->unlinked)) >> + urb->unlinked = status; >> >> - /* pass ownership to the completion handler */ >> - urb->status = status; >> - urb->complete (urb); >> - atomic_dec (&urb->use_count); >> - if (unlikely(atomic_read(&urb->reject))) >> - wake_up (&usb_kill_urb_queue); >> - usb_put_urb (urb); >> + if (!hcd_giveback_urb_in_bh(hcd) && !is_root_hub(urb->dev)) { >> + __usb_hcd_giveback_urb(urb); >> + return; >> + } >> + >> + if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe)) { >> + bh = &hcd->high_prio_bh; >> + high_prio_bh = 1; >> + } else { >> + bh = &hcd->low_prio_bh; >> + high_prio_bh = 0; >> + } > > Bool values should be assigned "true" or "false", not "1" or "0". Right. > >> + >> + spin_lock(&bh->lock); >> + list_add_tail(&urb->urb_list, &bh->head); >> + if (bh->running) >> + sched = 0; >> + else >> + sched = 1; >> + spin_unlock(&bh->lock); > > How about calling this variable "running" instead of "sched"? Then you > could just say: > > running = bh->running; > > with no "if" statement. OK, even we can do this below without name change: sched = !bh->running; > >> + >> + if (!sched) >> + ; >> + else if (high_prio_bh) >> + tasklet_hi_schedule(&bh->bh); >> + else >> + tasklet_schedule(&bh->bh); >> } >> EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb); >> >> @@ -2307,6 +2376,42 @@ EXPORT_SYMBOL_GPL (usb_hc_died); >> >> /*-------------------------------------------------------------------------*/ >> >> +static void __init_giveback_urb_bh(struct giveback_urb_bh *bh) >> +{ >> + >> + spin_lock_init(&bh->lock); >> + INIT_LIST_HEAD(&bh->head); >> + tasklet_init(&bh->bh, usb_giveback_urb_bh, (unsigned long)bh); >> +} >> + >> +static void init_giveback_urb_bh(struct usb_hcd *hcd) >> +{ >> + if (!hcd_giveback_urb_in_bh(hcd)) >> + return; > > As you mentioned, there's not much point in skipping this code when > it's not needed. In fact, you could just put the two lines below > directly into usb_create_shared_hcd(), and get rid of the "__" from the > beginning of the name. OK. > >> + >> + __init_giveback_urb_bh(&hcd->high_prio_bh); >> + __init_giveback_urb_bh(&hcd->low_prio_bh); >> +} >> + >> +static void exit_giveback_urb_bh(struct usb_hcd *hcd) >> +{ >> + if (!hcd_giveback_urb_in_bh(hcd)) >> + return; >> + >> + /* >> + * tasklet_kill() isn't needed here because: >> + * - driver's disconnec() called from usb_disconnect() should > > Misspelled "disconnect". > >> + * make sure its URBs are completed during the disconnect() >> + * callback >> + * >> + * - it is too late to run complete() here since driver may have >> + * been removed already now >> + * >> + * Using tasklet to run URB's complete() doesn't change this >> + * behavior of usbcore. >> + */ >> +} > > Why have a separate subroutine for doing nothing? Simply put this > comment directly into usb_remove_hcd(). (And you can remove the last > two lines of the comment; they don't make sense in this context.) Looks it is a bit clean to put the comment in the function, but it is OK to add them in usb_remove_hcd() too. Thanks again for your review. Thanks, -- Ming Lei
On Tue, 25 Jun 2013, Ming Lei wrote: > >> + > >> + spin_lock(&bh->lock); > >> + list_add_tail(&urb->urb_list, &bh->head); > >> + if (bh->running) > >> + sched = 0; > >> + else > >> + sched = 1; > >> + spin_unlock(&bh->lock); > > > > How about calling this variable "running" instead of "sched"? Then you > > could just say: > > > > running = bh->running; > > > > with no "if" statement. > > OK, even we can do this below without name change: > > sched = !bh->running; > > > > >> + > >> + if (!sched) > >> + ; > >> + else if (high_prio_bh) > >> + tasklet_hi_schedule(&bh->bh); > >> + else > >> + tasklet_schedule(&bh->bh); The advantage of "running" instead of "sched" is that it avoids a double negative: sched = !bh->running; ... if (!sched) ... as opposed to running = bh->running; ... if (running) ... Alan Stern
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index 014dc99..1fbd193 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -694,15 +694,7 @@ error: /* any errors get returned through the urb completion */ spin_lock_irq(&hcd_root_hub_lock); usb_hcd_unlink_urb_from_ep(hcd, urb); - - /* This peculiar use of spinlocks echoes what real HC drivers do. - * Avoiding calls to local_irq_disable/enable makes the code - * RT-friendly. - */ - spin_unlock(&hcd_root_hub_lock); usb_hcd_giveback_urb(hcd, urb, status); - spin_lock(&hcd_root_hub_lock); - spin_unlock_irq(&hcd_root_hub_lock); return 0; } @@ -742,9 +734,7 @@ void usb_hcd_poll_rh_status(struct usb_hcd *hcd) memcpy(urb->transfer_buffer, buffer, length); usb_hcd_unlink_urb_from_ep(hcd, urb); - spin_unlock(&hcd_root_hub_lock); usb_hcd_giveback_urb(hcd, urb, 0); - spin_lock(&hcd_root_hub_lock); } else { length = 0; set_bit(HCD_FLAG_POLL_PENDING, &hcd->flags); @@ -834,10 +824,7 @@ static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status) if (urb == hcd->status_urb) { hcd->status_urb = NULL; usb_hcd_unlink_urb_from_ep(hcd, urb); - - spin_unlock(&hcd_root_hub_lock); usb_hcd_giveback_urb(hcd, urb, status); - spin_lock(&hcd_root_hub_lock); } } done: @@ -1648,6 +1635,73 @@ int usb_hcd_unlink_urb (struct urb *urb, int status) /*-------------------------------------------------------------------------*/ +static void __usb_hcd_giveback_urb(struct urb *urb) +{ + struct usb_hcd *hcd = bus_to_hcd(urb->dev->bus); + int status = urb->unlinked; + unsigned long flags; + + urb->hcpriv = NULL; + if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) && + urb->actual_length < urb->transfer_buffer_length && + !status)) + status = -EREMOTEIO; + + unmap_urb_for_dma(hcd, urb); + usbmon_urb_complete(&hcd->self, urb, status); + usb_unanchor_urb(urb); + + /* pass ownership to the completion handler */ + urb->status = status; + + /* + * We disable local IRQs here avoid possible deadlock because + * drivers may call spin_lock() to hold lock which might be + * acquired in one hard interrupt handler. + * + * The local_irq_save()/local_irq_restore() around complete() + * will be removed if current USB drivers have been cleaned up + * and no one may trigger the above deadlock situation when + * running complete() in tasklet. + */ + local_irq_save(flags); + urb->complete(urb); + local_irq_restore(flags); + + atomic_dec(&urb->use_count); + if (unlikely(atomic_read(&urb->reject))) + wake_up(&usb_kill_urb_queue); + usb_put_urb(urb); +} + +static void usb_giveback_urb_bh(unsigned long param) +{ + struct giveback_urb_bh *bh = (struct giveback_urb_bh *)param; + unsigned long flags; + struct list_head local_list; + + spin_lock_irqsave(&bh->lock, flags); + bh->running = 1; + restart: + list_replace_init(&bh->head, &local_list); + spin_unlock_irqrestore(&bh->lock, flags); + + while (!list_empty(&local_list)) { + struct urb *urb; + + urb = list_entry(local_list.next, struct urb, urb_list); + list_del_init(&urb->urb_list); + __usb_hcd_giveback_urb(urb); + } + + /* check if there are new URBs to giveback */ + spin_lock_irqsave(&bh->lock, flags); + if (!list_empty(&bh->head)) + goto restart; + bh->running = 0; + spin_unlock_irqrestore(&bh->lock, flags); +} + /** * usb_hcd_giveback_urb - return URB from HCD to device driver * @hcd: host controller returning the URB @@ -1667,25 +1721,40 @@ int usb_hcd_unlink_urb (struct urb *urb, int status) */ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status) { - urb->hcpriv = NULL; - if (unlikely(urb->unlinked)) - status = urb->unlinked; - else if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) && - urb->actual_length < urb->transfer_buffer_length && - !status)) - status = -EREMOTEIO; + struct giveback_urb_bh *bh; + bool sched, high_prio_bh; - unmap_urb_for_dma(hcd, urb); - usbmon_urb_complete(&hcd->self, urb, status); - usb_unanchor_urb(urb); + /* pass status to tasklet via unlinked */ + if (likely(!urb->unlinked)) + urb->unlinked = status; - /* pass ownership to the completion handler */ - urb->status = status; - urb->complete (urb); - atomic_dec (&urb->use_count); - if (unlikely(atomic_read(&urb->reject))) - wake_up (&usb_kill_urb_queue); - usb_put_urb (urb); + if (!hcd_giveback_urb_in_bh(hcd) && !is_root_hub(urb->dev)) { + __usb_hcd_giveback_urb(urb); + return; + } + + if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe)) { + bh = &hcd->high_prio_bh; + high_prio_bh = 1; + } else { + bh = &hcd->low_prio_bh; + high_prio_bh = 0; + } + + spin_lock(&bh->lock); + list_add_tail(&urb->urb_list, &bh->head); + if (bh->running) + sched = 0; + else + sched = 1; + spin_unlock(&bh->lock); + + if (!sched) + ; + else if (high_prio_bh) + tasklet_hi_schedule(&bh->bh); + else + tasklet_schedule(&bh->bh); } EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb); @@ -2307,6 +2376,42 @@ EXPORT_SYMBOL_GPL (usb_hc_died); /*-------------------------------------------------------------------------*/ +static void __init_giveback_urb_bh(struct giveback_urb_bh *bh) +{ + + spin_lock_init(&bh->lock); + INIT_LIST_HEAD(&bh->head); + tasklet_init(&bh->bh, usb_giveback_urb_bh, (unsigned long)bh); +} + +static void init_giveback_urb_bh(struct usb_hcd *hcd) +{ + if (!hcd_giveback_urb_in_bh(hcd)) + return; + + __init_giveback_urb_bh(&hcd->high_prio_bh); + __init_giveback_urb_bh(&hcd->low_prio_bh); +} + +static void exit_giveback_urb_bh(struct usb_hcd *hcd) +{ + if (!hcd_giveback_urb_in_bh(hcd)) + return; + + /* + * tasklet_kill() isn't needed here because: + * - driver's disconnec() called from usb_disconnect() should + * make sure its URBs are completed during the disconnect() + * callback + * + * - it is too late to run complete() here since driver may have + * been removed already now + * + * Using tasklet to run URB's complete() doesn't change this + * behavior of usbcore. + */ +} + /** * usb_create_shared_hcd - create and initialize an HCD structure * @driver: HC driver that will use this hcd @@ -2573,6 +2678,8 @@ int usb_add_hcd(struct usb_hcd *hcd, && device_can_wakeup(&hcd->self.root_hub->dev)) dev_dbg(hcd->self.controller, "supports USB remote wakeup\n"); + init_giveback_urb_bh(hcd); + /* enable irqs just before we start the controller, * if the BIOS provides legacy PCI irqs. */ @@ -2681,6 +2788,8 @@ void usb_remove_hcd(struct usb_hcd *hcd) usb_disconnect(&rhdev); /* Sets rhdev to NULL */ mutex_unlock(&usb_bus_list_lock); + exit_giveback_urb_bh(hcd); + /* Prevent any more root-hub status calls from the timer. * The HCD might still restart the timer (if a port status change * interrupt occurs), but usb_hcd_poll_rh_status() won't invoke diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h index 1e88377..a9c7d44 100644 --- a/include/linux/usb/hcd.h +++ b/include/linux/usb/hcd.h @@ -22,6 +22,7 @@ #ifdef __KERNEL__ #include <linux/rwsem.h> +#include <linux/interrupt.h> #define MAX_TOPO_LEVEL 6 @@ -67,6 +68,13 @@ /*-------------------------------------------------------------------------*/ +struct giveback_urb_bh { + bool running; + spinlock_t lock; + struct list_head head; + struct tasklet_struct bh; +}; + struct usb_hcd { /* @@ -139,6 +147,9 @@ struct usb_hcd { resource_size_t rsrc_len; /* memory/io resource length */ unsigned power_budget; /* in mA, 0 = no limit */ + struct giveback_urb_bh high_prio_bh; + struct giveback_urb_bh low_prio_bh; + /* bandwidth_mutex should be taken before adding or removing * any new bus bandwidth constraints: * 1. Before adding a configuration for a new device. @@ -221,6 +232,7 @@ struct hc_driver { #define HCD_USB25 0x0030 /* Wireless USB 1.0 (USB 2.5)*/ #define HCD_USB3 0x0040 /* USB 3.0 */ #define HCD_MASK 0x0070 +#define HCD_BH 0x0100 /* URB complete in BH context */ /* called to init HCD and root hub */ int (*reset) (struct usb_hcd *hcd); @@ -361,6 +373,11 @@ struct hc_driver { int (*find_raw_port_number)(struct usb_hcd *, int); }; +static inline int hcd_giveback_urb_in_bh(struct usb_hcd *hcd) +{ + return hcd->driver->flags & HCD_BH; +} + extern int usb_hcd_link_urb_to_ep(struct usb_hcd *hcd, struct urb *urb); extern int usb_hcd_check_unlink_urb(struct usb_hcd *hcd, struct urb *urb, int status);
This patch implements the mechanism of giveback of URB in tasklet context, so that hardware interrupt handling time for usb host controller can be saved much, and HCD interrupt handling can be simplified. Motivations: 1), on some arch(such as ARM), DMA mapping/unmapping is a bit time-consuming, for example: when accessing usb mass storage via EHCI on pandaboard, the common length of transfer buffer is 120KB, the time consumed on DMA unmapping may reach hundreds of microseconds; even on A15 based box, the time is still about scores of microseconds 2), on some arch, reading DMA coherent memoery is very time-consuming, the most common example is usb video class driver[1] 3), driver's complete() callback may do much things which is driver specific, so the time is consumed unnecessarily in hardware irq context. 4), running driver's complete() callback in hardware irq context causes that host controller driver has to release its lock in interrupt handler, so reacquiring the lock after return may busy wait a while and increase interrupt handling time. More seriously, releasing the HCD lock makes HCD becoming quite complicated to deal with introduced races. So the patch proposes to run giveback of URB in tasklet context, then time consumed in HCD irq handling doesn't depend on drivers' complete and DMA mapping/unmapping any more, also we can simplify HCD since the HCD lock isn't needed to be released during irq handling. The patch should be reasonable and doable: 1), for drivers, they don't care if the complete() is called in hard irq context or softirq context 2), the biggest change is the situation in which usb_submit_urb() is called in complete() callback, so the introduced tasklet schedule delay might be a con, but it shouldn't be a big deal: - control/bulk asynchronous transfer isn't sensitive to schedule delay - the patch schedules giveback of periodic URBs using tasklet_hi_schedule, so the introduced delay should be very small - for ISOC transfer, generally, drivers submit several URBs concurrently to avoid interrupt delay, so it is OK with the little schedule delay. - for interrupt transfer, generally, drivers only submit one URB at the same time, but interrupt transfer is often used in event report, polling, ... situations, and a little delay should be OK. Considered that HCDs may optimize on submitting URB in complete(), the patch may cause the optimization not working, so introduces one flag to mark if the HCD supports to run giveback URB in tasklet context. When all HCDs are ready, the flag can be removed. [1], http://marc.info/?t=136438111600010&r=1&w=2 Cc: Oliver Neukum <oliver@neukum.org> Cc: Alan Stern <stern@rowland.harvard.edu> Signed-off-by: Ming Lei <ming.lei@canonical.com> --- drivers/usb/core/hcd.c | 169 ++++++++++++++++++++++++++++++++++++++--------- include/linux/usb/hcd.h | 17 +++++ 2 files changed, 156 insertions(+), 30 deletions(-)