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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 12 Jun 2018 11:11:31 -0400 (EDT)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Mikulas Patocka <mpatocka@...hat.com>
cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Ming Lei <ming.lei@...onical.com>, <linux-usb@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

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@...r.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@...r.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > > 
> > 
> 
>