| Message ID | 41fc74d0bdea4c0efc269150b78d72b2b26cb38c.1475992312.git.dalias@libc.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 9 Oct 2016, Rich Felker wrote: > Ideas for improvement are welcome -- for example the > irq_is_percpu(irq_desc_get_irq(desc)) thing looks rather silly but I See the other mail. > didn't see a better way without poking through abstractions -- but > overall I think this both solves the timer stall issue that I wasted > other people's time on, and addresses the concerns about the J-Core > AIC driver being oblivious to whether an irq is per-cpu. You could put that knowledge into the device tree so you can decide at mapping time whether it is per cpu or not. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote: > On Sun, 9 Oct 2016, Rich Felker wrote: > > Ideas for improvement are welcome -- for example the > > irq_is_percpu(irq_desc_get_irq(desc)) thing looks rather silly but I > > See the other mail. > > > didn't see a better way without poking through abstractions -- but > > overall I think this both solves the timer stall issue that I wasted > > other people's time on, and addresses the concerns about the J-Core > > AIC driver being oblivious to whether an irq is per-cpu. > > You could put that knowledge into the device tree so you can decide at > mapping time whether it is per cpu or not. As discussed in the previous thread(s?) on the percpu topic, I'd really rather avoid doing this. There's both the conceptual aspect that it's redundant information in the DT that's only there for the sake of making the kernel irq system happier (i.e. if there were a code path to allow it, the handler type could be set at request_irq time with knowledge of the flags rather than requiring the irqchip driver to already have duplicate knowledge of this), and a couple practical issues. One simply relates back to redundancy -- I'd like to avoid making the DT bigger since it's likely desirable to have the DT embedded in internal ROM in ASICs. But the other big issue is that we already have devices in the field using the existing DT bindings. My preference would just be to keep the branch, but with your improved version that doesn't need a function call: irqd_is_per_cpu(irq_desc_get_irq_data(desc)) While there is some overhead testing this condition every time, I can probably come up with several better places to look for a ~10 cycle improvement in the irq code path without imposing new requirements on the DT bindings. If that's really not acceptable, an alternative proposal would be a new optional DT property that would give the irqchip driver sufficient knowledge to bind the proper handler directly at mapping time, and then handle_jcore_irq would be needed only in the fallback case where this property is missing. As noted in my followup to the clocksource stall thread, there's also a possibility that it might make sense to consider the current behavior of having non-percpu irqs bound to a particular cpu as part of what's required by the compatible tag, in which case handle_percpu_irq or something similar/equivalent might be suitable for both the percpu and non-percpu cases. I don't understand the irq subsystem well enough to insist on that but I think it's worth consideration since it looks like it would improve performance of non-percpu interrupts a bit. Rich -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 9 Oct 2016, Rich Felker wrote: > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote: > My preference would just be to keep the branch, but with your improved > version that doesn't need a function call: > > irqd_is_per_cpu(irq_desc_get_irq_data(desc)) > > While there is some overhead testing this condition every time, I can > probably come up with several better places to look for a ~10 cycle > improvement in the irq code path without imposing new requirements on > the DT bindings. Fair enough. Your call. > As noted in my followup to the clocksource stall thread, there's also > a possibility that it might make sense to consider the current > behavior of having non-percpu irqs bound to a particular cpu as part > of what's required by the compatible tag, in which case > handle_percpu_irq or something similar/equivalent might be suitable > for both the percpu and non-percpu cases. I don't understand the irq > subsystem well enough to insist on that but I think it's worth > consideration since it looks like it would improve performance of > non-percpu interrupts a bit. Well, you can use handle_percpu_irq() for your device interrupts if you guarantee at the hardware level that there is no reentrancy. Once you make the hardware capable of delivering them on either core the picture changes. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote: > On Sun, 9 Oct 2016, Rich Felker wrote: > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote: > > My preference would just be to keep the branch, but with your improved > > version that doesn't need a function call: > > > > irqd_is_per_cpu(irq_desc_get_irq_data(desc)) > > > > While there is some overhead testing this condition every time, I can > > probably come up with several better places to look for a ~10 cycle > > improvement in the irq code path without imposing new requirements on > > the DT bindings. > > Fair enough. Your call. Thanks. > > As noted in my followup to the clocksource stall thread, there's also > > a possibility that it might make sense to consider the current > > behavior of having non-percpu irqs bound to a particular cpu as part > > of what's required by the compatible tag, in which case > > handle_percpu_irq or something similar/equivalent might be suitable > > for both the percpu and non-percpu cases. I don't understand the irq > > subsystem well enough to insist on that but I think it's worth > > consideration since it looks like it would improve performance of > > non-percpu interrupts a bit. > > Well, you can use handle_percpu_irq() for your device interrupts if you > guarantee at the hardware level that there is no reentrancy. Reentrancy is possible of course if the kernel enables irqs during the irq handler. Is not doing so a stable part of the kernel irq subsystem? My understanding is that modern kernels keep irqs disabled for the full duration of (hard) irq handlers. > Once you make > the hardware capable of delivering them on either core the picture changes. *nod* Perhaps if/when we do that, the path of least resistence would be to adjust the irq numbering so that percpu (i.e., hard-routed to a particular cpu) and global irqs (deliverable on any core) are in different ranges and the existing kernel frameworks work. Rich -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 9 Oct 2016, Rich Felker wrote: > On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote: > > Well, you can use handle_percpu_irq() for your device interrupts if you > > guarantee at the hardware level that there is no reentrancy. > > Reentrancy is possible of course if the kernel enables irqs during the > irq handler. Is not doing so a stable part of the kernel irq > subsystem? My understanding is that modern kernels keep irqs disabled > for the full duration of (hard) irq handlers. If you enable lockdep then it will yell at offenders which enable irqs in the interrupt handler. So yes, hard irq handlers are not allowed to reenable interrupts. > > Once you make > > the hardware capable of delivering them on either core the picture changes. > > *nod* Perhaps if/when we do that, the path of least resistence would > be to adjust the irq numbering so that percpu (i.e., hard-routed to a > particular cpu) and global irqs (deliverable on any core) are in > different ranges and the existing kernel frameworks work. That's very much what ARM and others do. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote: > On Sun, 9 Oct 2016, Rich Felker wrote: > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote: > > My preference would just be to keep the branch, but with your improved > > version that doesn't need a function call: > > > > irqd_is_per_cpu(irq_desc_get_irq_data(desc)) > > > > While there is some overhead testing this condition every time, I can > > probably come up with several better places to look for a ~10 cycle > > improvement in the irq code path without imposing new requirements on > > the DT bindings. > > Fair enough. Your call. > > > As noted in my followup to the clocksource stall thread, there's also > > a possibility that it might make sense to consider the current > > behavior of having non-percpu irqs bound to a particular cpu as part > > of what's required by the compatible tag, in which case > > handle_percpu_irq or something similar/equivalent might be suitable > > for both the percpu and non-percpu cases. I don't understand the irq > > subsystem well enough to insist on that but I think it's worth > > consideration since it looks like it would improve performance of > > non-percpu interrupts a bit. > > Well, you can use handle_percpu_irq() for your device interrupts if you > guarantee at the hardware level that there is no reentrancy. Once you make > the hardware capable of delivering them on either core the picture changes. One more concern here -- I see that handle_simple_irq is handling the soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff that's perhaps important too. Since soft-disable is all we have (there's no hard-disable of interrupts), is this a problem? In other words, can drivers have an expectation of not receiving interrupts when the irq is disabled? I would think anything compatible with irq sharing can't have such an expectation, but perhaps the kernel needs disabling internally for synchronization at module-unload time or similar cases? If you think any of these things are problems I'll switch back to the conditional version rather than using handle_percpu_irq for everything. Rich -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 11 Oct 2016, Rich Felker wrote: > On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote: > > On Sun, 9 Oct 2016, Rich Felker wrote: > > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote: > > > My preference would just be to keep the branch, but with your improved > > > version that doesn't need a function call: > > > > > > irqd_is_per_cpu(irq_desc_get_irq_data(desc)) > > > > > > While there is some overhead testing this condition every time, I can > > > probably come up with several better places to look for a ~10 cycle > > > improvement in the irq code path without imposing new requirements on > > > the DT bindings. > > > > Fair enough. Your call. > > > > > As noted in my followup to the clocksource stall thread, there's also > > > a possibility that it might make sense to consider the current > > > behavior of having non-percpu irqs bound to a particular cpu as part > > > of what's required by the compatible tag, in which case > > > handle_percpu_irq or something similar/equivalent might be suitable > > > for both the percpu and non-percpu cases. I don't understand the irq > > > subsystem well enough to insist on that but I think it's worth > > > consideration since it looks like it would improve performance of > > > non-percpu interrupts a bit. > > > > Well, you can use handle_percpu_irq() for your device interrupts if you > > guarantee at the hardware level that there is no reentrancy. Once you make > > the hardware capable of delivering them on either core the picture changes. > > One more concern here -- I see that handle_simple_irq is handling the > soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff > that's perhaps important too. Since soft-disable is all we have > (there's no hard-disable of interrupts), is this a problem? In other > words, can drivers have an expectation of not receiving interrupts > when the irq is disabled? I would think anything compatible with irq > sharing can't have such an expectation, but perhaps the kernel needs > disabling internally for synchronization at module-unload time or > similar cases? Sure. A driver would be surprised getting an interrupt when it is disabled, but with your exceptionally well thought out interrupt controller a pending (level) interrupt which is not handled will be reraised forever and just hard lock the machine. > If you think any of these things are problems I'll switch back to the > conditional version rather than using handle_percpu_irq for > everything. It might be the approach of least surprise, but it won't make a difference for the situation described above. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 12, 2016 at 10:18:02AM +0200, Thomas Gleixner wrote: > On Tue, 11 Oct 2016, Rich Felker wrote: > > On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote: > > > On Sun, 9 Oct 2016, Rich Felker wrote: > > > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote: > > > > My preference would just be to keep the branch, but with your improved > > > > version that doesn't need a function call: > > > > > > > > irqd_is_per_cpu(irq_desc_get_irq_data(desc)) > > > > > > > > While there is some overhead testing this condition every time, I can > > > > probably come up with several better places to look for a ~10 cycle > > > > improvement in the irq code path without imposing new requirements on > > > > the DT bindings. > > > > > > Fair enough. Your call. > > > > > > > As noted in my followup to the clocksource stall thread, there's also > > > > a possibility that it might make sense to consider the current > > > > behavior of having non-percpu irqs bound to a particular cpu as part > > > > of what's required by the compatible tag, in which case > > > > handle_percpu_irq or something similar/equivalent might be suitable > > > > for both the percpu and non-percpu cases. I don't understand the irq > > > > subsystem well enough to insist on that but I think it's worth > > > > consideration since it looks like it would improve performance of > > > > non-percpu interrupts a bit. > > > > > > Well, you can use handle_percpu_irq() for your device interrupts if you > > > guarantee at the hardware level that there is no reentrancy. Once you make > > > the hardware capable of delivering them on either core the picture changes. > > > > One more concern here -- I see that handle_simple_irq is handling the > > soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff > > that's perhaps important too. Since soft-disable is all we have > > (there's no hard-disable of interrupts), is this a problem? In other > > words, can drivers have an expectation of not receiving interrupts > > when the irq is disabled? I would think anything compatible with irq > > sharing can't have such an expectation, but perhaps the kernel needs > > disabling internally for synchronization at module-unload time or > > similar cases? > > Sure. A driver would be surprised getting an interrupt when it is disabled, > but with your exceptionally well thought out interrupt controller a pending > (level) interrupt which is not handled will be reraised forever and just > hard lock the machine. If you want to criticize the interrupt controller design (not my work or under my control) for limitations in the type of hardware that can be hooked up to it, that's okay -- this kind of input will actually be useful for designing the next iteration of it -- but I don't think this specific possibility is a concern. The controller is designed for SoC-internal use with devices that behave well, and not for level interrupts that require device-specific action to clear (the clearing of pending status is non-device-specific and takes place at the time the interrupt is accepted by the cpu). It might end up being that it makes sense to keep the AIC2 as-is but attach a separate, more general-purpose global interrupt cuntroller that can route to any cpu's AIC and that's friendlier to diverse external hardware (like the type of level interrupts you described) but the hardware team would know better than me. > > If you think any of these things are problems I'll switch back to the > > conditional version rather than using handle_percpu_irq for > > everything. > > It might be the approach of least surprise, but it won't make a difference > for the situation described above. I'm not seeing any easily measurable performance difference with the version using the conditional, so I'm going to submit that as a v3. Whether or not there's actually a safety concern, I'm not sure, but I'd rather use the functions the way they were intended to be used so we don't have to worry about unexpected bugs or regressions if the internals change. Rich -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 12, 2016 at 12:35:43PM -0400, Rich Felker wrote: > On Wed, Oct 12, 2016 at 10:18:02AM +0200, Thomas Gleixner wrote: > > On Tue, 11 Oct 2016, Rich Felker wrote: > > > On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote: > > > > On Sun, 9 Oct 2016, Rich Felker wrote: > > > > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote: > > > > > My preference would just be to keep the branch, but with your improved > > > > > version that doesn't need a function call: > > > > > > > > > > irqd_is_per_cpu(irq_desc_get_irq_data(desc)) > > > > > > > > > > While there is some overhead testing this condition every time, I can > > > > > probably come up with several better places to look for a ~10 cycle > > > > > improvement in the irq code path without imposing new requirements on > > > > > the DT bindings. > > > > > > > > Fair enough. Your call. > > > > > > > > > As noted in my followup to the clocksource stall thread, there's also > > > > > a possibility that it might make sense to consider the current > > > > > behavior of having non-percpu irqs bound to a particular cpu as part > > > > > of what's required by the compatible tag, in which case > > > > > handle_percpu_irq or something similar/equivalent might be suitable > > > > > for both the percpu and non-percpu cases. I don't understand the irq > > > > > subsystem well enough to insist on that but I think it's worth > > > > > consideration since it looks like it would improve performance of > > > > > non-percpu interrupts a bit. > > > > > > > > Well, you can use handle_percpu_irq() for your device interrupts if you > > > > guarantee at the hardware level that there is no reentrancy. Once you make > > > > the hardware capable of delivering them on either core the picture changes. > > > > > > One more concern here -- I see that handle_simple_irq is handling the > > > soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff > > > that's perhaps important too. Since soft-disable is all we have > > > (there's no hard-disable of interrupts), is this a problem? In other > > > words, can drivers have an expectation of not receiving interrupts > > > when the irq is disabled? I would think anything compatible with irq > > > sharing can't have such an expectation, but perhaps the kernel needs > > > disabling internally for synchronization at module-unload time or > > > similar cases? > > > > Sure. A driver would be surprised getting an interrupt when it is disabled, > > but with your exceptionally well thought out interrupt controller a pending > > (level) interrupt which is not handled will be reraised forever and just > > hard lock the machine. > > If you want to criticize the interrupt controller design (not my work > or under my control) for limitations in the type of hardware that can > be hooked up to it, that's okay -- this kind of input will actually be > useful for designing the next iteration of it -- but I don't think > this specific possibility is a concern. Well, if this scenario does happen, the machine will likely either lock up silently and hard, give you RCU CPU stall warning messages, or give you soft-lockup messages. Thanx, Paul > The controller is designed for > SoC-internal use with devices that behave well, and not for level > interrupts that require device-specific action to clear (the clearing > of pending status is non-device-specific and takes place at the time > the interrupt is accepted by the cpu). It might end up being that it > makes sense to keep the AIC2 as-is but attach a separate, more > general-purpose global interrupt cuntroller that can route to any > cpu's AIC and that's friendlier to diverse external hardware (like the > type of level interrupts you described) but the hardware team would > know better than me. > > > > If you think any of these things are problems I'll switch back to the > > > conditional version rather than using handle_percpu_irq for > > > everything. > > > > It might be the approach of least surprise, but it won't make a difference > > for the situation described above. > > I'm not seeing any easily measurable performance difference with the > version using the conditional, so I'm going to submit that as a v3. > Whether or not there's actually a safety concern, I'm not sure, but > I'd rather use the functions the way they were intended to be used so > we don't have to worry about unexpected bugs or regressions if the > internals change. > > Rich > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 12, 2016 at 01:34:17PM -0700, Paul E. McKenney wrote: > On Wed, Oct 12, 2016 at 12:35:43PM -0400, Rich Felker wrote: > > On Wed, Oct 12, 2016 at 10:18:02AM +0200, Thomas Gleixner wrote: > > > On Tue, 11 Oct 2016, Rich Felker wrote: > > > > On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote: > > > > > On Sun, 9 Oct 2016, Rich Felker wrote: > > > > > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote: > > > > > > My preference would just be to keep the branch, but with your improved > > > > > > version that doesn't need a function call: > > > > > > > > > > > > irqd_is_per_cpu(irq_desc_get_irq_data(desc)) > > > > > > > > > > > > While there is some overhead testing this condition every time, I can > > > > > > probably come up with several better places to look for a ~10 cycle > > > > > > improvement in the irq code path without imposing new requirements on > > > > > > the DT bindings. > > > > > > > > > > Fair enough. Your call. > > > > > > > > > > > As noted in my followup to the clocksource stall thread, there's also > > > > > > a possibility that it might make sense to consider the current > > > > > > behavior of having non-percpu irqs bound to a particular cpu as part > > > > > > of what's required by the compatible tag, in which case > > > > > > handle_percpu_irq or something similar/equivalent might be suitable > > > > > > for both the percpu and non-percpu cases. I don't understand the irq > > > > > > subsystem well enough to insist on that but I think it's worth > > > > > > consideration since it looks like it would improve performance of > > > > > > non-percpu interrupts a bit. > > > > > > > > > > Well, you can use handle_percpu_irq() for your device interrupts if you > > > > > guarantee at the hardware level that there is no reentrancy. Once you make > > > > > the hardware capable of delivering them on either core the picture changes. > > > > > > > > One more concern here -- I see that handle_simple_irq is handling the > > > > soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff > > > > that's perhaps important too. Since soft-disable is all we have > > > > (there's no hard-disable of interrupts), is this a problem? In other > > > > words, can drivers have an expectation of not receiving interrupts > > > > when the irq is disabled? I would think anything compatible with irq > > > > sharing can't have such an expectation, but perhaps the kernel needs > > > > disabling internally for synchronization at module-unload time or > > > > similar cases? > > > > > > Sure. A driver would be surprised getting an interrupt when it is disabled, > > > but with your exceptionally well thought out interrupt controller a pending > > > (level) interrupt which is not handled will be reraised forever and just > > > hard lock the machine. > > > > If you want to criticize the interrupt controller design (not my work > > or under my control) for limitations in the type of hardware that can > > be hooked up to it, that's okay -- this kind of input will actually be > > useful for designing the next iteration of it -- but I don't think > > this specific possibility is a concern. > > Well, if this scenario does happen, the machine will likely either lock > up silently and hard, give you RCU CPU stall warning messages, or give > you soft-lockup messages. The same situation can happen with badly-behaved hardware under software interrupt control too if it keeps generating interrupts rapidly (more quickly than the cpu can handle them), unless the kernel has some kind of framework for disabling the interrupt and only reenabling it later via a timer. It's equivalent to a realtime-prio process failing to block/sleep to give lower-priority processes a chance to run. Rich -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 12, 2016 at 06:19:27PM -0400, Rich Felker wrote: > On Wed, Oct 12, 2016 at 01:34:17PM -0700, Paul E. McKenney wrote: > > On Wed, Oct 12, 2016 at 12:35:43PM -0400, Rich Felker wrote: > > > On Wed, Oct 12, 2016 at 10:18:02AM +0200, Thomas Gleixner wrote: > > > > On Tue, 11 Oct 2016, Rich Felker wrote: > > > > > On Sun, Oct 09, 2016 at 09:23:58PM +0200, Thomas Gleixner wrote: > > > > > > On Sun, 9 Oct 2016, Rich Felker wrote: > > > > > > > On Sun, Oct 09, 2016 at 01:03:10PM +0200, Thomas Gleixner wrote: > > > > > > > My preference would just be to keep the branch, but with your improved > > > > > > > version that doesn't need a function call: > > > > > > > > > > > > > > irqd_is_per_cpu(irq_desc_get_irq_data(desc)) > > > > > > > > > > > > > > While there is some overhead testing this condition every time, I can > > > > > > > probably come up with several better places to look for a ~10 cycle > > > > > > > improvement in the irq code path without imposing new requirements on > > > > > > > the DT bindings. > > > > > > > > > > > > Fair enough. Your call. > > > > > > > > > > > > > As noted in my followup to the clocksource stall thread, there's also > > > > > > > a possibility that it might make sense to consider the current > > > > > > > behavior of having non-percpu irqs bound to a particular cpu as part > > > > > > > of what's required by the compatible tag, in which case > > > > > > > handle_percpu_irq or something similar/equivalent might be suitable > > > > > > > for both the percpu and non-percpu cases. I don't understand the irq > > > > > > > subsystem well enough to insist on that but I think it's worth > > > > > > > consideration since it looks like it would improve performance of > > > > > > > non-percpu interrupts a bit. > > > > > > > > > > > > Well, you can use handle_percpu_irq() for your device interrupts if you > > > > > > guarantee at the hardware level that there is no reentrancy. Once you make > > > > > > the hardware capable of delivering them on either core the picture changes. > > > > > > > > > > One more concern here -- I see that handle_simple_irq is handling the > > > > > soft-disable / IRQS_PENDING flag behavior, and irq_check_poll stuff > > > > > that's perhaps important too. Since soft-disable is all we have > > > > > (there's no hard-disable of interrupts), is this a problem? In other > > > > > words, can drivers have an expectation of not receiving interrupts > > > > > when the irq is disabled? I would think anything compatible with irq > > > > > sharing can't have such an expectation, but perhaps the kernel needs > > > > > disabling internally for synchronization at module-unload time or > > > > > similar cases? > > > > > > > > Sure. A driver would be surprised getting an interrupt when it is disabled, > > > > but with your exceptionally well thought out interrupt controller a pending > > > > (level) interrupt which is not handled will be reraised forever and just > > > > hard lock the machine. > > > > > > If you want to criticize the interrupt controller design (not my work > > > or under my control) for limitations in the type of hardware that can > > > be hooked up to it, that's okay -- this kind of input will actually be > > > useful for designing the next iteration of it -- but I don't think > > > this specific possibility is a concern. > > > > Well, if this scenario does happen, the machine will likely either lock > > up silently and hard, give you RCU CPU stall warning messages, or give > > you soft-lockup messages. > > The same situation can happen with badly-behaved hardware under > software interrupt control too if it keeps generating interrupts > rapidly (more quickly than the cpu can handle them), unless the kernel > has some kind of framework for disabling the interrupt and only > reenabling it later via a timer. It's equivalent to a realtime-prio > process failing to block/sleep to give lower-priority processes a > chance to run. Indeed, there are quite a few scenarios that can lead to silent hard lockups, RCU CPU stall warning messages, or soft-lockup messages. Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/irqchip/irq-jcore-aic.c b/drivers/irqchip/irq-jcore-aic.c index 5e5e3bb..b53a8a5 100644 --- a/drivers/irqchip/irq-jcore-aic.c +++ b/drivers/irqchip/irq-jcore-aic.c @@ -25,12 +25,20 @@ static struct irq_chip jcore_aic; +static void handle_jcore_irq(struct irq_desc *desc) +{ + if (irq_is_percpu(irq_desc_get_irq(desc))) + handle_percpu_irq(desc); + else + handle_simple_irq(desc); +} + static int jcore_aic_irqdomain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq) { struct irq_chip *aic = d->host_data; - irq_set_chip_and_handler(irq, aic, handle_simple_irq); + irq_set_chip_and_handler(irq, aic, handle_jcore_irq); return 0; }
The J-Core AIC does not have separate interrupt numbers reserved for cpu-local vs global interrupts. Instead, the driver requesting the irq is expected to know whether its device uses per-cpu interrupts or not. Previously it was assumed that handle_simple_irq could work for both cases, but it intentionally drops interrupts for an irq number that already has a handler running. This resulted in the timer interrupt for one cpu being lost when multiple cpus' timers were set for approximately the same expiration time, leading to stalls. In theory the same could also happen with IPIs. To solve the problem, instead of registering handle_simple_irq as the handler, register a wrapper function which checks whether the irq to be handled was requested as per-cpu or not, and passes it to handle_simple_irq or handle_percpu_irq accordingly. Signed-off-by: Rich Felker <dalias@libc.org> --- Ideas for improvement are welcome -- for example the irq_is_percpu(irq_desc_get_irq(desc)) thing looks rather silly but I didn't see a better way without poking through abstractions -- but overall I think this both solves the timer stall issue that I wasted other people's time on, and addresses the concerns about the J-Core AIC driver being oblivious to whether an irq is per-cpu. --- drivers/irqchip/irq-jcore-aic.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)