| Message ID | alpine.LRH.2.02.1806121020130.10769@file01.intranet.prod.int.rdu2.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 12 Jun 2018, Mikulas Patocka wrote: > I have a single-core machine with usb2 soundcard. When I increase mplayer > priority (to real-time or high non-realtime priority), the sound is > stuttering. The reason for the stuttering is that mplayer at high priority > preempts the softirq thread, preventing URBs from being completed. It was > caused by the patch 428aac8a81058 that offloads URB completion to softirq. > > This patch prevents offloading isochronous URBs to softirq to fix the > stuttering. How about just not running mplayer at such a high priority? Or raising the priority of the softirq thread? Alan Stern > > Fixes: c04ee4b1136e ("Revert "Revert "USB: EHCI: support running URB giveback in tasklet context""") > Cc: stable@vger.kernel.org > > --- > drivers/usb/core/hcd.c | 10 ++++++++++ > drivers/usb/host/ehci-q.c | 11 ++++++++++- > include/linux/usb/hcd.h | 2 ++ > 3 files changed, 22 insertions(+), 1 deletion(-) > > Index: linux-4.17/drivers/usb/core/hcd.c > =================================================================== > --- linux-4.17.orig/drivers/usb/core/hcd.c 2018-06-12 16:06:23.000000000 +0200 > +++ linux-4.17/drivers/usb/core/hcd.c 2018-06-12 16:07:51.000000000 +0200 > @@ -1858,6 +1858,16 @@ void usb_hcd_giveback_urb(struct usb_hcd > } > EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb); > > +void _usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status) > +{ > + /* pass status to tasklet via unlinked */ > + if (likely(!urb->unlinked)) > + urb->unlinked = status; > + > + __usb_hcd_giveback_urb(urb); > +} > +EXPORT_SYMBOL_GPL(_usb_hcd_giveback_urb); > + > /*-------------------------------------------------------------------------*/ > > /* Cancel all URBs pending on this endpoint and wait for the endpoint's > Index: linux-4.17/drivers/usb/host/ehci-q.c > =================================================================== > --- linux-4.17.orig/drivers/usb/host/ehci-q.c 2018-06-12 16:06:23.000000000 +0200 > +++ linux-4.17/drivers/usb/host/ehci-q.c 2018-06-12 16:09:28.000000000 +0200 > @@ -238,6 +238,8 @@ static int qtd_copy_status ( > > static void > ehci_urb_done(struct ehci_hcd *ehci, struct urb *urb, int status) > +__releases(ehci->lock) > +__acquires(ehci->lock) > { > if (usb_pipetype(urb->pipe) == PIPE_INTERRUPT) { > /* ... update hc-wide periodic stats */ > @@ -264,7 +266,14 @@ ehci_urb_done(struct ehci_hcd *ehci, str > #endif > > usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb); > - usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status); > + if (usb_pipeisoc(urb->pipe)) { > + /* complete() can reenter this HCD */ > + spin_unlock(&ehci->lock); > + _usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status); > + spin_lock(&ehci->lock); > + } else { > + usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status); > + } > } > > static int qh_schedule (struct ehci_hcd *ehci, struct ehci_qh *qh); > Index: linux-4.17/include/linux/usb/hcd.h > =================================================================== > --- linux-4.17.orig/include/linux/usb/hcd.h 2018-06-05 21:07:27.000000000 +0200 > +++ linux-4.17/include/linux/usb/hcd.h 2018-06-12 16:07:11.000000000 +0200 > @@ -428,6 +428,8 @@ extern int usb_hcd_submit_urb(struct urb > extern int usb_hcd_unlink_urb(struct urb *urb, int status); > extern void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, > int status); > +extern void _usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, > + int status); > extern int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, > gfp_t mem_flags); > extern void usb_hcd_unmap_urb_setup_for_dma(struct usb_hcd *, struct urb *); > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 12 Jun 2018, Alan Stern wrote: > On Tue, 12 Jun 2018, Mikulas Patocka wrote: > > > I have a single-core machine with usb2 soundcard. When I increase mplayer > > priority (to real-time or high non-realtime priority), the sound is > > stuttering. The reason for the stuttering is that mplayer at high priority > > preempts the softirq thread, preventing URBs from being completed. It was > > caused by the patch 428aac8a81058 that offloads URB completion to softirq. > > > > This patch prevents offloading isochronous URBs to softirq to fix the > > stuttering. > > How about just not running mplayer at such a high priority? I need to run mplayer at a high priority so that other work doesn't preempt mplayer and cause stuttering. > Or raising the priority of the softirq thread? Do you want to coordinate that with the softirq maintainers? I don't know if they would be happy to add an extra real-time softirq thread. > Alan Stern Mikulas > > Fixes: c04ee4b1136e ("Revert "Revert "USB: EHCI: support running URB giveback in tasklet context""") > > Cc: stable@vger.kernel.org > > > > --- > > drivers/usb/core/hcd.c | 10 ++++++++++ > > drivers/usb/host/ehci-q.c | 11 ++++++++++- > > include/linux/usb/hcd.h | 2 ++ > > 3 files changed, 22 insertions(+), 1 deletion(-) > > > > Index: linux-4.17/drivers/usb/core/hcd.c > > =================================================================== > > --- linux-4.17.orig/drivers/usb/core/hcd.c 2018-06-12 16:06:23.000000000 +0200 > > +++ linux-4.17/drivers/usb/core/hcd.c 2018-06-12 16:07:51.000000000 +0200 > > @@ -1858,6 +1858,16 @@ void usb_hcd_giveback_urb(struct usb_hcd > > } > > EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb); > > > > +void _usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status) > > +{ > > + /* pass status to tasklet via unlinked */ > > + if (likely(!urb->unlinked)) > > + urb->unlinked = status; > > + > > + __usb_hcd_giveback_urb(urb); > > +} > > +EXPORT_SYMBOL_GPL(_usb_hcd_giveback_urb); > > + > > /*-------------------------------------------------------------------------*/ > > > > /* Cancel all URBs pending on this endpoint and wait for the endpoint's > > Index: linux-4.17/drivers/usb/host/ehci-q.c > > =================================================================== > > --- linux-4.17.orig/drivers/usb/host/ehci-q.c 2018-06-12 16:06:23.000000000 +0200 > > +++ linux-4.17/drivers/usb/host/ehci-q.c 2018-06-12 16:09:28.000000000 +0200 > > @@ -238,6 +238,8 @@ static int qtd_copy_status ( > > > > static void > > ehci_urb_done(struct ehci_hcd *ehci, struct urb *urb, int status) > > +__releases(ehci->lock) > > +__acquires(ehci->lock) > > { > > if (usb_pipetype(urb->pipe) == PIPE_INTERRUPT) { > > /* ... update hc-wide periodic stats */ > > @@ -264,7 +266,14 @@ ehci_urb_done(struct ehci_hcd *ehci, str > > #endif > > > > usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb); > > - usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status); > > + if (usb_pipeisoc(urb->pipe)) { > > + /* complete() can reenter this HCD */ > > + spin_unlock(&ehci->lock); > > + _usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status); > > + spin_lock(&ehci->lock); > > + } else { > > + usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status); > > + } > > } > > > > static int qh_schedule (struct ehci_hcd *ehci, struct ehci_qh *qh); > > Index: linux-4.17/include/linux/usb/hcd.h > > =================================================================== > > --- linux-4.17.orig/include/linux/usb/hcd.h 2018-06-05 21:07:27.000000000 +0200 > > +++ linux-4.17/include/linux/usb/hcd.h 2018-06-12 16:07:11.000000000 +0200 > > @@ -428,6 +428,8 @@ extern int usb_hcd_submit_urb(struct urb > > extern int usb_hcd_unlink_urb(struct urb *urb, int status); > > extern void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, > > int status); > > +extern void _usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, > > + int status); > > extern int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, > > gfp_t mem_flags); > > extern void usb_hcd_unmap_urb_setup_for_dma(struct usb_hcd *, struct urb *); > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 12 Jun 2018, Mikulas Patocka wrote: > > > On Tue, 12 Jun 2018, Alan Stern wrote: > > > On Tue, 12 Jun 2018, Mikulas Patocka wrote: > > > > > I have a single-core machine with usb2 soundcard. When I increase mplayer > > > priority (to real-time or high non-realtime priority), the sound is > > > stuttering. The reason for the stuttering is that mplayer at high priority > > > preempts the softirq thread, preventing URBs from being completed. It was > > > caused by the patch 428aac8a81058 that offloads URB completion to softirq. > > > > > > This patch prevents offloading isochronous URBs to softirq to fix the > > > stuttering. > > > > How about just not running mplayer at such a high priority? > > I need to run mplayer at a high priority so that other work doesn't > preempt mplayer and cause stuttering. Think about this a little more... You _want_ the softirq thread to preempt mplayer. Or at least, you don't want mplayer to use so much CPU time that the softirq thread doesn't get a chance to run. > > Or raising the priority of the softirq thread? > > Do you want to coordinate that with the softirq maintainers? I don't know > if they would be happy to add an extra real-time softirq thread. How about making the softirq thread's priority adjustable? As for coordinating with the softirq maintainers -- whether I want to or not isn't the issue. Right now I don't have _time_ to do it. Alan Stern > > Alan Stern > > Mikulas > > > > Fixes: c04ee4b1136e ("Revert "Revert "USB: EHCI: support running URB giveback in tasklet context""") > > > Cc: stable@vger.kernel.org > > > > > > --- > > > drivers/usb/core/hcd.c | 10 ++++++++++ > > > drivers/usb/host/ehci-q.c | 11 ++++++++++- > > > include/linux/usb/hcd.h | 2 ++ > > > 3 files changed, 22 insertions(+), 1 deletion(-) > > > > > > Index: linux-4.17/drivers/usb/core/hcd.c > > > =================================================================== > > > --- linux-4.17.orig/drivers/usb/core/hcd.c 2018-06-12 16:06:23.000000000 +0200 > > > +++ linux-4.17/drivers/usb/core/hcd.c 2018-06-12 16:07:51.000000000 +0200 > > > @@ -1858,6 +1858,16 @@ void usb_hcd_giveback_urb(struct usb_hcd > > > } > > > EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb); > > > > > > +void _usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status) > > > +{ > > > + /* pass status to tasklet via unlinked */ > > > + if (likely(!urb->unlinked)) > > > + urb->unlinked = status; > > > + > > > + __usb_hcd_giveback_urb(urb); > > > +} > > > +EXPORT_SYMBOL_GPL(_usb_hcd_giveback_urb); > > > + > > > /*-------------------------------------------------------------------------*/ > > > > > > /* Cancel all URBs pending on this endpoint and wait for the endpoint's > > > Index: linux-4.17/drivers/usb/host/ehci-q.c > > > =================================================================== > > > --- linux-4.17.orig/drivers/usb/host/ehci-q.c 2018-06-12 16:06:23.000000000 +0200 > > > +++ linux-4.17/drivers/usb/host/ehci-q.c 2018-06-12 16:09:28.000000000 +0200 > > > @@ -238,6 +238,8 @@ static int qtd_copy_status ( > > > > > > static void > > > ehci_urb_done(struct ehci_hcd *ehci, struct urb *urb, int status) > > > +__releases(ehci->lock) > > > +__acquires(ehci->lock) > > > { > > > if (usb_pipetype(urb->pipe) == PIPE_INTERRUPT) { > > > /* ... update hc-wide periodic stats */ > > > @@ -264,7 +266,14 @@ ehci_urb_done(struct ehci_hcd *ehci, str > > > #endif > > > > > > usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb); > > > - usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status); > > > + if (usb_pipeisoc(urb->pipe)) { > > > + /* complete() can reenter this HCD */ > > > + spin_unlock(&ehci->lock); > > > + _usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status); > > > + spin_lock(&ehci->lock); > > > + } else { > > > + usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status); > > > + } > > > } > > > > > > static int qh_schedule (struct ehci_hcd *ehci, struct ehci_qh *qh); > > > Index: linux-4.17/include/linux/usb/hcd.h > > > =================================================================== > > > --- linux-4.17.orig/include/linux/usb/hcd.h 2018-06-05 21:07:27.000000000 +0200 > > > +++ linux-4.17/include/linux/usb/hcd.h 2018-06-12 16:07:11.000000000 +0200 > > > @@ -428,6 +428,8 @@ extern int usb_hcd_submit_urb(struct urb > > > extern int usb_hcd_unlink_urb(struct urb *urb, int status); > > > extern void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, > > > int status); > > > +extern void _usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, > > > + int status); > > > extern int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, > > > gfp_t mem_flags); > > > extern void usb_hcd_unmap_urb_setup_for_dma(struct usb_hcd *, struct urb *); > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 12 Jun 2018, Alan Stern wrote: > On Tue, 12 Jun 2018, Mikulas Patocka wrote: > > > > > > > On Tue, 12 Jun 2018, Alan Stern wrote: > > > > > On Tue, 12 Jun 2018, Mikulas Patocka wrote: > > > > > > > I have a single-core machine with usb2 soundcard. When I increase mplayer > > > > priority (to real-time or high non-realtime priority), the sound is > > > > stuttering. The reason for the stuttering is that mplayer at high priority > > > > preempts the softirq thread, preventing URBs from being completed. It was > > > > caused by the patch 428aac8a81058 that offloads URB completion to softirq. > > > > > > > > This patch prevents offloading isochronous URBs to softirq to fix the > > > > stuttering. > > > > > > How about just not running mplayer at such a high priority? > > > > I need to run mplayer at a high priority so that other work doesn't > > preempt mplayer and cause stuttering. > > Think about this a little more... You _want_ the softirq thread to > preempt mplayer. Or at least, you don't want mplayer to use so much > CPU time that the softirq thread doesn't get a chance to run. I had usb1 sound card before - and there was no problem with high-priority mplayer. I could set mplayer to real time and it played solid. Because the UHCI driver doesn't offload URB callbacks to softirq. When I bought usb2 sound card, this problem with softirq arised. If I set it to realtime or -20, it skips, if I set it to -15, it skips less, perhaps there is some boundary between 0 and -15 where it stops skipping - but it seems quite stupid that music player skips more often with higher priority. > > > Or raising the priority of the softirq thread? > > > > Do you want to coordinate that with the softirq maintainers? I don't know > > if they would be happy to add an extra real-time softirq thread. > > How about making the softirq thread's priority adjustable? But you would have to argue with softirq maintainers about it - and you say that you don't have time for that. > As for coordinating with the softirq maintainers -- whether I want to > or not isn't the issue. Right now I don't have _time_ to do it. > > Alan Stern I am wondering - whats the purpose of that patch 428aac8a81058e2303677a8fbf26670229e51d3a at all? The patch shows some performance difference, but they are minor, about 1%. If you want to call the urb callback as soon as possible - why don't you just call it? Why do you need to offload the callback to a softirq thread? Mikulas -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 12 Jun 2018, Mikulas Patocka wrote: > > How about making the softirq thread's priority adjustable? > > But you would have to argue with softirq maintainers about it - and you > say that you don't have time for that. But maybe _you_ do... > > As for coordinating with the softirq maintainers -- whether I want to > > or not isn't the issue. Right now I don't have _time_ to do it. > > > > Alan Stern > > I am wondering - whats the purpose of that patch > 428aac8a81058e2303677a8fbf26670229e51d3a at all? The patch shows some > performance difference, but they are minor, about 1%. > > If you want to call the urb callback as soon as possible - why don't you > just call it? Why do you need to offload the callback to a softirq thread? Please read the Changelog entry for commit 94dfd7edfd5c. Basically the idea was to reduce overall latency by not doing as much work in an interrupt handler. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 12 Jun 2018, Alan Stern wrote: > On Tue, 12 Jun 2018, Mikulas Patocka wrote: > > > > How about making the softirq thread's priority adjustable? > > > > But you would have to argue with softirq maintainers about it - and you > > say that you don't have time for that. > > But maybe _you_ do... ksoftirqd has priority 0 - it is not suitable for real-time tasks, such as audio. In my opinion, it is much easier to fix this in the ehci driver (by not offloading isochronous completions), than to design a new real-time-capable ksoftirqd. > > > As for coordinating with the softirq maintainers -- whether I want to > > > or not isn't the issue. Right now I don't have _time_ to do it. > > > > > > Alan Stern > > > > I am wondering - whats the purpose of that patch > > 428aac8a81058e2303677a8fbf26670229e51d3a at all? The patch shows some > > performance difference, but they are minor, about 1%. > > > > If you want to call the urb callback as soon as possible - why don't you > > just call it? Why do you need to offload the callback to a softirq thread? > > Please read the Changelog entry for commit 94dfd7edfd5c. Basically the > idea was to reduce overall latency by not doing as much work in an > interrupt handler. > > Alan Stern snd_complete_urb is doing nothing but submitting the same urb again. Is resubmitting the urb really causing so much latency that you can't do it in the interrupt handler? Mikulas -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 12, 2018 at 01:19:28PM -0400, Mikulas Patocka wrote: > > > On Tue, 12 Jun 2018, Alan Stern wrote: > > > On Tue, 12 Jun 2018, Mikulas Patocka wrote: > > > > > > How about making the softirq thread's priority adjustable? > > > > > > But you would have to argue with softirq maintainers about it - and you > > > say that you don't have time for that. > > > > But maybe _you_ do... > > ksoftirqd has priority 0 - it is not suitable for real-time tasks, such as > audio. > > In my opinion, it is much easier to fix this in the ehci driver (by not > offloading isochronous completions), than to design a new > real-time-capable ksoftirqd. Ok, but what happens when you plug your device into a xhci controller? Do we also need to change that? Only touching a specific host controller is not good, you will be playing "whack a mole" for forever. Isoc packets are, by definition, not supposed to be guaranteed at all. So if they are "slow" or dropped or delayed somehow, that's fine. The sound protocol should be fine with it. Now yes, in reality, as you have found out, things can be "tight" on low-powered processors under heavy load. But what you are doing here is a priority inversion. You do not solve such a thing by going around and raising everything else up as well, this is supposed to be a "general purpose" kernel. You can tune a specific machine/device just fine this way, but not by messing with the kernel for the most part. > > > > As for coordinating with the softirq maintainers -- whether I want to > > > > or not isn't the issue. Right now I don't have _time_ to do it. > > > > > > > > Alan Stern > > > > > > I am wondering - whats the purpose of that patch > > > 428aac8a81058e2303677a8fbf26670229e51d3a at all? The patch shows some > > > performance difference, but they are minor, about 1%. > > > > > > If you want to call the urb callback as soon as possible - why don't you > > > just call it? Why do you need to offload the callback to a softirq thread? > > > > Please read the Changelog entry for commit 94dfd7edfd5c. Basically the > > idea was to reduce overall latency by not doing as much work in an > > interrupt handler. > > > > Alan Stern > > snd_complete_urb is doing nothing but submitting the same urb again. Is > resubmitting the urb really causing so much latency that you can't do it > in the interrupt handler? snd_complete_urb() does much more than just submition of the same urb. Perhaps if this is a real problem, the sound driver should have more than one urb pending? Is there a pool here that is somehow getting used up? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 12 Jun 2018, Mikulas Patocka wrote: > On Tue, 12 Jun 2018, Alan Stern wrote: > > > On Tue, 12 Jun 2018, Mikulas Patocka wrote: > > > > > > How about making the softirq thread's priority adjustable? > > > > > > But you would have to argue with softirq maintainers about it - and you > > > say that you don't have time for that. > > > > But maybe _you_ do... > > ksoftirqd has priority 0 - it is not suitable for real-time tasks, such as > audio. There have been suggestions posted to this mailing list for changing the USB stack to use a threaded interrupt routine instead of a tasklet for this purpose. Would that make your situation any better? > In my opinion, it is much easier to fix this in the ehci driver (by not > offloading isochronous completions), than to design a new > real-time-capable ksoftirqd. You probably never noticed this, but in fact we use _two_ bottom-half handlers for URB completions: one scheduled with normal priority and one scheduled with high priority (tasklet_hi_schedule()). Isochronous URB completions go to the high-priority handler. Shouldn't a high-priority tasklet be up to the job of handling audio? > > > > As for coordinating with the softirq maintainers -- whether I want to > > > > or not isn't the issue. Right now I don't have _time_ to do it. > > > > > > > > Alan Stern > > > > > > I am wondering - whats the purpose of that patch > > > 428aac8a81058e2303677a8fbf26670229e51d3a at all? The patch shows some > > > performance difference, but they are minor, about 1%. > > > > > > If you want to call the urb callback as soon as possible - why don't you > > > just call it? Why do you need to offload the callback to a softirq thread? > > > > Please read the Changelog entry for commit 94dfd7edfd5c. Basically the > > idea was to reduce overall latency by not doing as much work in an > > interrupt handler. > > > > Alan Stern > > snd_complete_urb is doing nothing but submitting the same urb again. Is > resubmitting the urb really causing so much latency that you can't do it > in the interrupt handler? Perhaps snd_complete_urb doesn't doing very much, but other drivers most definitely do. In particular, the completion handler for the USB video class driver can be very time consuming. Your proposed change would make things worse for people using USB video. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 12 Jun 2018, Greg Kroah-Hartman wrote: > On Tue, Jun 12, 2018 at 01:19:28PM -0400, Mikulas Patocka wrote: > > > > > > On Tue, 12 Jun 2018, Alan Stern wrote: > > > > > On Tue, 12 Jun 2018, Mikulas Patocka wrote: > > > > > > > > How about making the softirq thread's priority adjustable? > > > > > > > > But you would have to argue with softirq maintainers about it - and you > > > > say that you don't have time for that. > > > > > > But maybe _you_ do... > > > > ksoftirqd has priority 0 - it is not suitable for real-time tasks, such as > > audio. > > > > In my opinion, it is much easier to fix this in the ehci driver (by not > > offloading isochronous completions), than to design a new > > real-time-capable ksoftirqd. > > Ok, but what happens when you plug your device into a xhci controller? > Do we also need to change that? Only touching a specific host > controller is not good, you will be playing "whack a mole" for forever. xhci doesn't set the HCD_BH flag, so it doesn't offload callbacks to softirq and doesn't suffer from this problem. Neither ohci and uhci do. > Isoc packets are, by definition, not supposed to be guaranteed at all. > So if they are "slow" or dropped or delayed somehow, that's fine. The > sound protocol should be fine with it. The sound USB protocol doesn't provide any redundancy - so a missed packet means audible clipping. There are crappy USB controllers that skip an isochronous packet when they encounter too high memory latency - they are unuseable for audio - but this is not the problem here. The USB specification doesn't allow re-sending failed isochronous packets. > Now yes, in reality, as you have found out, things can be "tight" on > low-powered processors under heavy load. But what you are doing here is > a priority inversion. You do not solve such a thing by going around and > raising everything else up as well, this is supposed to be a "general > purpose" kernel. You can tune a specific machine/device just fine this > way, but not by messing with the kernel for the most part. > > > snd_complete_urb is doing nothing but submitting the same urb again. Is > > resubmitting the urb really causing so much latency that you can't do it > > in the interrupt handler? > > snd_complete_urb() does much more than just submition of the same urb. How is snd_complete_urb() different from a hard-irq handler of a PCI sound card? AFAIK sound irq handlers just set the current position in the ring and wake up a process that wants to read or write to the ring. This is so simple task, that there's no reason to offload it to softirq. > Perhaps if this is a real problem, the sound driver should have more > than one urb pending? Is there a pool here that is somehow getting used > up? It has 12 urbs - but it means that 12ms scheduling latency (which is common) means sound skipping. Games or proffesional sound applications need bounded sound latency, so you can't just increase the number of urbs arbitrarily. > thanks, > > greg k-h Mikulas -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 12 Jun 2018, Alan Stern wrote: > On Tue, 12 Jun 2018, Mikulas Patocka wrote: > > > On Tue, 12 Jun 2018, Alan Stern wrote: > > > > > On Tue, 12 Jun 2018, Mikulas Patocka wrote: > > > > > > > > How about making the softirq thread's priority adjustable? > > > > > > > > But you would have to argue with softirq maintainers about it - and you > > > > say that you don't have time for that. > > > > > > But maybe _you_ do... > > > > ksoftirqd has priority 0 - it is not suitable for real-time tasks, such as > > audio. > > There have been suggestions posted to this mailing list for changing > the USB stack to use a threaded interrupt routine instead of a tasklet > for this purpose. Would that make your situation any better? If you set real-time priority to the interrupt thread, then yes, I think it would solve the problem. > > In my opinion, it is much easier to fix this in the ehci driver (by not > > offloading isochronous completions), than to design a new > > real-time-capable ksoftirqd. > > You probably never noticed this, but in fact we use _two_ bottom-half > handlers for URB completions: one scheduled with normal priority and > one scheduled with high priority (tasklet_hi_schedule()). Isochronous > URB completions go to the high-priority handler. > > Shouldn't a high-priority tasklet be up to the job of handling audio? I noticed the function tasklet_hi_schedule. It has higher priority than other softirqs - but it gets offloaded to the ksoftirqd thread that has priority 0. So it can be preempted by anything - and it doesn't protect against skipping. If we raise the priority of ksoftirqd, people will start complaining such as "my machine is unuseable when it receives too many network packets". So, you basically need two ksoftirqds, one for networking (with regular priority) and one for audio (with high priority). > > > > > As for coordinating with the softirq maintainers -- whether I want to > > > > > or not isn't the issue. Right now I don't have _time_ to do it. > > > > > > > > > > Alan Stern > > > > > > > > I am wondering - whats the purpose of that patch > > > > 428aac8a81058e2303677a8fbf26670229e51d3a at all? The patch shows some > > > > performance difference, but they are minor, about 1%. > > > > > > > > If you want to call the urb callback as soon as possible - why don't you > > > > just call it? Why do you need to offload the callback to a softirq thread? > > > > > > Please read the Changelog entry for commit 94dfd7edfd5c. Basically the > > > idea was to reduce overall latency by not doing as much work in an > > > interrupt handler. > > > > > > Alan Stern > > > > snd_complete_urb is doing nothing but submitting the same urb again. Is > > resubmitting the urb really causing so much latency that you can't do it > > in the interrupt handler? > > Perhaps snd_complete_urb doesn't doing very much, but other drivers > most definitely do. In particular, the completion handler for the USB > video class driver can be very time consuming. Your proposed change > would make things worse for people using USB video. In that case we can avoid offloading just for snd_complete_urb. Would you agree to adding a flag such as URB_FAST_COMPLETION that is set just by the audio driver? Do the video usb devices use isochronous or bulk transfers? > Alan Stern Mikulas -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 12 Jun 2018, Mikulas Patocka wrote: > On Tue, 12 Jun 2018, Alan Stern wrote: > > > On Tue, 12 Jun 2018, Mikulas Patocka wrote: > > > > > On Tue, 12 Jun 2018, Alan Stern wrote: > > > > > > > On Tue, 12 Jun 2018, Mikulas Patocka wrote: > > > > > > > > > > How about making the softirq thread's priority adjustable? > > > > > > > > > > But you would have to argue with softirq maintainers about it - and you > > > > > say that you don't have time for that. > > > > > > > > But maybe _you_ do... > > > > > > ksoftirqd has priority 0 - it is not suitable for real-time tasks, such as > > > audio. > > > > There have been suggestions posted to this mailing list for changing > > the USB stack to use a threaded interrupt routine instead of a tasklet > > for this purpose. Would that make your situation any better? > > If you set real-time priority to the interrupt thread, then yes, I think > it would solve the problem. So that's a possibility. Unfortunately the proposal for using a interrupt thread hasn't made much headway so far. > > > In my opinion, it is much easier to fix this in the ehci driver (by not > > > offloading isochronous completions), than to design a new > > > real-time-capable ksoftirqd. > > > > You probably never noticed this, but in fact we use _two_ bottom-half > > handlers for URB completions: one scheduled with normal priority and > > one scheduled with high priority (tasklet_hi_schedule()). Isochronous > > URB completions go to the high-priority handler. > > > > Shouldn't a high-priority tasklet be up to the job of handling audio? > > I noticed the function tasklet_hi_schedule. It has higher priority than > other softirqs - but it gets offloaded to the ksoftirqd thread that has > priority 0. So it can be preempted by anything - and it doesn't protect > against skipping. > > If we raise the priority of ksoftirqd, people will start complaining such > as "my machine is unuseable when it receives too many network packets". > So, you basically need two ksoftirqds, one for networking (with regular > priority) and one for audio (with high priority). I wonder if this is not a valid concern. At the very least we could ask the softirq maintainers what their opinion/recommendation is. > > > snd_complete_urb is doing nothing but submitting the same urb again. Is > > > resubmitting the urb really causing so much latency that you can't do it > > > in the interrupt handler? > > > > Perhaps snd_complete_urb doesn't doing very much, but other drivers > > most definitely do. In particular, the completion handler for the USB > > video class driver can be very time consuming. Your proposed change > > would make things worse for people using USB video. > > In that case we can avoid offloading just for snd_complete_urb. Would you > agree to adding a flag such as URB_FAST_COMPLETION that is set just by the > audio driver? That's another possibility. > Do the video usb devices use isochronous or bulk transfers? I believe they use isochronous (maybe some use bulk, I haven't checked). Certainly that's the sort of application it's meant for. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-4.17/drivers/usb/core/hcd.c =================================================================== --- linux-4.17.orig/drivers/usb/core/hcd.c 2018-06-12 16:06:23.000000000 +0200 +++ linux-4.17/drivers/usb/core/hcd.c 2018-06-12 16:07:51.000000000 +0200 @@ -1858,6 +1858,16 @@ void usb_hcd_giveback_urb(struct usb_hcd } EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb); +void _usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status) +{ + /* pass status to tasklet via unlinked */ + if (likely(!urb->unlinked)) + urb->unlinked = status; + + __usb_hcd_giveback_urb(urb); +} +EXPORT_SYMBOL_GPL(_usb_hcd_giveback_urb); + /*-------------------------------------------------------------------------*/ /* Cancel all URBs pending on this endpoint and wait for the endpoint's Index: linux-4.17/drivers/usb/host/ehci-q.c =================================================================== --- linux-4.17.orig/drivers/usb/host/ehci-q.c 2018-06-12 16:06:23.000000000 +0200 +++ linux-4.17/drivers/usb/host/ehci-q.c 2018-06-12 16:09:28.000000000 +0200 @@ -238,6 +238,8 @@ static int qtd_copy_status ( static void ehci_urb_done(struct ehci_hcd *ehci, struct urb *urb, int status) +__releases(ehci->lock) +__acquires(ehci->lock) { if (usb_pipetype(urb->pipe) == PIPE_INTERRUPT) { /* ... update hc-wide periodic stats */ @@ -264,7 +266,14 @@ ehci_urb_done(struct ehci_hcd *ehci, str #endif usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb); - usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status); + if (usb_pipeisoc(urb->pipe)) { + /* complete() can reenter this HCD */ + spin_unlock(&ehci->lock); + _usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status); + spin_lock(&ehci->lock); + } else { + usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status); + } } static int qh_schedule (struct ehci_hcd *ehci, struct ehci_qh *qh); Index: linux-4.17/include/linux/usb/hcd.h =================================================================== --- linux-4.17.orig/include/linux/usb/hcd.h 2018-06-05 21:07:27.000000000 +0200 +++ linux-4.17/include/linux/usb/hcd.h 2018-06-12 16:07:11.000000000 +0200 @@ -428,6 +428,8 @@ extern int usb_hcd_submit_urb(struct urb extern int usb_hcd_unlink_urb(struct urb *urb, int status); extern void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status); +extern void _usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, + int status); extern int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flags); extern void usb_hcd_unmap_urb_setup_for_dma(struct usb_hcd *, struct urb *);
I have a single-core machine with usb2 soundcard. When I increase mplayer priority (to real-time or high non-realtime priority), the sound is stuttering. The reason for the stuttering is that mplayer at high priority preempts the softirq thread, preventing URBs from being completed. It was caused by the patch 428aac8a81058 that offloads URB completion to softirq. This patch prevents offloading isochronous URBs to softirq to fix the stuttering. Fixes: c04ee4b1136e ("Revert "Revert "USB: EHCI: support running URB giveback in tasklet context""") Cc: stable@vger.kernel.org --- drivers/usb/core/hcd.c | 10 ++++++++++ drivers/usb/host/ehci-q.c | 11 ++++++++++- include/linux/usb/hcd.h | 2 ++ 3 files changed, 22 insertions(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html