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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.44L0.1806121603460.1582-100000@iolanthe.rowland.org>
Date:   Tue, 12 Jun 2018 16:06:43 -0400 (EDT)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Mikulas Patocka <mpatocka@...hat.com>
cc:     Ming Lei <ming.lei@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        USB list <linux-usb@...r.kernel.org>,
        Kernel development list <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:
> > 
> > > 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