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

[PATCH] 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.

Fixes: 94dfd7edfd5c ("USB: HCD: support giveback of URB in tasklet context")
Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com>
---
 drivers/usb/core/hcd.c  | 24 ++++++++++++++----------
 include/linux/usb/hcd.h |  1 +
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 06eea8848ccc..149c045dfdc1 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1705,10 +1705,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->hi_priority)
+			tasklet_hi_schedule(&bh->bh);
+		else
+			tasklet_schedule(&bh->bh);
+	}
 	bh->running = false;
 	spin_unlock_irq(&bh->lock);
 }
@@ -1737,7 +1743,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 +1754,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 +1766,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->hi_priority)
 		tasklet_hi_schedule(&bh->bh);
 	else
 		tasklet_schedule(&bh->bh);
@@ -2959,6 +2962,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
 
 	/* initialize tasklets */
 	init_giveback_urb_bh(&hcd->high_prio_bh);
+	hcd->high_prio_bh.hi_priority = 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..13d67239c66c 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -66,6 +66,7 @@
 
 struct giveback_urb_bh {
 	bool running;
+	bool hi_priority;
 	spinlock_t lock;
 	struct list_head  head;
 	struct tasklet_struct bh;
-- 
2.32.0

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

[Oliver Neukum]

On 21.07.22 08:08, 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.
> 

Hi,

ow do you know usb_hcd_giveback_urb() will be called in time to process
isoc URBs in time, if you leave them on the list? In fact how do
you be sure it will be called at all? I can see no upper time limit
on that.

	Regards
		Oliver

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

[WeitaoWang-oc]

On 2022/7/21 16:00, Oliver Neukum wrote:
> 
> 
> On 21.07.22 08:08, 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.
>>
> 
> Hi,
> 
> ow do you know usb_hcd_giveback_urb() will be called in time to process
> isoc URBs in time, if you leave them on the list? In fact how do
> you be sure it will be called at all? I can see no upper time limit
> on that.
> 
> 	Regards
> 		Oliver
> 
> .
If the bh->head list is not empty, patch method will raise a softirq
immediately to call work function of every tasklet. Therefore, I think
the left URB in bh->head list will be get giveback in time.

Thanks
weitao

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

[kernel test robot]

Hi Weitao,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on linus/master v5.19-rc7 next-20220720]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Weitao-Wang/USB-HCD-Fix-URB-giveback-issue-in-tasklet-function/20220721-144208
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: hexagon-randconfig-r045-20220718 (https://download.01.org/0day-ci/archive/20220721/202207211752.TObbLhyX-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 0c1b32717bcffcf8edf95294e98933bd4c1e76ed)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/302398ba5a76bb39957bad7a6a8cb9d0429cd43a
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Weitao-Wang/USB-HCD-Fix-URB-giveback-issue-in-tasklet-function/20220721-144208
        git checkout 302398ba5a76bb39957bad7a6a8cb9d0429cd43a
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/usb/core/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/usb/core/hcd.c:1694:2: warning: unused label 'restart' [-Wunused-label]
    restart:
    ^~~~~~~~
   1 warning generated.


vim +/restart +1694 drivers/usb/core/hcd.c

94dfd7edfd5c9b Ming Lei    2013-07-03  1686  
e71ea55a5b6f91 Allen Pais  2020-08-17  1687  static void usb_giveback_urb_bh(struct tasklet_struct *t)
94dfd7edfd5c9b Ming Lei    2013-07-03  1688  {
e71ea55a5b6f91 Allen Pais  2020-08-17  1689  	struct giveback_urb_bh *bh = from_tasklet(bh, t, bh);
94dfd7edfd5c9b Ming Lei    2013-07-03  1690  	struct list_head local_list;
94dfd7edfd5c9b Ming Lei    2013-07-03  1691  
94dfd7edfd5c9b Ming Lei    2013-07-03  1692  	spin_lock_irq(&bh->lock);
94dfd7edfd5c9b Ming Lei    2013-07-03  1693  	bh->running = true;
94dfd7edfd5c9b Ming Lei    2013-07-03 @1694   restart:
94dfd7edfd5c9b Ming Lei    2013-07-03  1695  	list_replace_init(&bh->head, &local_list);
94dfd7edfd5c9b Ming Lei    2013-07-03  1696  	spin_unlock_irq(&bh->lock);
94dfd7edfd5c9b Ming Lei    2013-07-03  1697  
94dfd7edfd5c9b Ming Lei    2013-07-03  1698  	while (!list_empty(&local_list)) {
94dfd7edfd5c9b Ming Lei    2013-07-03  1699  		struct urb *urb;
94dfd7edfd5c9b Ming Lei    2013-07-03  1700  
94dfd7edfd5c9b Ming Lei    2013-07-03  1701  		urb = list_entry(local_list.next, struct urb, urb_list);
94dfd7edfd5c9b Ming Lei    2013-07-03  1702  		list_del_init(&urb->urb_list);
c7ccde6eac6d3c Alan Stern  2013-09-03  1703  		bh->completing_ep = urb->ep;
94dfd7edfd5c9b Ming Lei    2013-07-03  1704  		__usb_hcd_giveback_urb(urb);
c7ccde6eac6d3c Alan Stern  2013-09-03  1705  		bh->completing_ep = NULL;
94dfd7edfd5c9b Ming Lei    2013-07-03  1706  	}
94dfd7edfd5c9b Ming Lei    2013-07-03  1707  
302398ba5a76bb Weitao Wang 2022-07-21  1708  	/* giveback new URBs next time to prevent this function from
302398ba5a76bb Weitao Wang 2022-07-21  1709  	 * not exiting for a long time.
302398ba5a76bb Weitao Wang 2022-07-21  1710  	 */
94dfd7edfd5c9b Ming Lei    2013-07-03  1711  	spin_lock_irq(&bh->lock);
302398ba5a76bb Weitao Wang 2022-07-21  1712  	if (!list_empty(&bh->head)) {
302398ba5a76bb Weitao Wang 2022-07-21  1713  		if (bh->hi_priority)
302398ba5a76bb Weitao Wang 2022-07-21  1714  			tasklet_hi_schedule(&bh->bh);
302398ba5a76bb Weitao Wang 2022-07-21  1715  		else
302398ba5a76bb Weitao Wang 2022-07-21  1716  			tasklet_schedule(&bh->bh);
302398ba5a76bb Weitao Wang 2022-07-21  1717  	}
94dfd7edfd5c9b Ming Lei    2013-07-03  1718  	bh->running = false;
94dfd7edfd5c9b Ming Lei    2013-07-03  1719  	spin_unlock_irq(&bh->lock);
94dfd7edfd5c9b Ming Lei    2013-07-03  1720  }
94dfd7edfd5c9b Ming Lei    2013-07-03  1721  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[Alan Stern]

On Thu, Jul 21, 2022 at 02:08:33PM +0800, 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.

The patch also replaces the local high_prio_bh variable with a new
bh->high_prio structure member.  This should be mentioned in the patch 
description.

Alan Stern

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

[kernel test robot]

Hi Weitao,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on linus/master v5.19-rc7 next-20220721]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Weitao-Wang/USB-HCD-Fix-URB-giveback-issue-in-tasklet-function/20220721-144208
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: i386-defconfig (https://download.01.org/0day-ci/archive/20220721/202207212305.50JoL7V2-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/302398ba5a76bb39957bad7a6a8cb9d0429cd43a
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Weitao-Wang/USB-HCD-Fix-URB-giveback-issue-in-tasklet-function/20220721-144208
        git checkout 302398ba5a76bb39957bad7a6a8cb9d0429cd43a
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/usb/core/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/usb/core/hcd.c: In function 'usb_giveback_urb_bh':
>> drivers/usb/core/hcd.c:1694:2: warning: label 'restart' defined but not used [-Wunused-label]
    1694 |  restart:
         |  ^~~~~~~


vim +/restart +1694 drivers/usb/core/hcd.c

94dfd7edfd5c9b Ming Lei    2013-07-03  1686  
e71ea55a5b6f91 Allen Pais  2020-08-17  1687  static void usb_giveback_urb_bh(struct tasklet_struct *t)
94dfd7edfd5c9b Ming Lei    2013-07-03  1688  {
e71ea55a5b6f91 Allen Pais  2020-08-17  1689  	struct giveback_urb_bh *bh = from_tasklet(bh, t, bh);
94dfd7edfd5c9b Ming Lei    2013-07-03  1690  	struct list_head local_list;
94dfd7edfd5c9b Ming Lei    2013-07-03  1691  
94dfd7edfd5c9b Ming Lei    2013-07-03  1692  	spin_lock_irq(&bh->lock);
94dfd7edfd5c9b Ming Lei    2013-07-03  1693  	bh->running = true;
94dfd7edfd5c9b Ming Lei    2013-07-03 @1694   restart:
94dfd7edfd5c9b Ming Lei    2013-07-03  1695  	list_replace_init(&bh->head, &local_list);
94dfd7edfd5c9b Ming Lei    2013-07-03  1696  	spin_unlock_irq(&bh->lock);
94dfd7edfd5c9b Ming Lei    2013-07-03  1697  
94dfd7edfd5c9b Ming Lei    2013-07-03  1698  	while (!list_empty(&local_list)) {
94dfd7edfd5c9b Ming Lei    2013-07-03  1699  		struct urb *urb;
94dfd7edfd5c9b Ming Lei    2013-07-03  1700  
94dfd7edfd5c9b Ming Lei    2013-07-03  1701  		urb = list_entry(local_list.next, struct urb, urb_list);
94dfd7edfd5c9b Ming Lei    2013-07-03  1702  		list_del_init(&urb->urb_list);
c7ccde6eac6d3c Alan Stern  2013-09-03  1703  		bh->completing_ep = urb->ep;
94dfd7edfd5c9b Ming Lei    2013-07-03  1704  		__usb_hcd_giveback_urb(urb);
c7ccde6eac6d3c Alan Stern  2013-09-03  1705  		bh->completing_ep = NULL;
94dfd7edfd5c9b Ming Lei    2013-07-03  1706  	}
94dfd7edfd5c9b Ming Lei    2013-07-03  1707  
302398ba5a76bb Weitao Wang 2022-07-21  1708  	/* giveback new URBs next time to prevent this function from
302398ba5a76bb Weitao Wang 2022-07-21  1709  	 * not exiting for a long time.
302398ba5a76bb Weitao Wang 2022-07-21  1710  	 */
94dfd7edfd5c9b Ming Lei    2013-07-03  1711  	spin_lock_irq(&bh->lock);
302398ba5a76bb Weitao Wang 2022-07-21  1712  	if (!list_empty(&bh->head)) {
302398ba5a76bb Weitao Wang 2022-07-21  1713  		if (bh->hi_priority)
302398ba5a76bb Weitao Wang 2022-07-21  1714  			tasklet_hi_schedule(&bh->bh);
302398ba5a76bb Weitao Wang 2022-07-21  1715  		else
302398ba5a76bb Weitao Wang 2022-07-21  1716  			tasklet_schedule(&bh->bh);
302398ba5a76bb Weitao Wang 2022-07-21  1717  	}
94dfd7edfd5c9b Ming Lei    2013-07-03  1718  	bh->running = false;
94dfd7edfd5c9b Ming Lei    2013-07-03  1719  	spin_unlock_irq(&bh->lock);
94dfd7edfd5c9b Ming Lei    2013-07-03  1720  }
94dfd7edfd5c9b Ming Lei    2013-07-03  1721  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[WeitaoWang-oc]

On 2022/7/21 21:50, Alan Stern wrote:
> On Thu, Jul 21, 2022 at 02:08:33PM +0800, 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.
> 
> The patch also replaces the local high_prio_bh variable with a new
> bh->high_prio structure member.  This should be mentioned in the patch
> description.
> 
> Alan Stern
> .
Okay,I will take your suggestion and submit a revised version.

Thanks
weitao