Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 27 Feb 2018 14:39:34 -0300
From:   Mauro Carvalho Chehab <mchehab@s-opensource.com>
To:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc:     Frederic Weisbecker <frederic@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Alan Stern <stern@rowland.harvard.edu>,
        linux-usb@vger.kernel.org
Subject: Re: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context
 instead of tasklet
Message-ID: <20180227143934.2aa847ac@vento.lan>
In-Reply-To: <20180216170450.yl5owfphuvltstnt@breakpoint.cc>
References: <20180216170450.yl5owfphuvltstnt@breakpoint.cc>
Organization: Samsung
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Sebastian,

Em Fri, 16 Feb 2018 18:04:50 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> 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@linutronix.de>
> ---
>  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