Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 12 Jun 2018 14:50:29 -0400 (EDT)
From:   Mikulas Patocka <mpatocka@redhat.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
cc:     Alan Stern <stern@rowland.harvard.edu>,
        Ming Lei <ming.lei@redhat.com>,
        USB list <linux-usb@vger.kernel.org>,
        Kernel development list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] usb: don't offload isochronous urb completions to
 ksoftirq
In-Reply-To: <20180612175242.GA12855@kroah.com>
Message-ID: <alpine.LRH.2.02.1806121403260.14879@file01.intranet.prod.int.rdu2.redhat.com>
References: <Pine.LNX.4.44L0.1806121232460.1582-100000@iolanthe.rowland.org> <alpine.LRH.2.02.1806121304530.1848@file01.intranet.prod.int.rdu2.redhat.com> <20180612175242.GA12855@kroah.com>
User-Agent: Alpine 2.02 (LRH 1266 2009-07-14)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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