On Thu, 13 Aug 2020 at 05:38, Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > Hi, > > > On Wed, Aug 12, 2020 at 8:27 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > > > Hi, > > > > On Wed, Aug 12, 2020 at 7:53 AM Sumit Garg <sumit.garg@xxxxxxxxxx> wrote: > > > > > > Hi Doug, > > > > > > On Tue, 11 Aug 2020 at 22:46, Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > > > > > > > Hi, > > > > > > > > On Tue, Aug 11, 2020 at 7:58 AM Greg Kroah-Hartman > > > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote: > > > > > > > > > > On Tue, Aug 11, 2020 at 07:59:24PM +0530, Sumit Garg wrote: > > > > > > Hi Greg, > > > > > > > > > > > > Thanks for your comments. > > > > > > > > > > > > On Tue, 11 Aug 2020 at 19:27, Greg Kroah-Hartman > > > > > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote: > > > > > > > > > > > > > > On Tue, Aug 11, 2020 at 07:20:26PM +0530, Sumit Garg wrote: > > > > > > > > On Tue, 21 Jul 2020 at 17:40, Sumit Garg <sumit.garg@xxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > Make it possible for UARTs to trigger magic sysrq from an NMI. With the > > > > > > > > > advent of pseudo NMIs on arm64 it became quite generic to request serial > > > > > > > > > device interrupt as an NMI rather than IRQ. And having NMI driven serial > > > > > > > > > RX will allow us to trigger magic sysrq as an NMI and hence drop into > > > > > > > > > kernel debugger in NMI context. > > > > > > > > > > > > > > > > > > The major use-case is to add NMI debugging capabilities to the kernel > > > > > > > > > in order to debug scenarios such as: > > > > > > > > > - Primary CPU is stuck in deadlock with interrupts disabled and hence > > > > > > > > > doesn't honor serial device interrupt. So having magic sysrq triggered > > > > > > > > > as an NMI is helpful for debugging. > > > > > > > > > - Always enabled NMI based magic sysrq irrespective of whether the serial > > > > > > > > > TTY port is active or not. > > > > > > > > > > > > > > > > > > Currently there is an existing kgdb NMI serial driver which provides > > > > > > > > > partial implementation in upstream to have a separate ttyNMI0 port but > > > > > > > > > that remained in silos with the serial core/drivers which made it a bit > > > > > > > > > odd to enable using serial device interrupt and hence remained unused. It > > > > > > > > > seems to be clearly intended to avoid almost all custom NMI changes to > > > > > > > > > the UART driver. > > > > > > > > > > > > > > > > > > But this patch-set allows the serial core/drivers to be NMI aware which > > > > > > > > > in turn provides NMI debugging capabilities via magic sysrq and hence > > > > > > > > > there is no specific reason to keep this special driver. So remove it > > > > > > > > > instead. > > > > > > > > > > > > > > > > > > Approach: > > > > > > > > > --------- > > > > > > > > > > > > > > > > > > The overall idea is to intercept serial RX characters in NMI context, if > > > > > > > > > those are specific to magic sysrq then allow corresponding handler to run > > > > > > > > > in NMI context. Otherwise, defer all other RX and TX operations onto IRQ > > > > > > > > > work queue in order to run those in normal interrupt context. > > > > > > > > > > > > > > > > > > This approach is demonstrated using amba-pl011 driver. > > > > > > > > > > > > > > > > > > Patch-wise description: > > > > > > > > > ----------------------- > > > > > > > > > > > > > > > > > > Patch #1 prepares magic sysrq handler to be NMI aware. > > > > > > > > > Patch #2 adds NMI framework to serial core. > > > > > > > > > Patch #3 and #4 demonstrates NMI aware uart port using amba-pl011 driver. > > > > > > > > > Patch #5 removes kgdb NMI serial driver. > > > > > > > > > > > > > > > > > > Goal of this RFC: > > > > > > > > > ----------------- > > > > > > > > > > > > > > > > > > My main reason for sharing this as an RFC is to help decide whether or > > > > > > > > > not to continue with this approach. The next step for me would to port > > > > > > > > > the work to a system with an 8250 UART. > > > > > > > > > > > > > > > > > > > > > > > > > A gentle reminder to seek feedback on this series. > > > > > > > > It's been on my list for a while. I started it Friday but ran out of > > > > time. This week hasn't been going as smoothly as I hoped but I'll > > > > prioritize this since it's been too long. > > > > > > > > > > No worries and thanks for your feedback. > > > > > > > > > > > > > > It's the middle of the merge window, and I can't do anything. > > > > > > > > > > > > > > Also, I almost never review RFC patches as I have have way too many > > > > > > > patches that people think are "right" to review first... > > > > > > > > > > > > > > > > > > > Okay, I understand and I can definitely wait for your feedback. > > > > > > > > > > My feedback here is this: > > > > > > > > > > > > I suggest you work to flesh this out first and submit something that you > > > > > > > feels works properly. > > > > > > > > > > :) > > > > > > > > > > > IIUC, in order to make this approach substantial I need to make it > > > > > > work with 8250 UART (major serial driver), correct? As currently it > > > > > > works properly for amba-pl011 driver. > > > > > > > > > > Yes, try to do that, or better yet, make it work with all serial drivers > > > > > automatically. > > > > > > > > A bit of early feedback... > > > > > > > > Although I'm not sure we can do Greg's "make it work everywhere > > > > automatically", it's possible you could get half of your patch done > > > > automatically. Specifically, your patch really does two things: > > > > > > > > a) It leaves the serial port "active" all the time to look for sysrq. > > > > In other words even if there is no serial client it's always reading > > > > the port looking for characters. IMO this concept should be separated > > > > out from the NMI concept and _could_ automatically work for all serial > > > > drivers. You'd just need something in the serial core that acted like > > > > a default client if nobody else opened the serial port. The nice > > > > thing here is that we go through all the normal code paths and don't > > > > need special cases in the driver. > > > > > > Okay, will try to explore this option to have default serial port > > > client. Would this client be active in normal serial operation or only > > > active when we have kgdb active? One drawback I see for normal > > > operation could be power management as if user is not using serial > > > port and would like to disable corresponding clock in order to reduce > > > power consumption. > > > > If I could pick the ideal, I'd say we'd do it any time the console is > > configured for that port and magic sysrq is enabled. Presumably if > > they're already choosing to output kernel log messages to the serial > > port and they've enabled magic sysrq they're in a state where they'd > > be OK with the extra power of also listening for characters? > > Okay, sounds reasonable to me. > > > > > > b) It enables NMI for your particular serial driver. This seems like > > > > it'd be hard to do automatically because you can't do the same things > > > > at NMI that you could do in a normal interrupt handler. > > > > > > Agree. > > > > > > > > > > > NOTE: to me, a) is more important than b) (though it'd be nice to have > > > > both). This would be especially true the earlier you could make a) > > > > work since the main time when an "agetty" isn't running on my serial > > > > port to read characters is during bootup. > > > > > > > > Why is b) less important to me? Sure, it would let you drop into the > > > > debugger in the case where the CPU handling serial port interrupts is > > > > hung with IRQs disabled, but it _woudln't_ let you drop into the > > > > debugger in the case where a different CPU is hung with IRQs disabled. > > > > To get that we need NMI roundup (which, I know, you are also working > > > > on for arm64). ...and, if we've got NMI roundup, presumably we can > > > > find our way into the debugger by either moving the serial interrupt > > > > to a different CPU ahead of time or using some type of lockup detector > > > > (which I know you are also working on for arm64). > > > > > > > > > > Thanks for sharing your preferences. I will try to get a) sorted out first. > > > > > > Overall I agree with your approaches to debug hard-lockup scenarios > > > but they might not be so trivial for kernel engineers who doesn't > > > posses kernel debugging experience as you do. :) > > > > > > And I still think NMI aware magic sysrq is useful for scenarios such as: > > > - Try to get system information during hard-lockup rather than just > > > panic via hard-lockup detection. > > > - Do normal start/stop debugger activity on a core which was stuck in > > > hard-lockup. > > > - Random boot freezes which are not easily reproducible. > > > > Don't get me wrong. Having sysrq from NMI seems like a good feature > > to me. Yeah I understand what you meant but I was just trying to highlight additional benefits that sysrq from NMI provides. > > That being said, it will require non-trivial changes to each > > serial driver to support it and that means that not all serial drivers > > will support it. It also starts requiring knowledge of how NMIs work > > (what's allowed in NMI mode / not allowed / how to avoid races) for > > authors of serial drivers. I have a bit of a worry that the benefit > > won't outweigh the extra complexity, but I guess time will tell. Yes I understand these concerns as well. That's why I have tried to defer almost all of the work to the IRQ work queue apart from essential parsing of RX chars in NMI mode. Also, I tried to keep most of this code common in serial core. > > One > > last worry is that I assume that most people testing (and even > > automated testing labs) will either always enable NMI or won't enable > > NMI. That means that everyone will be only testing one codepath or > > the other and (given the complexity) the non-tested codepath will > > break. > > The current patch-set only makes this NMI to work when debugger (kgdb) is enabled which I think is mostly suitable for development environments. So most people testing will involve existing IRQ mode only. However, it's very much possible to make NMI mode as default for a particular serial driver if the underlying irqchip supports it but it depends if we really see any production level usage of NMI debug feature. > > Hrm. Along the lines of the above, though: almost no modern systems > > are uniprocessor. That means that even if one CPU is stuck with IRQs > > off it's fairly likely that some other CPU is OK. Presumably you'd > > get almost as much benefit as your patch but with more done > > automatically if you could figure out how to detect that the serial > > interrupt isn't being serviced and re-route it to a different CPU. > > ...or possibly you could use some variant of the hard lockup detector > > and move all interrupts off a locked up CPU? You could make this an > > option that's "default Y" when kgdb is turned on or something? Yes we can reroute serial interrupts but what I meant to say is that we can at most get a backtrace of CPU stuck in hard-lockup whereas with NMI debugger entry on hard-lockup CPU, we can do normal start/stop/single-step debugging on that CPU. > > One other idea occurred to me that's maybe simpler. You could in > theory just poll the serial port periodically to accomplish. It would > actually probably even work to call the normal serial port interrupt > routine from any random CPU. On many serial drivers the entire > interrupt handler is wrapped with: > > spin_lock_irqsave(&uap->port.lock, flags); > ... > spin_unlock_irqrestore(&uap->port.lock, flags); > > And a few (the ones I was involved in fixing) have the similar pattern > of using uart_unlock_and_check_sysrq(). > > Any serial drivers following this pattern could have their interrupt > routine called periodically just to poll for characters and it'd be > fine, right? ...and having it take a second before a sysrq comes in > this case is probably not the end of the world? > Are you proposing to have complete RX operation in polling mode with RX interrupt disabled (eg. using a kernel thread)? > > One nice benefit of this is that it would actually work _better_ on > SMP systems for any sysrqs that aren't NMI safe. Specifically with > your patch series those would be queued with irq_work_queue() which > means they'd be blocked if the CPU processing the NMI is stuck with > IRQs disabled. Yes, the sysrq handlers which aren't NMI safe will behave similarly to existing IRQ based sysrq handlers. > With the polling mechanism they'd nicely just run on a > different CPU. It looks like polling would cause much CPU overhead. So I am not sure if that is the preferred approach. -Sumit > > > > > > One last bit of feedback is that I noticed that you didn't try to > > > > implement the old "knock" functionality of the old NMI driver that's > > > > being deleted. That is: your new patches don't provide an alternate > > > > way to drop into the debugger for systems where BREAK isn't hooked up. > > > > That's not a hard requirement, but I was kinda hoping for it since I > > > > have some systems that haven't routed BREAK properly. ;-) > > > > > > > > > > Yeah, this is on my TODO list to have a kgdb "knock" functionality to > > > be implemented via a common hook in serial core. > > > > > > > > > > > I'll try to get some more detailed feedback in the next few days. > > > > > > Thanks. I do look forward to your feedback. > > > > > > -Sumit > > > > > > > > > > > -Doug
