From: Alan Stern <stern@rowland.harvard.edu>
To: Bin Liu <b-liu@ti.com>
Cc: "Matwey V. Kornilov" <matwey@sai.msu.ru>,
gregkh@linuxfoundation.org, matwey.kornilov@gmail.com,
linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
Subject: [6/6] usb: musb: Decrease URB starting latency in musb_advance_schedule()
Date: Tue, 30 Apr 2019 13:29:35 -0400 (EDT) [thread overview]
Message-ID: <Pine.LNX.4.44L0.1904301323050.1465-100000@iolanthe.rowland.org> (raw)
On Tue, 30 Apr 2019, Bin Liu wrote:
> Hi Greg and all devs,
>
> On Wed, Apr 03, 2019 at 09:53:10PM +0300, Matwey V. Kornilov wrote:
> > Previously, the algorithm was the following:
> >
> > 1. giveback current URB
> > 2. if current qh is not empty
> > then start next URB
> > 3. if current qh is empty
> > then dispose the qh, find next qh if any, and start URB.
> >
> > It may take a while to run urb->callback inside URB giveback which is
> > run synchronously in musb. In order to improve the latency we rearrange
> > the function behaviour for the case when qh is not empty: next URB is
> > started before URB giveback. When qh is empty then the behaviour is
> > intentionally kept in order not to break existing inter qh scheduling:
> > URB giveback could potentionally enqueue other URB to the empty qh
> > preventing it from being disposed.
>
> This patch changes the sequence of urb giveback in musb.
>
> before after
> ------ -----
> 1. giveback current urb 1. start next urb if qh != empty
> 2. start next urb if qh != empty 2. giveback current urb
>
> I see there is a potential that the urb giveback could be out of order,
> for example, if urb giveback in BH and the next urb finishes before BH
> runs.
>
> If this potential is possible, is it a problem for any class driver?
I don't know of any specific examples where this would be a problem.
But it definitely goes against the guarantee that except for unlinks,
URBs for each endpoint are always given back in order.
There's also a guarantee that when an URB has an error status, it
causes the endpoint queue to stop. This is necessary so that the class
driver can cancel any outstanding URBs before they run and cause even
more trouble. Your brief outline above doesn't mention this.
On the other hand, it shouldn't be hard to maintain the order even
here. For example, you could have a FIFO list of URBs waiting to be
given back, and the BH could always give back the URB at the front of
the list.
Alan Stern
WARNING: multiple messages have this Message-ID (diff)
From: Alan Stern <stern@rowland.harvard.edu>
To: Bin Liu <b-liu@ti.com>
Cc: "Matwey V. Kornilov" <matwey@sai.msu.ru>,
<gregkh@linuxfoundation.org>, <matwey.kornilov@gmail.com>,
<linux-kernel@vger.kernel.org>, <linux-usb@vger.kernel.org>
Subject: Re: [PATCH 6/6] usb: musb: Decrease URB starting latency in musb_advance_schedule()
Date: Tue, 30 Apr 2019 13:29:35 -0400 (EDT) [thread overview]
Message-ID: <Pine.LNX.4.44L0.1904301323050.1465-100000@iolanthe.rowland.org> (raw)
Message-ID: <20190430172935.2qjW2yinKlhX5jObbXAY9Puv8KuGJtaT_hKorUdYOGk@z> (raw)
In-Reply-To: <20190430153118.GI20993@uda0271908>
On Tue, 30 Apr 2019, Bin Liu wrote:
> Hi Greg and all devs,
>
> On Wed, Apr 03, 2019 at 09:53:10PM +0300, Matwey V. Kornilov wrote:
> > Previously, the algorithm was the following:
> >
> > 1. giveback current URB
> > 2. if current qh is not empty
> > then start next URB
> > 3. if current qh is empty
> > then dispose the qh, find next qh if any, and start URB.
> >
> > It may take a while to run urb->callback inside URB giveback which is
> > run synchronously in musb. In order to improve the latency we rearrange
> > the function behaviour for the case when qh is not empty: next URB is
> > started before URB giveback. When qh is empty then the behaviour is
> > intentionally kept in order not to break existing inter qh scheduling:
> > URB giveback could potentionally enqueue other URB to the empty qh
> > preventing it from being disposed.
>
> This patch changes the sequence of urb giveback in musb.
>
> before after
> ------ -----
> 1. giveback current urb 1. start next urb if qh != empty
> 2. start next urb if qh != empty 2. giveback current urb
>
> I see there is a potential that the urb giveback could be out of order,
> for example, if urb giveback in BH and the next urb finishes before BH
> runs.
>
> If this potential is possible, is it a problem for any class driver?
I don't know of any specific examples where this would be a problem.
But it definitely goes against the guarantee that except for unlinks,
URBs for each endpoint are always given back in order.
There's also a guarantee that when an URB has an error status, it
causes the endpoint queue to stop. This is necessary so that the class
driver can cancel any outstanding URBs before they run and cause even
more trouble. Your brief outline above doesn't mention this.
On the other hand, it shouldn't be hard to maintain the order even
here. For example, you could have a FIFO list of URBs waiting to be
given back, and the BH could always give back the URB at the front of
the list.
Alan Stern
next prev reply other threads:[~2019-04-30 17:29 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20190403185310.8437-1-matwey@sai.msu.ru>
2019-04-03 18:53 ` [6/6] usb: musb: Decrease URB starting latency in musb_advance_schedule() Matwey V. Kornilov
2019-04-30 15:31 ` Bin Liu
2019-04-30 15:31 ` [PATCH 6/6] " Bin Liu
2019-04-30 17:29 ` Alan Stern [this message]
2019-04-30 17:29 ` Alan Stern
2019-05-04 9:38 ` [6/6] " Matwey V. Kornilov
2019-05-04 9:38 ` [PATCH 6/6] " Matwey V. Kornilov
2019-04-24 15:42 ` [PATCH 0/6] musb: Improve performance for hub-attached webcams Matwey V. Kornilov
2019-04-30 15:20 ` Bin Liu
2019-06-14 16:45 ` [PATCH v2 " Matwey V. Kornilov
2019-06-14 16:45 ` [PATCH v2 1/6] usb: musb: Use USB_DIR_IN when calling musb_advance_schedule() Matwey V. Kornilov
2019-06-14 16:45 ` [PATCH v2 2/6] usb: musb: Introduce musb_qh_empty() helper function Matwey V. Kornilov
2019-06-14 16:45 ` [PATCH v2 3/6] usb: musb: Introduce musb_qh_free() " Matwey V. Kornilov
2019-06-14 16:45 ` [PATCH v2 4/6] usb: musb: Rename musb_start_urb() to musb_start_next_urb() Matwey V. Kornilov
2019-06-14 16:45 ` [PATCH v2 5/6] usb: musb: Introduce musb_start_urb() Matwey V. Kornilov
2019-06-14 16:45 ` [PATCH v2 6/6] usb: musb: Decrease URB starting latency in musb_advance_schedule() Matwey V. Kornilov
2019-07-02 17:29 ` [PATCH v2 0/6] musb: Improve performance for hub-attached webcams Matwey V. Kornilov
2019-07-02 17:33 ` Bin Liu
2019-09-09 16:33 ` Matwey V. Kornilov
2019-10-23 8:12 ` Matwey V. Kornilov
2020-01-01 16:26 ` [PATCH RESEND " Matwey V. Kornilov
2020-01-01 16:26 ` [PATCH RESEND v2 1/6] usb: musb: Use USB_DIR_IN when calling musb_advance_schedule() Matwey V. Kornilov
2020-01-01 16:26 ` [PATCH RESEND v2 2/6] usb: musb: Introduce musb_qh_empty() helper function Matwey V. Kornilov
2020-01-01 16:26 ` [PATCH RESEND v2 3/6] usb: musb: Introduce musb_qh_free() " Matwey V. Kornilov
2020-01-01 16:26 ` [PATCH RESEND v2 4/6] usb: musb: Rename musb_start_urb() to musb_start_next_urb() Matwey V. Kornilov
2020-01-01 16:26 ` [PATCH RESEND v2 5/6] usb: musb: Introduce musb_start_urb() Matwey V. Kornilov
2020-01-01 16:26 ` [PATCH RESEND v2 6/6] usb: musb: Decrease URB starting latency in musb_advance_schedule() Matwey V. Kornilov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Pine.LNX.4.44L0.1904301323050.1465-100000@iolanthe.rowland.org \
--to=stern@rowland.harvard.edu \
--cc=b-liu@ti.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=matwey.kornilov@gmail.com \
--cc=matwey@sai.msu.ru \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).