| Message ID | 20220721060833.4173-1-WeitaoWang-oc@zhaoxin.com |
|---|---|
| State | New |
| Headers | show |
| Series | USB: HCD: Fix URB giveback issue in tasklet function | expand |
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
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
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
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;
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(-)