Re: Threaded interrupts for USB HCD instead of tasklets
On Fri, 1 Jun 2018, Sebastian Andrzej Siewior wrote: > On 2018-05-22 15:14:17 [-0400], Alan Stern wrote: > > Sorry, I don't understand that sentence at all. And I don't see how it > > could be relevant to the point I was trying to make. > > > > Consider, for example, drivers/hid/usbhid/hid-core.c. In that file, > > hid_io_error() is called by hid_irq_in(), which is an URB completion > > handler. hid_io_error() acquires usbhid_lock. Therefore it would be > > necessary to audit the usbhid driver to see whether interrupts are > > enabled or disabled any place where usbhid_lock is acquired. And in > > fact, hid_ctrl() (another completion handler) calls > > spin_lock(>lock) -- this could cause problems for you. And > > usbhid->lock is acquired in other places, ones that are not inside > > completion handlers. > > To pick up this example. hid_io_error() is called from process context > (usb_hid_open()) or the completion handler and disables interrupts while > acquiring the lock. hid_ctrl() acquires the lock without disabling the > interrupts. This is not a problem because hid_ctrl() can not be > preempted while it is holding the lock by anything that also wants the > lock. So hid-core could stay as-is and we would be fine. However > lockdep would complain if hid-core would be used on ehci > (softirq/tasklet completion) and ohci (IRQ-handler completion) because > then lockdep would think that hid_ctrl() invoked by ehci could be > preempted by the ohci driver. In any case, it's generally bad form for two routines to obtain the same lock in process context, if one of them uses spin_lock and the other uses spin_lock_irqsave. It indicates that the programmer didn't have a firm grasp on how the lock would be used. > > None of this has anything to do with IRQ usage or hrtimers. > hid-core sets up a timer_list timer hid_retry_timeout(). This timer runs > in softirq context which means it can't preempt the completion handler > (hid_ctrlhid_ctrl()) which does just spin_lock() (both run in BH). > However, if hid_retry_timeout() were a hrtimer timer then it would be > invoked in IRQ-context. In that case it could preempt hid_ctrlhid_ctrl() > and _irqsave() would be required in hid_ctrlhid_ctrl(). > > My counting reveals ~250 USB drivers. I think it is best to audit them > first and make sure that completion handler like hid_ctrl() do And any routines they call, obviously. > _irqsave(). Once that is done, the local_irq_save() in > __usb_hcd_giveback_urb() could go. Agreed. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Threaded interrupts for USB HCD instead of tasklets
On 2018-05-22 15:14:17 [-0400], Alan Stern wrote: > Sorry, I don't understand that sentence at all. And I don't see how it > could be relevant to the point I was trying to make. > > Consider, for example, drivers/hid/usbhid/hid-core.c. In that file, > hid_io_error() is called by hid_irq_in(), which is an URB completion > handler. hid_io_error() acquires usbhid_lock. Therefore it would be > necessary to audit the usbhid driver to see whether interrupts are > enabled or disabled any place where usbhid_lock is acquired. And in > fact, hid_ctrl() (another completion handler) calls > spin_lock(>lock) -- this could cause problems for you. And > usbhid->lock is acquired in other places, ones that are not inside > completion handlers. To pick up this example. hid_io_error() is called from process context (usb_hid_open()) or the completion handler and disables interrupts while acquiring the lock. hid_ctrl() acquires the lock without disabling the interrupts. This is not a problem because hid_ctrl() can not be preempted while it is holding the lock by anything that also wants the lock. So hid-core could stay as-is and we would be fine. However lockdep would complain if hid-core would be used on ehci (softirq/tasklet completion) and ohci (IRQ-handler completion) because then lockdep would think that hid_ctrl() invoked by ehci could be preempted by the ohci driver. > None of this has anything to do with IRQ usage or hrtimers. hid-core sets up a timer_list timer hid_retry_timeout(). This timer runs in softirq context which means it can't preempt the completion handler (hid_ctrlhid_ctrl()) which does just spin_lock() (both run in BH). However, if hid_retry_timeout() were a hrtimer timer then it would be invoked in IRQ-context. In that case it could preempt hid_ctrlhid_ctrl() and _irqsave() would be required in hid_ctrlhid_ctrl(). My counting reveals ~250 USB drivers. I think it is best to audit them first and make sure that completion handler like hid_ctrl() do _irqsave(). Once that is done, the local_irq_save() in __usb_hcd_giveback_urb() could go. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Threaded interrupts for USB HCD instead of tasklets
On 2018-05-22 15:14:17 [-0400], Alan Stern wrote:
> On Tue, 22 May 2018, Sebastian Andrzej Siewior wrote:
>
> > On 2018-05-07 11:37:29 [-0400], Alan Stern wrote:
> > > > As far as I understand it there should be no deadlock. Without the
> > > > local_irq_save() things should not deadlock because the HCD invokes USB
> > > > driver's (usb-storage for instance) ->complete callback always in the
> > > > same way. If you mix the usb-driver with two different HCDs (say ehci
> > > > and xhci) then lockdep would complain. However, the locks which were
> > > > allocated by usb-storage for the ehci driver would never be used in the
> > > > context of the xhci driver. So lockdep would report a deacklock but
> > > > there wouldn't be one (again, if I got the piece right).
> > >
> > > That argument would be correct if the completion routines were the only
> > > places where the higher-level drivers (such as usb-storage or usbhid)
> > > acquired their locks. But we can't depend on this being true; you
> > > would have to audit each USB driver to make sure.
> >
> > an entry point for IRQ usage outside of the driver would be the usage of
> > hrtimer.
>
> Sorry, I don't understand that sentence at all. And I don't see how it
> could be relevant to the point I was trying to make.
>
> Consider, for example, drivers/hid/usbhid/hid-core.c. In that file,
> hid_io_error() is called by hid_irq_in(), which is an URB completion
> handler. hid_io_error() acquires usbhid_lock. Therefore it would be
> necessary to audit the usbhid driver to see whether interrupts are
> enabled or disabled any place where usbhid_lock is acquired. And in
> fact, hid_ctrl() (another completion handler) calls
> spin_lock(>lock) -- this could cause problems for you. And
> usbhid->lock is acquired in other places, ones that are not inside
> completion handlers.
>
> None of this has anything to do with IRQ usage or hrtimers.
Yeah, I get what you mean.
> > You mean the "Reserved Bandwidth Transfers:" paragraph?
>
> Paragraphs (plural). Three paragraphs, to be precise.
>
> > > It's possible to rewrite the HCDs not to rely on this (I did exactly
> > > that for ehci-hcd), but it is a nontrivial job.
> >
> > are you referring to commit 9118f9eb4f1e ("USB: EHCI: improve interrupt
> > qh unlink")?
>
> That one, plus:
>
> 46c73d1d3ebc ("USB: EHCI: handle isochronous underruns with tasklets")
> e4e18cbd52c8 ("USB: EHCI: code rearrangement in iso_stream_schedule()")
> 24f531371de1 ("USB: EHCI: accept very late isochronous URBs")
> 35371e4fbc3e ("USB: EHCI: improve ehci_endpoint_disable")
>
> Not all parts of all those commits were relevant, but as far as I
> recall, they each contributed something. And I may have omitted
> one or two commits by mistake.
Thank you, let me look at those so I can see what is needed…
> Alan Stern
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Threaded interrupts for USB HCD instead of tasklets
On Tue, 22 May 2018, Sebastian Andrzej Siewior wrote:
> On 2018-05-07 11:37:29 [-0400], Alan Stern wrote:
> > > As far as I understand it there should be no deadlock. Without the
> > > local_irq_save() things should not deadlock because the HCD invokes USB
> > > driver's (usb-storage for instance) ->complete callback always in the
> > > same way. If you mix the usb-driver with two different HCDs (say ehci
> > > and xhci) then lockdep would complain. However, the locks which were
> > > allocated by usb-storage for the ehci driver would never be used in the
> > > context of the xhci driver. So lockdep would report a deacklock but
> > > there wouldn't be one (again, if I got the piece right).
> >
> > That argument would be correct if the completion routines were the only
> > places where the higher-level drivers (such as usb-storage or usbhid)
> > acquired their locks. But we can't depend on this being true; you
> > would have to audit each USB driver to make sure.
>
> an entry point for IRQ usage outside of the driver would be the usage of
> hrtimer.
Sorry, I don't understand that sentence at all. And I don't see how it
could be relevant to the point I was trying to make.
Consider, for example, drivers/hid/usbhid/hid-core.c. In that file,
hid_io_error() is called by hid_irq_in(), which is an URB completion
handler. hid_io_error() acquires usbhid_lock. Therefore it would be
necessary to audit the usbhid driver to see whether interrupts are
enabled or disabled any place where usbhid_lock is acquired. And in
fact, hid_ctrl() (another completion handler) calls
spin_lock(>lock) -- this could cause problems for you. And
usbhid->lock is acquired in other places, ones that are not inside
completion handlers.
None of this has anything to do with IRQ usage or hrtimers.
> We have a flag to let the hrtimer run in softirq but yes, we
> need to audit them.
>
> > > And I was thinking about converting all drivers to one model and then we
> > > could get rid of the block I quoted above.
> > >
> > > If nobody rejects the approach as such I would go and start hacking…
> > >
> > > > And even for those two, the conversion will not be easy. Simply
> > > > changing the giveback routines would violate the documented guarantees
> > > > for isochronous transfers.
> > >
> > > The requirement was that the ISO urb is completed before the BULK urb,
> > > right?
> >
> > No, that's not what I meant. For one thing, isochronous URBs don't
> > need to complete before bulk URBs in general (although they do have
> > higher priority).
> >
> > However, I was really referring to the kerneldoc for usb_submit_urb().
> > The part that talks about scheduling of isochronous and interrupt URBs
> > lists a bunch of requirements. In order to meet these requirements
> > some of the host controller drivers may rely on the fact that when they
> > give back an URB, the URB's completion routine will return before the
> > giveback call finishes.
>
> You mean the "Reserved Bandwidth Transfers:" paragraph?
Paragraphs (plural). Three paragraphs, to be precise.
> > It's possible to rewrite the HCDs not to rely on this (I did exactly
> > that for ehci-hcd), but it is a nontrivial job.
>
> are you referring to commit 9118f9eb4f1e ("USB: EHCI: improve interrupt
> qh unlink")?
That one, plus:
46c73d1d3ebc ("USB: EHCI: handle isochronous underruns with tasklets")
e4e18cbd52c8 ("USB: EHCI: code rearrangement in iso_stream_schedule()")
24f531371de1 ("USB: EHCI: accept very late isochronous URBs")
35371e4fbc3e ("USB: EHCI: improve ehci_endpoint_disable")
Not all parts of all those commits were relevant, but as far as I
recall, they each contributed something. And I may have omitted
one or two commits by mistake.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Threaded interrupts for USB HCD instead of tasklets
On 2018-05-07 11:37:29 [-0400], Alan Stern wrote:
> > As far as I understand it there should be no deadlock. Without the
> > local_irq_save() things should not deadlock because the HCD invokes USB
> > driver's (usb-storage for instance) ->complete callback always in the
> > same way. If you mix the usb-driver with two different HCDs (say ehci
> > and xhci) then lockdep would complain. However, the locks which were
> > allocated by usb-storage for the ehci driver would never be used in the
> > context of the xhci driver. So lockdep would report a deacklock but
> > there wouldn't be one (again, if I got the piece right).
>
> That argument would be correct if the completion routines were the only
> places where the higher-level drivers (such as usb-storage or usbhid)
> acquired their locks. But we can't depend on this being true; you
> would have to audit each USB driver to make sure.
an entry point for IRQ usage outside of the driver would be the usage of
hrtimer. We have a flag to let the hrtimer run in softirq but yes, we
need to audit them.
> > And I was thinking about converting all drivers to one model and then we
> > could get rid of the block I quoted above.
> >
> > If nobody rejects the approach as such I would go and start hacking…
> >
> > > And even for those two, the conversion will not be easy. Simply
> > > changing the giveback routines would violate the documented guarantees
> > > for isochronous transfers.
> >
> > The requirement was that the ISO urb is completed before the BULK urb,
> > right?
>
> No, that's not what I meant. For one thing, isochronous URBs don't
> need to complete before bulk URBs in general (although they do have
> higher priority).
>
> However, I was really referring to the kerneldoc for usb_submit_urb().
> The part that talks about scheduling of isochronous and interrupt URBs
> lists a bunch of requirements. In order to meet these requirements
> some of the host controller drivers may rely on the fact that when they
> give back an URB, the URB's completion routine will return before the
> giveback call finishes.
You mean the "Reserved Bandwidth Transfers:" paragraph?
> It's possible to rewrite the HCDs not to rely on this (I did exactly
> that for ehci-hcd), but it is a nontrivial job.
are you referring to commit 9118f9eb4f1e ("USB: EHCI: improve interrupt
qh unlink")?
> Alan Stern
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Threaded interrupts for USB HCD instead of tasklets
On Mon, 7 May 2018, Sebastian Andrzej Siewior wrote:
> On 2018-05-04 16:07:22 [-0400], Alan Stern wrote:
> > On Fri, 4 May 2018, Sebastian Andrzej Siewior wrote:
> >
> > > Hi Alan,
> > >
> > > I posted a RFC [0] to let the HCD complete the URB in threaded-IRQ
> > > context to avoid the softirq/tasklet. Mauro reported that this does not
> > > help him and never came back after that.
> > > You did not oppose the approach as long as there is no performance issue
> > > which I did not see. Would you mind if I revisit the approach and
> > > convert all HCDs to threaded IRQs while at it?
> >
> > I don't mind revisiting the approach, although it certainly would be
> > good to find out why it doesn't help Mauro's video problem.
> >
> > However, converting _all_ the HCDs to threaded IRQs is a different
> > story. It should be okay to convert the ones that currently use
> > tasklets, but I can't approve changes to all the others. Only the
> > drivers that I maintain (ohci-hcd and uhci-hcd).
>
> Sure. The plan was to convert all drivers to one model so we could get
> rid of:
>
> |static void __usb_hcd_giveback_urb(struct urb *urb)
> |{
> |…
> | /*
> | * We disable local IRQs here avoid possible deadlock because
> | * drivers may call spin_lock() to hold lock which might be
> | * acquired in one hard interrupt handler.
> | *
> | * The local_irq_save()/local_irq_restore() around complete()
> | * will be removed if current USB drivers have been cleaned up
> | * and no one may trigger the above deadlock situation when
> | * running complete() in tasklet.
> | */
> | local_irq_save(flags);
> | urb->complete(urb);
> | local_irq_restore(flags);
> |…
> |}
>
> As far as I understand it there should be no deadlock. Without the
> local_irq_save() things should not deadlock because the HCD invokes USB
> driver's (usb-storage for instance) ->complete callback always in the
> same way. If you mix the usb-driver with two different HCDs (say ehci
> and xhci) then lockdep would complain. However, the locks which were
> allocated by usb-storage for the ehci driver would never be used in the
> context of the xhci driver. So lockdep would report a deacklock but
> there wouldn't be one (again, if I got the piece right).
That argument would be correct if the completion routines were the only
places where the higher-level drivers (such as usb-storage or usbhid)
acquired their locks. But we can't depend on this being true; you
would have to audit each USB driver to make sure.
> And I was thinking about converting all drivers to one model and then we
> could get rid of the block I quoted above.
>
> If nobody rejects the approach as such I would go and start hacking…
>
> > And even for those two, the conversion will not be easy. Simply
> > changing the giveback routines would violate the documented guarantees
> > for isochronous transfers.
>
> The requirement was that the ISO urb is completed before the BULK urb,
> right?
No, that's not what I meant. For one thing, isochronous URBs don't
need to complete before bulk URBs in general (although they do have
higher priority).
However, I was really referring to the kerneldoc for usb_submit_urb().
The part that talks about scheduling of isochronous and interrupt URBs
lists a bunch of requirements. In order to meet these requirements
some of the host controller drivers may rely on the fact that when they
give back an URB, the URB's completion routine will return before the
giveback call finishes.
It's possible to rewrite the HCDs not to rely on this (I did exactly
that for ehci-hcd), but it is a nontrivial job.
Alan Stern
> If so, the last patch would enqueue the ISO urb and wait until the BULK
> urb was completed before it completed the ISO urb.
>
> > Alan Stern
>
> Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Threaded interrupts for USB HCD instead of tasklets
On 2018-05-04 16:07:22 [-0400], Alan Stern wrote:
> On Fri, 4 May 2018, Sebastian Andrzej Siewior wrote:
>
> > Hi Alan,
> >
> > I posted a RFC [0] to let the HCD complete the URB in threaded-IRQ
> > context to avoid the softirq/tasklet. Mauro reported that this does not
> > help him and never came back after that.
> > You did not oppose the approach as long as there is no performance issue
> > which I did not see. Would you mind if I revisit the approach and
> > convert all HCDs to threaded IRQs while at it?
>
> I don't mind revisiting the approach, although it certainly would be
> good to find out why it doesn't help Mauro's video problem.
>
> However, converting _all_ the HCDs to threaded IRQs is a different
> story. It should be okay to convert the ones that currently use
> tasklets, but I can't approve changes to all the others. Only the
> drivers that I maintain (ohci-hcd and uhci-hcd).
Sure. The plan was to convert all drivers to one model so we could get
rid of:
|static void __usb_hcd_giveback_urb(struct urb *urb)
|{
|…
| /*
| * We disable local IRQs here avoid possible deadlock because
| * drivers may call spin_lock() to hold lock which might be
| * acquired in one hard interrupt handler.
| *
| * The local_irq_save()/local_irq_restore() around complete()
| * will be removed if current USB drivers have been cleaned up
| * and no one may trigger the above deadlock situation when
| * running complete() in tasklet.
| */
| local_irq_save(flags);
| urb->complete(urb);
| local_irq_restore(flags);
|…
|}
As far as I understand it there should be no deadlock. Without the
local_irq_save() things should not deadlock because the HCD invokes USB
driver's (usb-storage for instance) ->complete callback always in the
same way. If you mix the usb-driver with two different HCDs (say ehci
and xhci) then lockdep would complain. However, the locks which were
allocated by usb-storage for the ehci driver would never be used in the
context of the xhci driver. So lockdep would report a deacklock but
there wouldn't be one (again, if I got the piece right).
And I was thinking about converting all drivers to one model and then we
could get rid of the block I quoted above.
If nobody rejects the approach as such I would go and start hacking…
> And even for those two, the conversion will not be easy. Simply
> changing the giveback routines would violate the documented guarantees
> for isochronous transfers.
The requirement was that the ISO urb is completed before the BULK urb,
right?
If so, the last patch would enqueue the ISO urb and wait until the BULK
urb was completed before it completed the ISO urb.
> Alan Stern
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Threaded interrupts for USB HCD instead of tasklets
On Fri, 4 May 2018, Sebastian Andrzej Siewior wrote: > Hi Alan, > > I posted a RFC [0] to let the HCD complete the URB in threaded-IRQ > context to avoid the softirq/tasklet. Mauro reported that this does not > help him and never came back after that. > You did not oppose the approach as long as there is no performance issue > which I did not see. Would you mind if I revisit the approach and > convert all HCDs to threaded IRQs while at it? I don't mind revisiting the approach, although it certainly would be good to find out why it doesn't help Mauro's video problem. However, converting _all_ the HCDs to threaded IRQs is a different story. It should be okay to convert the ones that currently use tasklets, but I can't approve changes to all the others. Only the drivers that I maintain (ohci-hcd and uhci-hcd). And even for those two, the conversion will not be easy. Simply changing the giveback routines would violate the documented guarantees for isochronous transfers. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Threaded interrupts for USB HCD instead of tasklets
Hi Alan, I posted a RFC [0] to let the HCD complete the URB in threaded-IRQ context to avoid the softirq/tasklet. Mauro reported that this does not help him and never came back after that. You did not oppose the approach as long as there is no performance issue which I did not see. Would you mind if I revisit the approach and convert all HCDs to threaded IRQs while at it? [0] https://lkml.kernel.org/r/20180216170450.yl5owfphuvlts...@breakpoint.cc Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
