[PATCH] usb: don't offload isochronous urb completions to ksoftirq

[PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Mikulas Patocka]

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(-)

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 *);

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Alan Stern]

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
> 
> 

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Mikulas Patocka]

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
> > 
> > 
> 

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Alan Stern]

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
> > > 
> > > 
> > 
> 
> 

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Mikulas Patocka]

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

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Alan Stern]

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

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Mikulas Patocka]

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

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Greg Kroah-Hartman]

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

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Mikulas Patocka]

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

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Alan Stern]

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

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Mikulas Patocka]

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

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Alan Stern]

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

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Mikulas Patocka]

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:
> > > 
> > > > 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.

Who maintains the softirq code? IRQ subsystem is maintained by Thomas 
Gleixner. I added him to this email.

> > > > 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.

Here I'm sending a patch that adds the flag URB_FAST_COMPLETION.

BTW I found this piece of code in sound/usb/usx2y/usbusx2yaudio.c:
                        urb->transfer_buffer_length = subs->maxpacksize * nr_of_packs();
                        if ((err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
                                snd_printk (KERN_ERR "cannot submit datapipe for urb %d, err = %d\n", i, err);
                                err = -EPIPE;
                                goto cleanup;
                        } else
                                if (i == 0)
                                        usX2Y->wait_iso_frame = urb->start_frame;
                        urb->transfer_flags = 0;
It seems like a bug to modify transfer_flags after the urb is submitted.

Mikulas





From: Mikulas Patocka <mpatocka@redhat.com>
Subject: [PATCH] usb: don't offload isochronous usb transfers to ksoftirq

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 introduces a flag URB_FAST_COMPLETION. When this flag is used,
the usb subsystem will not offload an URB completion to a thread. This
flag is set for the audio drivers.

Fixes: c04ee4b1136e ("Revert "Revert "USB: EHCI: support running URB giveback in tasklet context""")
Cc: stable@vger.kernel.org

---
 drivers/usb/core/hcd.c       |    3 ++-
 drivers/usb/core/urb.c       |    4 ++--
 drivers/usb/host/ehci-q.c    |   14 +++++++++++++-
 include/linux/usb.h          |    1 +
 sound/usb/6fire/pcm.c        |    1 +
 sound/usb/caiaq/audio.c      |    1 +
 sound/usb/endpoint.c         |    4 ++--
 sound/usb/line6/capture.c    |    2 +-
 sound/usb/line6/playback.c   |    2 +-
 sound/usb/misc/ua101.c       |    2 +-
 sound/usb/usx2y/usb_stream.c |    1 +
 11 files changed, 26 insertions(+), 9 deletions(-)

Index: linux-4.17/include/linux/usb.h
===================================================================
--- linux-4.17.orig/include/linux/usb.h	2018-06-05 21:07:26.000000000 +0200
+++ linux-4.17/include/linux/usb.h	2018-06-12 22:39:01.000000000 +0200
@@ -1299,6 +1299,7 @@ extern int usb_disabled(void);
 #define URB_ISO_ASAP		0x0002	/* iso-only; use the first unexpired
 					 * slot in the schedule */
 #define URB_NO_TRANSFER_DMA_MAP	0x0004	/* urb->transfer_dma valid on submit */
+#define URB_FAST_COMPLETION	0x0008	/* Complete from hardirq, don't offload completion to softirq */
 #define URB_ZERO_PACKET		0x0040	/* Finish bulk OUT with short packet */
 #define URB_NO_INTERRUPT	0x0080	/* HINT: no non-error interrupt
 					 * needed */
Index: linux-4.17/drivers/usb/core/hcd.c
===================================================================
--- linux-4.17.orig/drivers/usb/core/hcd.c	2018-06-12 22:35:21.000000000 +0200
+++ linux-4.17/drivers/usb/core/hcd.c	2018-06-12 22:40:44.000000000 +0200
@@ -1831,7 +1831,8 @@ void usb_hcd_giveback_urb(struct usb_hcd
 	if (likely(!urb->unlinked))
 		urb->unlinked = status;
 
-	if (!hcd_giveback_urb_in_bh(hcd) && !is_root_hub(urb->dev)) {
+	if ((!hcd_giveback_urb_in_bh(hcd) || urb->transfer_flags & URB_FAST_COMPLETION) &&
+	    !is_root_hub(urb->dev)) {
 		__usb_hcd_giveback_urb(urb);
 		return;
 	}
Index: linux-4.17/drivers/usb/host/ehci-q.c
===================================================================
--- linux-4.17.orig/drivers/usb/host/ehci-q.c	2018-06-12 22:35:21.000000000 +0200
+++ linux-4.17/drivers/usb/host/ehci-q.c	2018-06-12 22:44:09.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,17 @@ 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 (urb->transfer_flags & URB_FAST_COMPLETION) {
+		/*
+		 * USB audio experiences skipping of we offload completion
+		 * to ksoftirq.
+		 */
+		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/sound/usb/endpoint.c
===================================================================
--- linux-4.17.orig/sound/usb/endpoint.c	2018-06-12 20:45:25.000000000 +0200
+++ linux-4.17/sound/usb/endpoint.c	2018-06-13 13:46:44.000000000 +0200
@@ -789,7 +789,7 @@ static int data_ep_set_params(struct snd
 		if (!u->urb->transfer_buffer)
 			goto out_of_memory;
 		u->urb->pipe = ep->pipe;
-		u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
+		u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP | URB_FAST_COMPLETION;
 		u->urb->interval = 1 << ep->datainterval;
 		u->urb->context = u;
 		u->urb->complete = snd_complete_urb;
@@ -827,7 +827,7 @@ static int sync_ep_set_params(struct snd
 		u->urb->transfer_dma = ep->sync_dma + i * 4;
 		u->urb->transfer_buffer_length = 4;
 		u->urb->pipe = ep->pipe;
-		u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
+		u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP | URB_FAST_COMPLETION;
 		u->urb->number_of_packets = 1;
 		u->urb->interval = 1 << ep->syncinterval;
 		u->urb->context = u;
Index: linux-4.17/drivers/usb/core/urb.c
===================================================================
--- linux-4.17.orig/drivers/usb/core/urb.c	2018-06-05 21:06:59.000000000 +0200
+++ linux-4.17/drivers/usb/core/urb.c	2018-06-13 00:36:06.000000000 +0200
@@ -479,8 +479,8 @@ int usb_submit_urb(struct urb *urb, gfp_
 			usb_pipetype(urb->pipe), pipetypes[xfertype]);
 
 	/* Check against a simple/standard policy */
-	allowed = (URB_NO_TRANSFER_DMA_MAP | URB_NO_INTERRUPT | URB_DIR_MASK |
-			URB_FREE_BUFFER);
+	allowed = URB_NO_TRANSFER_DMA_MAP | URB_FAST_COMPLETION | URB_NO_INTERRUPT |
+		URB_FREE_BUFFER | URB_DIR_MASK;
 	switch (xfertype) {
 	case USB_ENDPOINT_XFER_BULK:
 	case USB_ENDPOINT_XFER_INT:
Index: linux-4.17/sound/usb/6fire/pcm.c
===================================================================
--- linux-4.17.orig/sound/usb/6fire/pcm.c	2017-11-29 02:53:58.000000000 +0100
+++ linux-4.17/sound/usb/6fire/pcm.c	2018-06-13 13:58:37.000000000 +0200
@@ -574,6 +574,7 @@ static void usb6fire_pcm_init_urb(struct
 {
 	urb->chip = chip;
 	usb_init_urb(&urb->instance);
+	urb->instance.transfer_flags = URB_FAST_COMPLETION;
 	urb->instance.transfer_buffer = urb->buffer;
 	urb->instance.transfer_buffer_length =
 			PCM_N_PACKETS_PER_URB * PCM_MAX_PACKET_SIZE;
Index: linux-4.17/sound/usb/caiaq/audio.c
===================================================================
--- linux-4.17.orig/sound/usb/caiaq/audio.c	2017-11-29 02:53:58.000000000 +0100
+++ linux-4.17/sound/usb/caiaq/audio.c	2018-06-13 13:51:22.000000000 +0200
@@ -758,6 +758,7 @@ static struct urb **alloc_urbs(struct sn
 
 		urbs[i]->dev = usb_dev;
 		urbs[i]->pipe = pipe;
+		urbs[i]->transfer_flags = URB_FAST_COMPLETION;
 		urbs[i]->transfer_buffer_length = FRAMES_PER_URB
 						* BYTES_PER_FRAME;
 		urbs[i]->context = &cdev->data_cb_info[i];
Index: linux-4.17/sound/usb/usx2y/usb_stream.c
===================================================================
--- linux-4.17.orig/sound/usb/usx2y/usb_stream.c	2018-06-05 21:07:57.000000000 +0200
+++ linux-4.17/sound/usb/usx2y/usb_stream.c	2018-06-13 13:53:12.000000000 +0200
@@ -69,6 +69,7 @@ static int init_pipe_urbs(struct usb_str
 	     ++u, transfer += transfer_length) {
 		struct urb *urb = urbs[u];
 		struct usb_iso_packet_descriptor *desc;
+		urb->transfer_flags = URB_FAST_COMPLETION;
 		urb->transfer_buffer = transfer;
 		urb->dev = dev;
 		urb->pipe = pipe;
Index: linux-4.17/sound/usb/line6/capture.c
===================================================================
--- linux-4.17.orig/sound/usb/line6/capture.c	2018-06-05 21:07:57.000000000 +0200
+++ linux-4.17/sound/usb/line6/capture.c	2018-06-13 15:17:45.000000000 +0200
@@ -285,7 +285,7 @@ int line6_create_audio_in_urbs(struct sn
 		    usb_rcvisocpipe(line6->usbdev,
 				    line6->properties->ep_audio_r &
 				    USB_ENDPOINT_NUMBER_MASK);
-		urb->transfer_flags = URB_ISO_ASAP;
+		urb->transfer_flags = URB_ISO_ASAP | URB_FAST_COMPLETION;
 		urb->start_frame = -1;
 		urb->number_of_packets = LINE6_ISO_PACKETS;
 		urb->interval = LINE6_ISO_INTERVAL;
Index: linux-4.17/sound/usb/line6/playback.c
===================================================================
--- linux-4.17.orig/sound/usb/line6/playback.c	2018-06-05 21:07:57.000000000 +0200
+++ linux-4.17/sound/usb/line6/playback.c	2018-06-13 15:19:31.000000000 +0200
@@ -430,7 +430,7 @@ int line6_create_audio_out_urbs(struct s
 		    usb_sndisocpipe(line6->usbdev,
 				    line6->properties->ep_audio_w &
 				    USB_ENDPOINT_NUMBER_MASK);
-		urb->transfer_flags = URB_ISO_ASAP;
+		urb->transfer_flags = URB_ISO_ASAP | URB_FAST_COMPLETION;
 		urb->start_frame = -1;
 		urb->number_of_packets = LINE6_ISO_PACKETS;
 		urb->interval = LINE6_ISO_INTERVAL;
Index: linux-4.17/sound/usb/misc/ua101.c
===================================================================
--- linux-4.17.orig/sound/usb/misc/ua101.c	2017-11-29 02:53:58.000000000 +0100
+++ linux-4.17/sound/usb/misc/ua101.c	2018-06-13 15:20:32.000000000 +0200
@@ -1120,7 +1120,7 @@ static int alloc_stream_urbs(struct ua10
 			usb_init_urb(&urb->urb);
 			urb->urb.dev = ua->dev;
 			urb->urb.pipe = stream->usb_pipe;
-			urb->urb.transfer_flags = URB_NO_TRANSFER_DMA_MAP;
+			urb->urb.transfer_flags = URB_NO_TRANSFER_DMA_MAP | URB_FAST_COMPLETION;
 			urb->urb.transfer_buffer = addr;
 			urb->urb.transfer_dma = dma;
 			urb->urb.transfer_buffer_length = max_packet_size;

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Alan Stern]

On Wed, 13 Jun 2018, Mikulas Patocka wrote:

[Skipping lots of material...]

> BTW I found this piece of code in sound/usb/usx2y/usbusx2yaudio.c:
>                         urb->transfer_buffer_length = subs->maxpacksize * nr_of_packs();
>                         if ((err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
>                                 snd_printk (KERN_ERR "cannot submit datapipe for urb %d, err = %d\n", i, err);
>                                 err = -EPIPE;
>                                 goto cleanup;
>                         } else
>                                 if (i == 0)
>                                         usX2Y->wait_iso_frame = urb->start_frame;
>                         urb->transfer_flags = 0;
> It seems like a bug to modify transfer_flags after the urb is submitted.

Yes, it is definitely a bug.

> 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.
> --- linux-4.17.orig/drivers/usb/host/ehci-q.c	2018-06-12 22:35:21.000000000 +0200
> +++ linux-4.17/drivers/usb/host/ehci-q.c	2018-06-12 22:44:09.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,17 @@ 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 (urb->transfer_flags & URB_FAST_COMPLETION) {
> +		/*
> +		 * USB audio experiences skipping of we offload completion
> +		 * to ksoftirq.
> +		 */

This comment seems unnecessary.

> +		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);
> +	}
>  }

I'm not at all sure about this.  Have you audited all of ehci-hcd to 
make sure the driver doesn't assume that ehci->lock remains held while 
an URB is given back?  It's been so long since I worked on this area 
that I don't remember the answer offhand.

Alan Stern

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Mikulas Patocka]

On Wed, 13 Jun 2018, Alan Stern wrote:

> On Wed, 13 Jun 2018, Mikulas Patocka wrote:
> 
> [Skipping lots of material...]
> 
> > BTW I found this piece of code in sound/usb/usx2y/usbusx2yaudio.c:
> >                         urb->transfer_buffer_length = subs->maxpacksize * nr_of_packs();
> >                         if ((err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
> >                                 snd_printk (KERN_ERR "cannot submit datapipe for urb %d, err = %d\n", i, err);
> >                                 err = -EPIPE;
> >                                 goto cleanup;
> >                         } else
> >                                 if (i == 0)
> >                                         usX2Y->wait_iso_frame = urb->start_frame;
> >                         urb->transfer_flags = 0;
> > It seems like a bug to modify transfer_flags after the urb is submitted.
> 
> Yes, it is definitely a bug.
> 
> > 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.
> > --- linux-4.17.orig/drivers/usb/host/ehci-q.c	2018-06-12 22:35:21.000000000 +0200
> > +++ linux-4.17/drivers/usb/host/ehci-q.c	2018-06-12 22:44:09.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,17 @@ 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 (urb->transfer_flags & URB_FAST_COMPLETION) {
> > +		/*
> > +		 * USB audio experiences skipping of we offload completion
> > +		 * to ksoftirq.
> > +		 */
> 
> This comment seems unnecessary.
> 
> > +		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);
> > +	}
> >  }
> 
> I'm not at all sure about this.  Have you audited all of ehci-hcd to 
> make sure the driver doesn't assume that ehci->lock remains held while 
> an URB is given back?  It's been so long since I worked on this area 
> that I don't remember the answer offhand.
> 
> Alan Stern

I compared the current ehci code the code in the kernel 3.11 (that was the 
last kernel that didn't offload callbacks) and it is very similar. So, we 
can assume that if it was ok in the kernel 3.11, it is ok now.

itd_complete and sitd_complete are the same except for small formatting 
changes.

itd_submit and sitd_submit newly call ehci_urb_done, but it drops the 
spinlock after the call, therefore it tolerates that ehci_urb_done drops 
the spinlock.

qh_completions is almost the same in the kernel 3.11 and upstream, so if 
it tolerated dropped spinlock and resubmission in 3.11, it should tolerate 
it now.


BTW - if you think that dropping the spinlock could cause trouble - should 
we add the urbs to temporary list and call the callbacks just after the 
spinlock is dropped? But that would just add a lot of junk code to the 
ehci driver.

Another possibility is to hack the softirq subsystem so that tasklet_hi is 
never offloaded - but I don't know if it makes sense to make this change 
jsut because of ehci.

Mikulas

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Alan Stern]

[Steve: Sorry for dumping you into the middle of this discussion.  
Please see especially the last two paragraphs below.  Mikulas is
getting dropouts with USB audio because part of the processing uses a
tasklet.]

On Wed, 13 Jun 2018, Mikulas Patocka wrote:

> > > +		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);
> > > +	}
> > >  }
> > 
> > I'm not at all sure about this.  Have you audited all of ehci-hcd to 
> > make sure the driver doesn't assume that ehci->lock remains held while 
> > an URB is given back?  It's been so long since I worked on this area 
> > that I don't remember the answer offhand.
> > 
> > Alan Stern
> 
> I compared the current ehci code the code in the kernel 3.11 (that was the 
> last kernel that didn't offload callbacks) and it is very similar. So, we 
> can assume that if it was ok in the kernel 3.11, it is ok now.
> 
> itd_complete and sitd_complete are the same except for small formatting 
> changes.
> 
> itd_submit and sitd_submit newly call ehci_urb_done, but it drops the 
> spinlock after the call, therefore it tolerates that ehci_urb_done drops 
> the spinlock.
> 
> qh_completions is almost the same in the kernel 3.11 and upstream, so if 
> it tolerated dropped spinlock and resubmission in 3.11, it should tolerate 
> it now.

It's also necessary to check the places these routines get called from:
scan_isoc, scan_intr, scan_async, and ehci_work.  However, the comments
in those routines say that they expect the lock might be dropped, so
they're probably okay.  ehci_work, in particular, is very careful about
this -- it started out that way, and I decided not to remove the
safeguards when the driver switched over to tasklets.

> BTW - if you think that dropping the spinlock could cause trouble - should 
> we add the urbs to temporary list and call the callbacks just after the 
> spinlock is dropped? But that would just add a lot of junk code to the 
> ehci driver.

I don't think this will be needed.

> Another possibility is to hack the softirq subsystem so that tasklet_hi is 
> never offloaded - but I don't know if it makes sense to make this change 
> jsut because of ehci.

I don't know.  But it isn't just ehci-hcd; _anything_ that tries to use
tasklets for bounded-latency applications will have the same problem.  
This sounds like the sort of thing the RT kernels must have addressed 
long ago.

Alan Stern

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Mikulas Patocka]

On Wed, 13 Jun 2018, Alan Stern wrote:

> [Steve: Sorry for dumping you into the middle of this discussion.  
> Please see especially the last two paragraphs below.  Mikulas is
> getting dropouts with USB audio because part of the processing uses a
> tasklet.]

The problem is this:

I have a single core machine with a usb2 sound card. When I increase the 
priority of a music player, the audio starts skipping.

The reason for the skipping is that the ehci usb driver is offloading urb 
callbacks using tasklet_hi_schedule, the callbacks end up being offloaded 
to the ksoftirqd thread (that has priority 0), the music player with 
elevated priority preempts ksoftirqd and causes delays in the urb 
callbacks.

Is this some deficiency in the softirq subsystem? (should we perhaps treat 
tasklet_hi specially and not offload it as much as the others?) Or should 
the ehci driver be fixed not to use tasklets?

Mikulas

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Steven Rostedt]

On Wed, 13 Jun 2018 15:30:31 -0400 (EDT)
Mikulas Patocka <mpatocka@redhat.com> wrote:

> On Wed, 13 Jun 2018, Alan Stern wrote:
> 
> > [Steve: Sorry for dumping you into the middle of this discussion.  
> > Please see especially the last two paragraphs below.  Mikulas is
> > getting dropouts with USB audio because part of the processing uses a
> > tasklet.]  
> 
> The problem is this:
> 
> I have a single core machine with a usb2 sound card. When I increase the 
> priority of a music player, the audio starts skipping.
> 
> The reason for the skipping is that the ehci usb driver is offloading urb 
> callbacks using tasklet_hi_schedule, the callbacks end up being offloaded 
> to the ksoftirqd thread (that has priority 0), the music player with 
> elevated priority preempts ksoftirqd and causes delays in the urb 
> callbacks.
> 
> Is this some deficiency in the softirq subsystem? (should we perhaps treat 
> tasklet_hi specially and not offload it as much as the others?) Or should 
> the ehci driver be fixed not to use tasklets?
> 

What we do for softirqs in the RT patch is to have whoever raised the
softirq run the softirq. If local_bh_disabled() is active (bh is
disabled) then a bit is set in the current task struct, where when
local_bh_enable() is called, it will then execute the softirqs that it
raised while bh was disabled.

Perhaps try out the PREEMPT_RT patch and see if the problem goes away.
Hopefully this softirq work may make it into the kernel soon. We could
even enabled it without full PREEMPT_RT.

 https://mirrors.edge.kernel.org/pub/linux/kernel/projects/rt/4.16/patch-4.16.12-rt5.patch.xz

-- Steve

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Mikulas Patocka]

On Wed, 13 Jun 2018, Steven Rostedt wrote:

> On Wed, 13 Jun 2018 15:30:31 -0400 (EDT)
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > On Wed, 13 Jun 2018, Alan Stern wrote:
> > 
> > > [Steve: Sorry for dumping you into the middle of this discussion.  
> > > Please see especially the last two paragraphs below.  Mikulas is
> > > getting dropouts with USB audio because part of the processing uses a
> > > tasklet.]  
> > 
> > The problem is this:
> > 
> > I have a single core machine with a usb2 sound card. When I increase the 
> > priority of a music player, the audio starts skipping.
> > 
> > The reason for the skipping is that the ehci usb driver is offloading urb 
> > callbacks using tasklet_hi_schedule, the callbacks end up being offloaded 
> > to the ksoftirqd thread (that has priority 0), the music player with 
> > elevated priority preempts ksoftirqd and causes delays in the urb 
> > callbacks.
> > 
> > Is this some deficiency in the softirq subsystem? (should we perhaps treat 
> > tasklet_hi specially and not offload it as much as the others?) Or should 
> > the ehci driver be fixed not to use tasklets?
> > 
> 
> What we do for softirqs in the RT patch is to have whoever raised the
> softirq run the softirq. If local_bh_disabled() is active (bh is
> disabled) then a bit is set in the current task struct, where when
> local_bh_enable() is called, it will then execute the softirqs that it
> raised while bh was disabled.
> 
> Perhaps try out the PREEMPT_RT patch and see if the problem goes away.

I tried the realtime patch with CONFIG_PREEMPT_RT_FULL and it plays well.

> Hopefully this softirq work may make it into the kernel soon. We could
> even enabled it without full PREEMPT_RT.

I don't think it's so easy. The kernel 2.4 and below did this. And the 
problem was that if there's a storm of network packets, the softirq code 
would cause lockup of the whole machine. In order to not lockup the 
machine - somewhere in the 2.4 cycle - the ksoftirqd thread was 
introduced.

If you have CONFIG_PREEMPT_RT_FULL and you call softirqs in the interrupt 
thread, you could only stall the interrupt thread. If you do the same 
thing without CONFIG_PREEMPT_RT_FULL, you stall the whole CPU.

Mikulas

>  https://mirrors.edge.kernel.org/pub/linux/kernel/projects/rt/4.16/patch-4.16.12-rt5.patch.xz
> 
> -- Steve
> 

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Steven Rostedt]

On Thu, 14 Jun 2018 18:23:11 -0400 (EDT)
Mikulas Patocka <mpatocka@redhat.com> wrote:

> I don't think it's so easy. The kernel 2.4 and below did this. And the 
> problem was that if there's a storm of network packets, the softirq code 
> would cause lockup of the whole machine. In order to not lockup the 
> machine - somewhere in the 2.4 cycle - the ksoftirqd thread was 
> introduced.
> 
> If you have CONFIG_PREEMPT_RT_FULL and you call softirqs in the interrupt 
> thread, you could only stall the interrupt thread. If you do the same 
> thing without CONFIG_PREEMPT_RT_FULL, you stall the whole CPU.
> 

Note, PREEMPT_RT also uses ksoftirqd too. Although we may set it to RT
prio 1. It is triggered if the softirq itself raises a softirq of the
same kind, and then the work is passed off to the ksoftirqd.

Causing the interrupt thread to stall (or by going into a loop of
softirqs) is likely to lock up the CPU on RT too, as interrupt threads
are usually run at priority 50, which will keep normal threads from
running on that CPU.

-- Steve

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Mikulas Patocka]

On Thu, 14 Jun 2018, Steven Rostedt wrote:

> On Thu, 14 Jun 2018 18:23:11 -0400 (EDT)
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > I don't think it's so easy. The kernel 2.4 and below did this. And the 
> > problem was that if there's a storm of network packets, the softirq code 
> > would cause lockup of the whole machine. In order to not lockup the 
> > machine - somewhere in the 2.4 cycle - the ksoftirqd thread was 
> > introduced.
> > 
> > If you have CONFIG_PREEMPT_RT_FULL and you call softirqs in the interrupt 
> > thread, you could only stall the interrupt thread. If you do the same 
> > thing without CONFIG_PREEMPT_RT_FULL, you stall the whole CPU.
> > 
> 
> Note, PREEMPT_RT also uses ksoftirqd too. Although we may set it to RT
> prio 1. It is triggered if the softirq itself raises a softirq of the
> same kind, and then the work is passed off to the ksoftirqd.

I think the major problem (in the upstream kernel) with softirq latency is 
this:
static inline void invoke_softirq(void)
{
        if (ksoftirqd_running())
                return;

It means that if any piece of code kicks ksoftirq, no tasklets are 
processed in the irq context at all.

So, the ehci callbacks will be offloaded to ksoftirqd (no matter how small 
they are) and this causes audio skipping. Could this be changed, so that 
it processes tasklets submitted with "tasklet_hi_schedule" in irq context 
even if ksoftirqd is running?

It's not easy - __do_softirq lacks any locking - so it can't run 
concurrently in process context and in irq context.

> Causing the interrupt thread to stall (or by going into a loop of
> softirqs) is likely to lock up the CPU on RT too, as interrupt threads
> are usually run at priority 50, which will keep normal threads from
> running on that CPU.
> 
> -- Steve

BTW. when I subject the machine to a ping flood (ping -f), the 
non-realtime kernel (with the patch to avoid offloading ehci urb 
callbacks) performs better than the real-time kernel.

With the real-time kernel, all the networking work is done in the thread 
"irq/12-eth0", that has (by default) priority -51, it consumes 30% CPU 
time and causes sound skipping. I can avoid the skipping by lowering the 
priority of "irq/12-eth0".

With non-realtime kernel, no such problem during ping flood exists.

Mikulas

Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Steven Rostedt]

On Fri, 15 Jun 2018 12:41:10 -0400 (EDT)
Mikulas Patocka <mpatocka@redhat.com> wrote:

> BTW. when I subject the machine to a ping flood (ping -f), the 
> non-realtime kernel (with the patch to avoid offloading ehci urb 
> callbacks) performs better than the real-time kernel.
> 
> With the real-time kernel, all the networking work is done in the thread 
> "irq/12-eth0", that has (by default) priority -51, it consumes 30% CPU 
> time and causes sound skipping. I can avoid the skipping by lowering the 
> priority of "irq/12-eth0".

That's actually the correct thing to do. When dealing with RT
applications, one needs to modify the priority of the interrupts that
are required (or lower the ones that are not).

> 
> With non-realtime kernel, no such problem during ping flood exists.

But that was with a patched kernel?

-- Steve

High-priority softirqs [was: [PATCH] usb: don't offload isochronous urb completions to ksoftirq]

[Alan Stern]

On Fri, 15 Jun 2018, Mikulas Patocka wrote:

> I think the major problem (in the upstream kernel) with softirq latency is 
> this:
> static inline void invoke_softirq(void)
> {
>         if (ksoftirqd_running())
>                 return;
> 
> It means that if any piece of code kicks ksoftirq, no tasklets are 
> processed in the irq context at all.
> 
> So, the ehci callbacks will be offloaded to ksoftirqd (no matter how small 
> they are) and this causes audio skipping. Could this be changed, so that 
> it processes tasklets submitted with "tasklet_hi_schedule" in irq context 
> even if ksoftirqd is running?

That's a great question.  Basically you're asking to have the HI 
softirq always handled in the bottom half, never in ksoftirqd, right?  

Or maybe to have more than one softirq thread, to handle different 
softirq levels, so that they can be assigned differing priorities.

> It's not easy - __do_softirq lacks any locking - so it can't run 
> concurrently in process context and in irq context.

I have no idea.  Maybe someone with a better understanding of this part
of the kernel can help.

Alan Stern

Re: High-priority softirqs [was: [PATCH] usb: don't offload isochronous urb completions to ksoftirq]

[Thomas Gleixner]

On Fri, 15 Jun 2018, Alan Stern wrote:
> On Fri, 15 Jun 2018, Mikulas Patocka wrote:
> 
> > I think the major problem (in the upstream kernel) with softirq latency is 
> > this:
> > static inline void invoke_softirq(void)
> > {
> >         if (ksoftirqd_running())
> >                 return;
> > 
> > It means that if any piece of code kicks ksoftirq, no tasklets are 
> > processed in the irq context at all.
> > 
> > So, the ehci callbacks will be offloaded to ksoftirqd (no matter how small 
> > they are) and this causes audio skipping. Could this be changed, so that 
> > it processes tasklets submitted with "tasklet_hi_schedule" in irq context 
> > even if ksoftirqd is running?
> 
> That's a great question.  Basically you're asking to have the HI 
> softirq always handled in the bottom half, never in ksoftirqd, right?  
> 
> Or maybe to have more than one softirq thread, to handle different 
> softirq levels, so that they can be assigned differing priorities.
> 
> > It's not easy - __do_softirq lacks any locking - so it can't run 
> > concurrently in process context and in irq context.
> 
> I have no idea.  Maybe someone with a better understanding of this part
> of the kernel can help.

Well, yes. Similar issues have been discussed in the past, but softirqs and
especially tasklets have horrible semantics. There was an attempt to deal
with that, but that didn't end well.

There is another even worse issue with USB/audio which I looked at a few
days ago. Some USB hosts offload to kworkers, which has even worse
behaviour than the ksoftirqd case.

One solution to that is to avoid both tasklets and kworkers and change the
USB code to make use of threaded interrupt handlers. I.e. handle the fast
stuff in the primary (hardirq) handler and delegate the rest to the irq
thread. That thread still can offload disk type stuff to a kworker if
needed. But the irq thread allows to bring the stuff under scheduler
control and experiments which I did a few years ago worked out pretty good.

Thanks,

	tglx

Re: High-priority softirqs [was: [PATCH] usb: don't offload isochronous urb completions to ksoftirq]

[Steven Rostedt]

On Fri, 15 Jun 2018 19:28:34 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> One solution to that is to avoid both tasklets and kworkers and change the
> USB code to make use of threaded interrupt handlers. I.e. handle the fast
> stuff in the primary (hardirq) handler and delegate the rest to the irq
> thread. That thread still can offload disk type stuff to a kworker if
> needed. But the irq thread allows to bring the stuff under scheduler
> control and experiments which I did a few years ago worked out pretty good.

If there's any question about this, drivers can request to have their
interrupt handlers run as threads. This has been added to mainline
years ago. And it really should be the default solution before pushing
off to tasklets or kworkers.

-- Steve

Re: High-priority softirqs [was: [PATCH] usb: don't offload isochronous urb completions to ksoftirq]

[Sebastian Sewior]

On 2018-06-15 13:34:28 [-0400], Steven Rostedt wrote:
> On Fri, 15 Jun 2018 19:28:34 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > One solution to that is to avoid both tasklets and kworkers and change the
> > USB code to make use of threaded interrupt handlers. I.e. handle the fast
> > stuff in the primary (hardirq) handler and delegate the rest to the irq
> > thread. That thread still can offload disk type stuff to a kworker if
> > needed. But the irq thread allows to bring the stuff under scheduler
> > control and experiments which I did a few years ago worked out pretty good.
> 
> If there's any question about this, drivers can request to have their
> interrupt handlers run as threads. This has been added to mainline
> years ago. And it really should be the default solution before pushing
> off to tasklets or kworkers.

 https://lkml.kernel.org/r/20180216170450.yl5owfphuvltstnt@breakpoint.cc

> -- Steve

Sebastian

Re: High-priority softirqs [was: [PATCH] usb: don't offload isochronous urb completions to ksoftirq]

[Alan Stern]

On Fri, 15 Jun 2018, Sebastian Sewior wrote:

> On 2018-06-15 13:34:28 [-0400], Steven Rostedt wrote:
> > On Fri, 15 Jun 2018 19:28:34 +0200 (CEST)
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > One solution to that is to avoid both tasklets and kworkers and change the
> > > USB code to make use of threaded interrupt handlers. I.e. handle the fast
> > > stuff in the primary (hardirq) handler and delegate the rest to the irq
> > > thread. That thread still can offload disk type stuff to a kworker if
> > > needed. But the irq thread allows to bring the stuff under scheduler
> > > control and experiments which I did a few years ago worked out pretty good.
> > 
> > If there's any question about this, drivers can request to have their
> > interrupt handlers run as threads. This has been added to mainline
> > years ago. And it really should be the default solution before pushing
> > off to tasklets or kworkers.
> 
>  https://lkml.kernel.org/r/20180216170450.yl5owfphuvltstnt@breakpoint.cc

This is more or less independent of our original question.  Even if we 
migrate the USB stack to use threaded interrupt handlers instead of 
tasklets, we still have to face the issue of completing some 
isochronous URBs at high priority and others not in interrupt context 
(presumably at normal priority).

The URB_FAST_COMPLETION approach seems like the best solution to the
audio problem.  And if we do switch over to threaded interrupts for URB
completions in the future, it should still be compatible.

Alan Stern

Re: High-priority softirqs [was: [PATCH] usb: don't offload isochronous urb completions to ksoftirq]

[Mikulas Patocka]

On Fri, 15 Jun 2018, Thomas Gleixner wrote:

> One solution to that is to avoid both tasklets and kworkers and change the
> USB code to make use of threaded interrupt handlers. I.e. handle the fast
> stuff in the primary (hardirq) handler and delegate the rest to the irq
> thread. That thread still can offload disk type stuff to a kworker if
> needed. But the irq thread allows to bring the stuff under scheduler
> control and experiments which I did a few years ago worked out pretty good.
> 
> Thanks,
> 
> 	tglx

I don't think that threaded interrupt handlers are a good idea.

There are existing tools such as rtkit in Linux distributions that 
increase the priority of audio applications to real time. And if rtkit 
increases the priority of audio player or audio server above the priority 
of the interrupt thread that handles the soundcard - sound playback is 
screwed.

You would have to set the priority of the interrupt thread to the highest 
real-time priority - and in such a case, the interrupt thread is no 
different than a hard-irq handler.

Mikulas

Re: High-priority softirqs [was: [PATCH] usb: don't offload isochronous urb completions to ksoftirq]

[Steven Rostedt]

On Fri, 15 Jun 2018 17:05:01 -0400 (EDT)
Mikulas Patocka <mpatocka@redhat.com> wrote:


> I don't think that threaded interrupt handlers are a good idea.
> 
> There are existing tools such as rtkit in Linux distributions that 
> increase the priority of audio applications to real time. And if rtkit 
> increases the priority of audio player or audio server above the priority 
> of the interrupt thread that handles the soundcard - sound playback is 
> screwed.
> 
> You would have to set the priority of the interrupt thread to the highest 
> real-time priority - and in such a case, the interrupt thread is no 
> different than a hard-irq handler.

Perhaps rtkit et.al. should be updated to know about interrupt threads.
It's pretty trivial to know about them and that can easily be
automated. The only problem is to keep the irq thread higher than the
audio server. And we always tell people to never put a thread to the
maximum priority unless they had a damn good reason to do so.

-- Steve