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