| Message ID | 20160808135711.GA13300@pathway.suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 8/8/2016 9:57 AM, Petr Mladek wrote: > On Thu 2016-07-14 16:50:29, Chris Metcalf wrote: >> Currently you can only request a backtrace of either all cpus, or >> all cpus but yourself. It can also be helpful to request a remote >> backtrace of a single cpu, and since we want that, the logical >> extension is to support a cpumask as the underlying primitive. >> >> This change modifies the existing lib/nmi_backtrace.c code to take >> a cpumask as its basic primitive, and modifies the linux/nmi.h code >> to use either the old "all/all_but_self" arch methods, or the new >> "cpumask" method, depending on which is available. > I triggered this function using > echo l >/proc/sysrq-trigger > > > and got > > [ 270.791328] ----------- All but itself: --------------------- > > [ 270.791331] =============================== > [ 270.791331] [ INFO: suspicious RCU usage. ] > [ 270.791333] 4.8.0-rc1-4-default+ #3086 Not tainted > [ 270.791333] ------------------------------- > [ 270.791335] ./include/linux/rcupdate.h:556 Illegal context switch in RCU read-side critical section! Ah hah, you tested this with CPUMASK_OFFSTACK, which I didn't. That explains why you got RCU kmalloc warnings. >> + cpumask_copy(mask, cpu_online_mask); >> + cpumask_clear_cpu(cpu, mask); >> + arch_trigger_cpumask_backtrace(mask); >> + put_cpu(); >> + free_cpumask_var(mask); >> + return true; > Also this looks too much code for an inlined function. > It is rather slow and there is not a big gain. I would move > the definition to lib/nmi_backtrace.c. After some thought, I ended up just removing both cpumask allocation sites. For the allbutself() case, I just re-introduced the "include_self" boolean that the code used to have. If it is false when we get into the inner nmi_trigger_cpumask_backtrace(), I just clear the cpu bit of the current cpu. It requires passing a funny boolean around with the mask, but the alternative (if we don't want to allocate a mask on this path) is to break apart the nmi_trigger_cpumask_backtrace() function so we can piggy-back on its locking and its cpumask and set up the cpumask the way we want, which I think is too much added ugliness. For the trigger_single_cpu_backtrace() case, I remembered that there was a cpumask_of() function that we can use that is fast and doesn't allocate, even with CPUMASK_OFFSTACK, so I just used that instead. > PS: I am sorry for sending this so late in the game. I was > curious why the patch had not been upstream yet and. I made > a closer look to give a Reviewed-by tag... No worries - even a late review is much better than none! I'll send v7 shortly and please do let me know if it works for you.
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c index 52bbd27e93ae..404a32699554 100644 --- a/drivers/tty/sysrq.c +++ b/drivers/tty/sysrq.c @@ -242,6 +242,7 @@ static void sysrq_handle_showallcpus(int key) * backtrace printing did not succeed or the * architecture has no support for it: */ + printk("----------- All CPUs: ---------------------\n"); if (!trigger_all_cpu_backtrace()) { struct pt_regs *regs = get_irq_regs(); @@ -251,6 +252,10 @@ static void sysrq_handle_showallcpus(int key) } schedule_work(&sysrq_showallcpus); } + printk("----------- All but itself: ---------------------\n"); + trigger_allbutself_cpu_backtrace(); + printk("----------- Only two: ---------------------\n"); + trigger_single_cpu_backtrace(2); } static struct sysrq_key_op sysrq_showallcpus_op = {