[PATCH v2] USB: HCD: Fix URB giveback issue in tasklet function

[PATCH v2] USB: HCD: Fix URB giveback issue in tasklet function

[Weitao Wang]

Usb core introduce the mechanism of giveback of URB in tasklet context to
reduce hardware interrupt handling time. On some test situation(such as
FIO with 4KB block size), when tasklet callback function called to
giveback URB, interrupt handler add URB node to the bh->head list also.
If check bh->head list again after finish all URB giveback of local_list,
then it may introduce a "dynamic balance" between giveback URB and add URB
to bh->head list. This tasklet callback function may not exit for a long
time, which will cause other tasklet function calls to be delayed. Some
real-time applications(such as KB and Mouse) will see noticeable lag.

Fix this issue by taking new URBs giveback in next tasklet function call.
Add a member high_prio for structure giveback_urb_bh and replace the local
high_prio_bh variable with this structure member in usb_hcd_giveback_urb.

Fixes: 94dfd7edfd5c ("USB: HCD: support giveback of URB in tasklet context")
Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>
---
v1->v2:
 - Fix compile warning by remove label "restart".
 - Modify the patch description info.
 - Change structure member from hi_priority to high_prio.

 drivers/usb/core/hcd.c  | 25 ++++++++++++++-----------
 include/linux/usb/hcd.h |  1 +
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 06eea8848ccc..1feb9a604380 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1691,7 +1691,6 @@ static void usb_giveback_urb_bh(struct tasklet_struct *t)
 
 	spin_lock_irq(&bh->lock);
 	bh->running = true;
- restart:
 	list_replace_init(&bh->head, &local_list);
 	spin_unlock_irq(&bh->lock);
 
@@ -1705,10 +1704,16 @@ static void usb_giveback_urb_bh(struct tasklet_struct *t)
 		bh->completing_ep = NULL;
 	}
 
-	/* check if there are new URBs to giveback */
+	/* giveback new URBs next time to prevent this function from
+	 * not exiting for a long time.
+	 */
 	spin_lock_irq(&bh->lock);
-	if (!list_empty(&bh->head))
-		goto restart;
+	if (!list_empty(&bh->head)) {
+		if (bh->high_prio)
+			tasklet_hi_schedule(&bh->bh);
+		else
+			tasklet_schedule(&bh->bh);
+	}
 	bh->running = false;
 	spin_unlock_irq(&bh->lock);
 }
@@ -1737,7 +1742,7 @@ static void usb_giveback_urb_bh(struct tasklet_struct *t)
 void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
 {
 	struct giveback_urb_bh *bh;
-	bool running, high_prio_bh;
+	bool running;
 
 	/* pass status to tasklet via unlinked */
 	if (likely(!urb->unlinked))
@@ -1748,13 +1753,10 @@ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
 		return;
 	}
 
-	if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe)) {
+	if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe))
 		bh = &hcd->high_prio_bh;
-		high_prio_bh = true;
-	} else {
+	else
 		bh = &hcd->low_prio_bh;
-		high_prio_bh = false;
-	}
 
 	spin_lock(&bh->lock);
 	list_add_tail(&urb->urb_list, &bh->head);
@@ -1763,7 +1765,7 @@ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
 
 	if (running)
 		;
-	else if (high_prio_bh)
+	else if (bh->high_prio)
 		tasklet_hi_schedule(&bh->bh);
 	else
 		tasklet_schedule(&bh->bh);
@@ -2959,6 +2961,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
 
 	/* initialize tasklets */
 	init_giveback_urb_bh(&hcd->high_prio_bh);
+	hcd->high_prio_bh.high_prio = 1;
 	init_giveback_urb_bh(&hcd->low_prio_bh);
 
 	/* enable irqs just before we start the controller,
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 2c1fc9212cf2..98d1921f02b1 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -66,6 +66,7 @@
 
 struct giveback_urb_bh {
 	bool running;
+	bool high_prio;
 	spinlock_t lock;
 	struct list_head  head;
 	struct tasklet_struct bh;
-- 
2.32.0

Re: [PATCH v2] USB: HCD: Fix URB giveback issue in tasklet function

[Sergei Shtylyov]

Hello!

On 7/25/22 9:52 AM, Weitao Wang wrote:

> Usb core introduce the mechanism of giveback of URB in tasklet context to
> reduce hardware interrupt handling time. On some test situation(such as
> FIO with 4KB block size), when tasklet callback function called to
> giveback URB, interrupt handler add URB node to the bh->head list also.
> If check bh->head list again after finish all URB giveback of local_list,
> then it may introduce a "dynamic balance" between giveback URB and add URB
> to bh->head list. This tasklet callback function may not exit for a long
> time, which will cause other tasklet function calls to be delayed. Some
> real-time applications(such as KB and Mouse) will see noticeable lag.
> 
> Fix this issue by taking new URBs giveback in next tasklet function call.
> Add a member high_prio for structure giveback_urb_bh and replace the local
> high_prio_bh variable with this structure member in usb_hcd_giveback_urb.
> 
> Fixes: 94dfd7edfd5c ("USB: HCD: support giveback of URB in tasklet context")
> Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>
> ---
> v1->v2:
>  - Fix compile warning by remove label "restart".
>  - Modify the patch description info.
>  - Change structure member from hi_priority to high_prio.
> 
>  drivers/usb/core/hcd.c  | 25 ++++++++++++++-----------
>  include/linux/usb/hcd.h |  1 +
>  2 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 06eea8848ccc..1feb9a604380 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
[...]
> @@ -2959,6 +2961,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  
>  	/* initialize tasklets */
>  	init_giveback_urb_bh(&hcd->high_prio_bh);
> +	hcd->high_prio_bh.high_prio = 1;

   s/1/true/?

>  	init_giveback_urb_bh(&hcd->low_prio_bh);
>  
>  	/* enable irqs just before we start the controller,
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index 2c1fc9212cf2..98d1921f02b1 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -66,6 +66,7 @@
>  
>  struct giveback_urb_bh {
>  	bool running;
> +	bool high_prio;
>  	spinlock_t lock;
>  	struct list_head  head;
>  	struct tasklet_struct bh;

MBR, Sergey

Re: [PATCH v2] USB: HCD: Fix URB giveback issue in tasklet function

[WeitaoWang-oc]

On 2022/7/25 16:29, Sergei Shtylyov wrote:
> Hello!
> 
> On 7/25/22 9:52 AM, Weitao Wang wrote:
> 
>> Usb core introduce the mechanism of giveback of URB in tasklet context to
>> reduce hardware interrupt handling time. On some test situation(such as
>> FIO with 4KB block size), when tasklet callback function called to
>> giveback URB, interrupt handler add URB node to the bh->head list also.
>> If check bh->head list again after finish all URB giveback of local_list,
>> then it may introduce a "dynamic balance" between giveback URB and add URB
>> to bh->head list. This tasklet callback function may not exit for a long
>> time, which will cause other tasklet function calls to be delayed. Some
>> real-time applications(such as KB and Mouse) will see noticeable lag.
>>
>> Fix this issue by taking new URBs giveback in next tasklet function call.
>> Add a member high_prio for structure giveback_urb_bh and replace the local
>> high_prio_bh variable with this structure member in usb_hcd_giveback_urb.
>>
>> Fixes: 94dfd7edfd5c ("USB: HCD: support giveback of URB in tasklet context")
>> Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>
>> ---
>> v1->v2:
>>   - Fix compile warning by remove label "restart".
>>   - Modify the patch description info.
>>   - Change structure member from hi_priority to high_prio.
>>
>>   drivers/usb/core/hcd.c  | 25 ++++++++++++++-----------
>>   include/linux/usb/hcd.h |  1 +
>>   2 files changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 06eea8848ccc..1feb9a604380 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
> [...]
>> @@ -2959,6 +2961,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>>   
>>   	/* initialize tasklets */
>>   	init_giveback_urb_bh(&hcd->high_prio_bh);
>> +	hcd->high_prio_bh.high_prio = 1;
> 
>     s/1/true/?
Okay,this Boolean variable should be initialized to ture.
Thanks for your suggestion.

weitao
>>   	init_giveback_urb_bh(&k->low_prio_bh);
>>   
>>   	/* enable irqs just before we start the controller,
>> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
>> index 2c1fc9212cf2..98d1921f02b1 100644
>> --- a/include/linux/usb/hcd.h
>> +++ b/include/linux/usb/hcd.h
>> @@ -66,6 +66,7 @@
>>   
>>   struct giveback_urb_bh {
>>   	bool running;
>> +	bool high_prio;
>>   	spinlock_t lock;
>>   	struct list_head  head;
>>   	struct tasklet_struct bh;
> 
> MBR, Sergey
> .

Re: [PATCH v2] USB: HCD: Fix URB giveback issue in tasklet function

[Alan Stern]

On Mon, Jul 25, 2022 at 02:52:51PM +0800, Weitao Wang wrote:

This is basically okay.  Just a couple of small comments...

> Usb core introduce the mechanism of giveback of URB in tasklet context to
> reduce hardware interrupt handling time. On some test situation(such as
> FIO with 4KB block size), when tasklet callback function called to
> giveback URB, interrupt handler add URB node to the bh->head list also.
> If check bh->head list again after finish all URB giveback of local_list,
> then it may introduce a "dynamic balance" between giveback URB and add URB
> to bh->head list. This tasklet callback function may not exit for a long
> time, which will cause other tasklet function calls to be delayed. Some
> real-time applications(such as KB and Mouse) will see noticeable lag.
> 
> Fix this issue by taking new URBs giveback in next tasklet function call.
> Add a member high_prio for structure giveback_urb_bh and replace the local
> high_prio_bh variable with this structure member in usb_hcd_giveback_urb.

The patch description should do more than say what the new code _is_ -- 
we can see that easily enough by reading the patch.  The description 
should explain _why_ the code was changed.

> Fixes: 94dfd7edfd5c ("USB: HCD: support giveback of URB in tasklet context")
> Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>
> ---
> v1->v2:
>  - Fix compile warning by remove label "restart".
>  - Modify the patch description info.
>  - Change structure member from hi_priority to high_prio.
> 
>  drivers/usb/core/hcd.c  | 25 ++++++++++++++-----------
>  include/linux/usb/hcd.h |  1 +
>  2 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 06eea8848ccc..1feb9a604380 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1691,7 +1691,6 @@ static void usb_giveback_urb_bh(struct tasklet_struct *t)
>  
>  	spin_lock_irq(&bh->lock);
>  	bh->running = true;
> - restart:
>  	list_replace_init(&bh->head, &local_list);
>  	spin_unlock_irq(&bh->lock);
>  
> @@ -1705,10 +1704,16 @@ static void usb_giveback_urb_bh(struct tasklet_struct *t)
>  		bh->completing_ep = NULL;
>  	}
>  
> -	/* check if there are new URBs to giveback */
> +	/* giveback new URBs next time to prevent this function from
> +	 * not exiting for a long time.
> +	 */

Minor stylistic issue: The currently accepted format for multi-line 
comments is like this:

	/*
	 * Blah blah blah
	 * Blah blah blah
	 */

Alan Stern

Re: [PATCH v2] USB: HCD: Fix URB giveback issue in tasklet function

[WeitaoWang-oc]

On 2022/7/25 21:58, Alan Stern wrote:
> On Mon, Jul 25, 2022 at 02:52:51PM +0800, Weitao Wang wrote:
> 
> This is basically okay.  Just a couple of small comments...
> 
>> Usb core introduce the mechanism of giveback of URB in tasklet context to
>> reduce hardware interrupt handling time. On some test situation(such as
>> FIO with 4KB block size), when tasklet callback function called to
>> giveback URB, interrupt handler add URB node to the bh->head list also.
>> If check bh->head list again after finish all URB giveback of local_list,
>> then it may introduce a "dynamic balance" between giveback URB and add URB
>> to bh->head list. This tasklet callback function may not exit for a long
>> time, which will cause other tasklet function calls to be delayed. Some
>> real-time applications(such as KB and Mouse) will see noticeable lag.
>>
>> Fix this issue by taking new URBs giveback in next tasklet function call.
>> Add a member high_prio for structure giveback_urb_bh and replace the local
>> high_prio_bh variable with this structure member in usb_hcd_giveback_urb.
> 
> The patch description should do more than say what the new code _is_ --
> we can see that easily enough by reading the patch.  The description
> should explain _why_ the code was changed.
> 
Okay, I will improve this patch description.

>> -	/* check if there are new URBs to giveback */
>> +	/* giveback new URBs next time to prevent this function from
>> +	 * not exiting for a long time.
>> +	 */
> 
> Minor stylistic issue: The currently accepted format for multi-line
> comments is like this:
> 
This notes will be improved in next patch version also.
Thanks for your suggestion.

weitao
> 	/*
> 	 * Blah blah blah
> 	 * Blah blah blah
> 	 */
> 
> Alan Stern
> .