Hi Sebastian, Em Fri, 16 Feb 2018 18:04:50 +0100 Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> escreveu: > I've been going over Frederic's softirq patches and it seems that there > were two problems. One was network related, the other was Mauro's USB > dvb-[stc] device which was not able to stream properly over time. > > Here is an attempt to let the URB complete in the threaded handler > instead of going to the tasklet. In case the system is SMP then the > patch [0] would be required in order to have the IRQ and its thread on > the same CPU. > > Mauro, would you mind giving it a shot? Sorry for taking some time to test it, has been busy those days... Anyway, I tested it today. Didn't work. It keep losing data. Regards, > > [0] genirq: Let irq thread follow the effective hard irq affinity > https://git.kernel.org/tip/cbf8699996a6e7f2f674b3a2a4cef9f666ff613e > > --- > > The urb->complete callback was initially invoked in IRQ context after > the HCD dropped its lock because the callback could re-queue the URB > again. Later this completion was deferred to the tasklet allowing the > HCD hold the lock. Also the BH handler can be interrupted by the IRQ > handler adding more "completed" requests to its queue. > While this batching is good in general, the softirq defers its doing for > short period of time if it is running constantly without a break. This > breaks some use cases where constant USB throughput is required. > As an alternative approach to tasklet handling, I defer the URB > completion to the HCD's threaded handler. There are two lists for > "high-prio" proccessing and lower priority (to mimic current behaviour). > The URBs in the high-priority list are always preffered over the URBs > in the low-priority list. > The URBs from the root-hub never create an interrupt so I currently > process them in a workqueue (I'm not sure if an URB-enqueue in the > completion handler would break something). > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> > --- > drivers/usb/core/hcd.c | 131 ++++++++++++++++++++++++++++++++---------------- > include/linux/usb/hcd.h | 14 +++--- > 2 files changed, 95 insertions(+), 50 deletions(-) > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index fc32391a34d5..8d6dd4d3cbe7 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -1775,33 +1775,75 @@ static void __usb_hcd_giveback_urb(struct urb *urb) > usb_put_urb(urb); > } > > -static void usb_giveback_urb_bh(unsigned long param) > +static void usb_hcd_rh_gb_urb(struct work_struct *work) > { > - struct giveback_urb_bh *bh = (struct giveback_urb_bh *)param; > - struct list_head local_list; > + struct giveback_urb *bh = container_of(work, struct giveback_urb, > + rh_compl); > + struct list_head urb_list; > > spin_lock_irq(&bh->lock); > - bh->running = true; > - restart: > - list_replace_init(&bh->head, &local_list); > + list_replace_init(&bh->rh_head, &urb_list); > spin_unlock_irq(&bh->lock); > > - while (!list_empty(&local_list)) { > + while (!list_empty(&urb_list)) { > struct urb *urb; > > - urb = list_entry(local_list.next, struct urb, urb_list); > + urb = list_first_entry(&urb_list, struct urb, urb_list); > list_del_init(&urb->urb_list); > - bh->completing_ep = urb->ep; > __usb_hcd_giveback_urb(urb); > - bh->completing_ep = NULL; > + } > +} > + > + > +#define URB_PRIO_HIGH 0 > +#define URB_PRIO_LOW 1 > + > +static irqreturn_t usb_hcd_gb_urb(int irq, void *__hcd) > +{ > + struct usb_hcd *hcd = __hcd; > + struct giveback_urb *bh = &hcd->gb_urb; > + struct list_head urb_list[2]; > + int i; > + > + INIT_LIST_HEAD(&urb_list[URB_PRIO_HIGH]); > + INIT_LIST_HEAD(&urb_list[URB_PRIO_LOW]); > + > + spin_lock_irq(&bh->lock); > + restart: > + list_splice_tail_init(&bh->prio_hi_head, &urb_list[URB_PRIO_HIGH]); > + list_splice_tail_init(&bh->prio_lo_head, &urb_list[URB_PRIO_LOW]); > + spin_unlock_irq(&bh->lock); > + > + for (i = 0; i < ARRAY_SIZE(urb_list); i++) { > + while (!list_empty(&urb_list[i])) { > + struct urb *urb; > + > + urb = list_first_entry(&urb_list[i], > + struct urb, urb_list); > + list_del_init(&urb->urb_list); > + if (i == URB_PRIO_HIGH) > + bh->completing_ep = urb->ep; > + > + __usb_hcd_giveback_urb(urb); > + > + if (i == URB_PRIO_HIGH) > + bh->completing_ep = NULL; > + > + if (i == URB_PRIO_LOW && > + !list_empty_careful(&urb_list[URB_PRIO_HIGH])) { > + spin_lock_irq(&bh->lock); > + goto restart; > + } > + } > } > > /* check if there are new URBs to giveback */ > spin_lock_irq(&bh->lock); > - if (!list_empty(&bh->head)) > + if (!list_empty(&bh->prio_hi_head) || > + !list_empty(&bh->prio_lo_head)) > goto restart; > - bh->running = false; > spin_unlock_irq(&bh->lock); > + return IRQ_HANDLED; > } > > /** > @@ -1823,37 +1865,32 @@ static void usb_giveback_urb_bh(unsigned long param) > */ > void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status) > { > - struct giveback_urb_bh *bh; > - bool running, high_prio_bh; > + struct giveback_urb *bh = &hcd->gb_urb; > + struct list_head *lh; > > /* pass status to tasklet via unlinked */ > if (likely(!urb->unlinked)) > urb->unlinked = status; > > - if (!hcd_giveback_urb_in_bh(hcd) && !is_root_hub(urb->dev)) { > + if (is_root_hub(urb->dev)) { > + spin_lock(&bh->lock); > + list_add_tail(&urb->urb_list, &bh->rh_head); > + spin_unlock(&bh->lock); > + queue_work(system_highpri_wq, &bh->rh_compl); > + return; > + } > + if (!hcd_giveback_urb_in_bh(hcd)) { > __usb_hcd_giveback_urb(urb); > return; > } > > - if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe)) { > - bh = &hcd->high_prio_bh; > - high_prio_bh = true; > - } else { > - bh = &hcd->low_prio_bh; > - high_prio_bh = false; > - } > - > - spin_lock(&bh->lock); > - list_add_tail(&urb->urb_list, &bh->head); > - running = bh->running; > - spin_unlock(&bh->lock); > - > - if (running) > - ; > - else if (high_prio_bh) > - tasklet_hi_schedule(&bh->bh); > + if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe)) > + lh = &bh->prio_hi_head; > else > - tasklet_schedule(&bh->bh); > + lh = &bh->prio_lo_head; > + spin_lock(&bh->lock); > + list_add_tail(&urb->urb_list, lh); > + spin_unlock(&bh->lock); > } > EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb); > > @@ -2436,9 +2473,17 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd) > rc = IRQ_NONE; > else if (hcd->driver->irq(hcd) == IRQ_NONE) > rc = IRQ_NONE; > - else > - rc = IRQ_HANDLED; > + else { > + struct giveback_urb *bh = &hcd->gb_urb; > > + spin_lock(&bh->lock); > + if (!list_empty(&bh->prio_hi_head) || > + !list_empty(&bh->prio_lo_head)) > + rc = IRQ_WAKE_THREAD; > + else > + rc = IRQ_HANDLED; > + spin_unlock(&bh->lock); > + } > return rc; > } > EXPORT_SYMBOL_GPL(usb_hcd_irq); > @@ -2492,12 +2537,13 @@ EXPORT_SYMBOL_GPL (usb_hc_died); > > /*-------------------------------------------------------------------------*/ > > -static void init_giveback_urb_bh(struct giveback_urb_bh *bh) > +static void init_giveback_urb(struct giveback_urb *bh) > { > - > spin_lock_init(&bh->lock); > - INIT_LIST_HEAD(&bh->head); > - tasklet_init(&bh->bh, usb_giveback_urb_bh, (unsigned long)bh); > + INIT_LIST_HEAD(&bh->prio_lo_head); > + INIT_LIST_HEAD(&bh->prio_hi_head); > + INIT_LIST_HEAD(&bh->rh_head); > + INIT_WORK(&bh->rh_compl, usb_hcd_rh_gb_urb); > } > > struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver, > @@ -2672,7 +2718,8 @@ static int usb_hcd_request_irqs(struct usb_hcd *hcd, > > snprintf(hcd->irq_descr, sizeof(hcd->irq_descr), "%s:usb%d", > hcd->driver->description, hcd->self.busnum); > - retval = request_irq(irqnum, &usb_hcd_irq, irqflags, > + retval = request_threaded_irq(irqnum, usb_hcd_irq, > + usb_hcd_gb_urb, irqflags, > hcd->irq_descr, hcd); > if (retval != 0) { > dev_err(hcd->self.controller, > @@ -2863,9 +2910,7 @@ 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"); > > - /* initialize tasklets */ > - init_giveback_urb_bh(&hcd->high_prio_bh); > - init_giveback_urb_bh(&hcd->low_prio_bh); > + init_giveback_urb(&hcd->gb_urb); > > /* enable irqs just before we start the controller, > * if the BIOS provides legacy PCI irqs. > diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h > index 176900528822..12573515acb6 100644 > --- a/include/linux/usb/hcd.h > +++ b/include/linux/usb/hcd.h > @@ -64,11 +64,12 @@ > > /*-------------------------------------------------------------------------*/ > > -struct giveback_urb_bh { > - bool running; > +struct giveback_urb { > spinlock_t lock; > - struct list_head head; > - struct tasklet_struct bh; > + struct list_head prio_lo_head; > + struct list_head prio_hi_head; > + struct list_head rh_head; > + struct work_struct rh_compl; > struct usb_host_endpoint *completing_ep; > }; > > @@ -169,8 +170,7 @@ 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; > + struct giveback_urb gb_urb; > > /* bandwidth_mutex should be taken before adding or removing > * any new bus bandwidth constraints: > @@ -410,7 +410,7 @@ static inline int hcd_giveback_urb_in_bh(struct usb_hcd *hcd) > static inline bool hcd_periodic_completion_in_progress(struct usb_hcd *hcd, > struct usb_host_endpoint *ep) > { > - return hcd->high_prio_bh.completing_ep == ep; > + return hcd->gb_urb.completing_ep == ep; > } > > extern int usb_hcd_link_urb_to_ep(struct usb_hcd *hcd, struct urb *urb); Thanks, Mauro -- 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