* [PATCH 0/2] ARC show_regs fixes @ 2018-12-18 18:53 Vineet Gupta 2018-12-18 18:53 ` [PATCH 1/2] ARC: show_regs: avoid page allocator Vineet Gupta 2018-12-18 18:53 ` [PATCH 2/2] ARC: show_regs: fix lockdep splat for good Vineet Gupta 0 siblings, 2 replies; 19+ messages in thread From: Vineet Gupta @ 2018-12-18 18:53 UTC (permalink / raw) To: linux-snps-arc Cc: linux-mm, linux-kernel, linux-arch, Peter Zijlstra, Vineet Gupta Vineet Gupta (2): ARC: show_regs: avoid page allocator ARC: show_regs: fix lockdep splat for good arch/arc/kernel/troubleshoot.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/2] ARC: show_regs: avoid page allocator 2018-12-18 18:53 [PATCH 0/2] ARC show_regs fixes Vineet Gupta @ 2018-12-18 18:53 ` Vineet Gupta 2018-12-19 17:04 ` Eugeniy Paltsev ` (2 more replies) 2018-12-18 18:53 ` [PATCH 2/2] ARC: show_regs: fix lockdep splat for good Vineet Gupta 1 sibling, 3 replies; 19+ messages in thread From: Vineet Gupta @ 2018-12-18 18:53 UTC (permalink / raw) To: linux-snps-arc Cc: linux-mm, linux-kernel, linux-arch, Peter Zijlstra, Vineet Gupta Use on-stack smaller buffers instead of dynamic pages. The motivation for this change was to address lockdep splat when signal handling code calls show_regs (with preemption disabled) and ARC show_regs calls into sleepable page allocator. | potentially unexpected fatal signal 11. | BUG: sleeping function called from invalid context at ../mm/page_alloc.c:4317 | in_atomic(): 1, irqs_disabled(): 0, pid: 57, name: segv | no locks held by segv/57. | Preemption disabled at: | [<8182f17e>] get_signal+0x4a6/0x7c4 | CPU: 0 PID: 57 Comm: segv Not tainted 4.17.0+ #23 | | Stack Trace: | arc_unwind_core.constprop.1+0xd0/0xf4 | __might_sleep+0x1f6/0x234 | __get_free_pages+0x174/0xca0 | show_regs+0x22/0x330 | get_signal+0x4ac/0x7c4 # print_fatal_signals() -> preempt_disable() | do_signal+0x30/0x224 | resume_user_mode_begin+0x90/0xd8 Despite this, lockdep still barfs (see next change), but this patch still has merit as in we use smaller/localized buffers now and there's less instructoh trace to sift thru when debugging pesky issues. Signed-off-by: Vineet Gupta <vgupta@synopsys.com> --- arch/arc/kernel/troubleshoot.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/arch/arc/kernel/troubleshoot.c b/arch/arc/kernel/troubleshoot.c index e8d9fb452346..2885bec71fb8 100644 --- a/arch/arc/kernel/troubleshoot.c +++ b/arch/arc/kernel/troubleshoot.c @@ -58,11 +58,12 @@ static void show_callee_regs(struct callee_regs *cregs) print_reg_file(&(cregs->r13), 13); } -static void print_task_path_n_nm(struct task_struct *tsk, char *buf) +static void print_task_path_n_nm(struct task_struct *tsk) { char *path_nm = NULL; struct mm_struct *mm; struct file *exe_file; + char buf[256]; mm = get_task_mm(tsk); if (!mm) @@ -80,10 +81,9 @@ static void print_task_path_n_nm(struct task_struct *tsk, char *buf) pr_info("Path: %s\n", !IS_ERR(path_nm) ? path_nm : "?"); } -static void show_faulting_vma(unsigned long address, char *buf) +static void show_faulting_vma(unsigned long address) { struct vm_area_struct *vma; - char *nm = buf; struct mm_struct *active_mm = current->active_mm; /* can't use print_vma_addr() yet as it doesn't check for @@ -96,8 +96,11 @@ static void show_faulting_vma(unsigned long address, char *buf) * if the container VMA is not found */ if (vma && (vma->vm_start <= address)) { + char buf[256]; + char *nm = "?"; + if (vma->vm_file) { - nm = file_path(vma->vm_file, buf, PAGE_SIZE - 1); + nm = file_path(vma->vm_file, buf, 256-1); if (IS_ERR(nm)) nm = "?"; } @@ -173,13 +176,8 @@ void show_regs(struct pt_regs *regs) { struct task_struct *tsk = current; struct callee_regs *cregs; - char *buf; - - buf = (char *)__get_free_page(GFP_KERNEL); - if (!buf) - return; - print_task_path_n_nm(tsk, buf); + print_task_path_n_nm(tsk); show_regs_print_info(KERN_INFO); show_ecr_verbose(regs); @@ -189,7 +187,7 @@ void show_regs(struct pt_regs *regs) (void *)regs->blink, (void *)regs->ret); if (user_mode(regs)) - show_faulting_vma(regs->ret, buf); /* faulting code, not data */ + show_faulting_vma(regs->ret); /* faulting code, not data */ pr_info("[STAT32]: 0x%08lx", regs->status32); @@ -221,8 +219,6 @@ void show_regs(struct pt_regs *regs) cregs = (struct callee_regs *)current->thread.callee_reg; if (cregs) show_callee_regs(cregs); - - free_page((unsigned long)buf); } void show_kernel_fault_diag(const char *str, struct pt_regs *regs, -- 2.7.4 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] ARC: show_regs: avoid page allocator 2018-12-18 18:53 ` [PATCH 1/2] ARC: show_regs: avoid page allocator Vineet Gupta @ 2018-12-19 17:04 ` Eugeniy Paltsev 2018-12-19 17:36 ` Vineet Gupta 2018-12-20 1:16 ` Vineet Gupta 2018-12-19 20:46 ` William Kucharski 2018-12-20 12:57 ` Michal Hocko 2 siblings, 2 replies; 19+ messages in thread From: Eugeniy Paltsev @ 2018-12-19 17:04 UTC (permalink / raw) To: vineet.gupta1, linux-snps-arc; +Cc: linux-arch, linux-mm, peterz, linux-kernel Hi Vineet, Just curious: isn't that enough to use GFP_NOWAIT instead of GFP_KERNEL when we allocate page in show_regs()? As I can see x86 use print_vma_addr() in their show_signal_msg() function which allocate page with __get_free_page(GFP_NOWAIT); On Tue, 2018-12-18 at 10:53 -0800, Vineet Gupta wrote: > Use on-stack smaller buffers instead of dynamic pages. > > The motivation for this change was to address lockdep splat when > signal handling code calls show_regs (with preemption disabled) and > ARC show_regs calls into sleepable page allocator. > > > potentially unexpected fatal signal 11. > > BUG: sleeping function called from invalid context at ../mm/page_alloc.c:4317 > > in_atomic(): 1, irqs_disabled(): 0, pid: 57, name: segv > > no locks held by segv/57. > > Preemption disabled at: > > [<8182f17e>] get_signal+0x4a6/0x7c4 > > CPU: 0 PID: 57 Comm: segv Not tainted 4.17.0+ #23 > > > > Stack Trace: > > arc_unwind_core.constprop.1+0xd0/0xf4 > > __might_sleep+0x1f6/0x234 > > __get_free_pages+0x174/0xca0 > > show_regs+0x22/0x330 > > get_signal+0x4ac/0x7c4 # print_fatal_signals() -> preempt_disable() > > do_signal+0x30/0x224 > > resume_user_mode_begin+0x90/0xd8 > > Despite this, lockdep still barfs (see next change), but this patch > still has merit as in we use smaller/localized buffers now and there's > less instructoh trace to sift thru when debugging pesky issues. > > Signed-off-by: Vineet Gupta <vgupta@synopsys.com> > --- > arch/arc/kernel/troubleshoot.c | 22 +++++++++------------- > 1 file changed, 9 insertions(+), 13 deletions(-) > > diff --git a/arch/arc/kernel/troubleshoot.c b/arch/arc/kernel/troubleshoot.c > index e8d9fb452346..2885bec71fb8 100644 > --- a/arch/arc/kernel/troubleshoot.c > +++ b/arch/arc/kernel/troubleshoot.c > @@ -58,11 +58,12 @@ static void show_callee_regs(struct callee_regs *cregs) > print_reg_file(&(cregs->r13), 13); > } > > -static void print_task_path_n_nm(struct task_struct *tsk, char *buf) > +static void print_task_path_n_nm(struct task_struct *tsk) > { > char *path_nm = NULL; > struct mm_struct *mm; > struct file *exe_file; > + char buf[256]; > > mm = get_task_mm(tsk); > if (!mm) > @@ -80,10 +81,9 @@ static void print_task_path_n_nm(struct task_struct *tsk, char *buf) > pr_info("Path: %s\n", !IS_ERR(path_nm) ? path_nm : "?"); > } > > -static void show_faulting_vma(unsigned long address, char *buf) > +static void show_faulting_vma(unsigned long address) > { > struct vm_area_struct *vma; > - char *nm = buf; > struct mm_struct *active_mm = current->active_mm; > > /* can't use print_vma_addr() yet as it doesn't check for > @@ -96,8 +96,11 @@ static void show_faulting_vma(unsigned long address, char *buf) > * if the container VMA is not found > */ > if (vma && (vma->vm_start <= address)) { > + char buf[256]; > + char *nm = "?"; > + > if (vma->vm_file) { > - nm = file_path(vma->vm_file, buf, PAGE_SIZE - 1); > + nm = file_path(vma->vm_file, buf, 256-1); > if (IS_ERR(nm)) > nm = "?"; > } > @@ -173,13 +176,8 @@ void show_regs(struct pt_regs *regs) > { > struct task_struct *tsk = current; > struct callee_regs *cregs; > - char *buf; > - > - buf = (char *)__get_free_page(GFP_KERNEL); > - if (!buf) > - return; > > - print_task_path_n_nm(tsk, buf); > + print_task_path_n_nm(tsk); > show_regs_print_info(KERN_INFO); > > show_ecr_verbose(regs); > @@ -189,7 +187,7 @@ void show_regs(struct pt_regs *regs) > (void *)regs->blink, (void *)regs->ret); > > if (user_mode(regs)) > - show_faulting_vma(regs->ret, buf); /* faulting code, not data */ > + show_faulting_vma(regs->ret); /* faulting code, not data */ > > pr_info("[STAT32]: 0x%08lx", regs->status32); > > @@ -221,8 +219,6 @@ void show_regs(struct pt_regs *regs) > cregs = (struct callee_regs *)current->thread.callee_reg; > if (cregs) > show_callee_regs(cregs); > - > - free_page((unsigned long)buf); > } > > void show_kernel_fault_diag(const char *str, struct pt_regs *regs, -- Eugeniy Paltsev ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] ARC: show_regs: avoid page allocator 2018-12-19 17:04 ` Eugeniy Paltsev @ 2018-12-19 17:36 ` Vineet Gupta 2018-12-20 1:16 ` Vineet Gupta 1 sibling, 0 replies; 19+ messages in thread From: Vineet Gupta @ 2018-12-19 17:36 UTC (permalink / raw) To: Eugeniy Paltsev, linux-snps-arc Cc: linux-arch, linux-mm, peterz, linux-kernel On 12/19/18 9:04 AM, Eugeniy Paltsev wrote: > Just curious: isn't that enough to use GFP_NOWAIT instead > of GFP_KERNEL when we allocate page in show_regs()? > > As I can see x86 use print_vma_addr() in their show_signal_msg() > function which allocate page with __get_free_page(GFP_NOWAIT); I'm not sure if lockdep will be happy with it still. At any rate, as explained in changelog, this still has merit, since the buffer is only needed for nested d_path calls, which are better served with a smaller on-stack buffer. For cases such as kernel crash, we want lesser code/traces in fault path to sift thru ! -Vineet ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] ARC: show_regs: avoid page allocator 2018-12-19 17:04 ` Eugeniy Paltsev 2018-12-19 17:36 ` Vineet Gupta @ 2018-12-20 1:16 ` Vineet Gupta 2018-12-20 13:30 ` Tetsuo Handa 1 sibling, 1 reply; 19+ messages in thread From: Vineet Gupta @ 2018-12-20 1:16 UTC (permalink / raw) To: Eugeniy Paltsev, vineet.gupta1, linux-snps-arc Cc: linux-arch, linux-mm, peterz, linux-kernel On 12/19/18 9:04 AM, Eugeniy Paltsev wrote: > As I can see x86 use print_vma_addr() in their show_signal_msg() > function which allocate page with __get_free_page(GFP_NOWAIT); Indeed with that the __get_free_page() lockdep splat is gone. There's a different one now hence my other patch. | [ARCLinux]# ./segv-null-ptr | potentially unexpected fatal signal 11. | BUG: sleeping function called from invalid context at kernel/fork.c:1011 | in_atomic(): 1, irqs_disabled(): 0, pid: 70, name: segv-null-ptr | no locks held by segv-null-ptr/70. | CPU: 0 PID: 70 Comm: segv-null-ptr Not tainted 4.18.0+ #69 | | Stack Trace: | arc_unwind_core+0xcc/0x100 | ___might_sleep+0x17a/0x190 | mmput+0x16/0xb8 | show_regs+0x52/0x310 | get_signal+0x5ee/0x610 | do_signal+0x2c/0x218 | resume_user_mode_begin+0x90/0xd8 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] ARC: show_regs: avoid page allocator 2018-12-20 1:16 ` Vineet Gupta @ 2018-12-20 13:30 ` Tetsuo Handa 2018-12-20 18:36 ` Vineet Gupta 2018-12-20 18:43 ` Vineet Gupta 0 siblings, 2 replies; 19+ messages in thread From: Tetsuo Handa @ 2018-12-20 13:30 UTC (permalink / raw) To: Vineet Gupta, Eugeniy Paltsev, linux-snps-arc Cc: linux-arch, linux-mm, peterz, linux-kernel, Michal Hocko On 2018/12/20 10:16, Vineet Gupta wrote: > On 12/19/18 9:04 AM, Eugeniy Paltsev wrote: >> As I can see x86 use print_vma_addr() in their show_signal_msg() >> function which allocate page with __get_free_page(GFP_NOWAIT); > > Indeed with that the __get_free_page() lockdep splat is gone. > > There's a different one now hence my other patch. > > | [ARCLinux]# ./segv-null-ptr > | potentially unexpected fatal signal 11. > | BUG: sleeping function called from invalid context at kernel/fork.c:1011 > | in_atomic(): 1, irqs_disabled(): 0, pid: 70, name: segv-null-ptr > | no locks held by segv-null-ptr/70. > | CPU: 0 PID: 70 Comm: segv-null-ptr Not tainted 4.18.0+ #69 > | > | Stack Trace: > | arc_unwind_core+0xcc/0x100 > | ___might_sleep+0x17a/0x190 > | mmput+0x16/0xb8 Then, does mmput_async() help? > | show_regs+0x52/0x310 > | get_signal+0x5ee/0x610 > | do_signal+0x2c/0x218 > | resume_user_mode_begin+0x90/0xd8 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] ARC: show_regs: avoid page allocator 2018-12-20 13:30 ` Tetsuo Handa @ 2018-12-20 18:36 ` Vineet Gupta 2018-12-20 18:43 ` Vineet Gupta 1 sibling, 0 replies; 19+ messages in thread From: Vineet Gupta @ 2018-12-20 18:36 UTC (permalink / raw) To: Tetsuo Handa, Eugeniy Paltsev, linux-snps-arc Cc: linux-arch, linux-mm, peterz, linux-kernel, Michal Hocko On 12/20/18 5:30 AM, Tetsuo Handa wrote: >> | mmput+0x16/0xb8 > Then, does mmput_async() help? Probably, I can try. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] ARC: show_regs: avoid page allocator 2018-12-20 13:30 ` Tetsuo Handa 2018-12-20 18:36 ` Vineet Gupta @ 2018-12-20 18:43 ` Vineet Gupta 1 sibling, 0 replies; 19+ messages in thread From: Vineet Gupta @ 2018-12-20 18:43 UTC (permalink / raw) To: Tetsuo Handa, Eugeniy Paltsev, linux-snps-arc Cc: linux-arch, linux-mm, peterz, linux-kernel, Michal Hocko On 12/20/18 5:30 AM, Tetsuo Handa wrote: >> | Stack Trace: >> | arc_unwind_core+0xcc/0x100 >> | ___might_sleep+0x17a/0x190 >> | mmput+0x16/0xb8 > Then, does mmput_async() help? > It helps, but then we get the next one (w/o my patch 2/2) BUG: sleeping function called from invalid context at kernel/locking/rwsem.c:23 in_atomic(): 1, irqs_disabled(): 0, pid: 69, name: segv-null-ptr no locks held by segv-null-ptr/69. CPU: 0 PID: 69 Comm: segv-null-ptr Not tainted 4.18.0+ #72 Stack Trace: arc_unwind_core+0xcc/0x100 ___might_sleep+0x17a/0x190 down_read+0x18/0x38 show_regs+0x102/0x310 get_signal+0x5ee/0x610 do_signal+0x2c/0x218 resume_user_mode_begin+0x90/0xd8 @off 0x103d4 in [/segv-null-pt ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] ARC: show_regs: avoid page allocator 2018-12-18 18:53 ` [PATCH 1/2] ARC: show_regs: avoid page allocator Vineet Gupta 2018-12-19 17:04 ` Eugeniy Paltsev @ 2018-12-19 20:46 ` William Kucharski 2018-12-19 21:36 ` Vineet Gupta 2018-12-20 12:57 ` Michal Hocko 2 siblings, 1 reply; 19+ messages in thread From: William Kucharski @ 2018-12-19 20:46 UTC (permalink / raw) To: Vineet Gupta Cc: linux-snps-arc, linux-mm, linux-kernel, linux-arch, Peter Zijlstra > On Dec 18, 2018, at 11:53 AM, Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote: > > Use on-stack smaller buffers instead of dynamic pages. > > The motivation for this change was to address lockdep splat when > signal handling code calls show_regs (with preemption disabled) and > ARC show_regs calls into sleepable page allocator. > > | potentially unexpected fatal signal 11. > | BUG: sleeping function called from invalid context at ../mm/page_alloc.c:4317 > | in_atomic(): 1, irqs_disabled(): 0, pid: 57, name: segv > | no locks held by segv/57. > | Preemption disabled at: > | [<8182f17e>] get_signal+0x4a6/0x7c4 > | CPU: 0 PID: 57 Comm: segv Not tainted 4.17.0+ #23 > | > | Stack Trace: > | arc_unwind_core.constprop.1+0xd0/0xf4 > | __might_sleep+0x1f6/0x234 > | __get_free_pages+0x174/0xca0 > | show_regs+0x22/0x330 > | get_signal+0x4ac/0x7c4 # print_fatal_signals() -> preempt_disable() > | do_signal+0x30/0x224 > | resume_user_mode_begin+0x90/0xd8 > > Despite this, lockdep still barfs (see next change), but this patch > still has merit as in we use smaller/localized buffers now and there's > less instructoh trace to sift thru when debugging pesky issues. > > Signed-off-by: Vineet Gupta <vgupta@synopsys.com> I would rather see 256 as a #define somewhere rather than a magic number sprinkled around arch/arc/kernel/troubleshoot.c. Still, that's what the existing code does, so I suppose it's OK. Otherwise the change looks good. Reviewed-by: William Kucharski <william.kucharski@oracle.com> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] ARC: show_regs: avoid page allocator 2018-12-19 20:46 ` William Kucharski @ 2018-12-19 21:36 ` Vineet Gupta 0 siblings, 0 replies; 19+ messages in thread From: Vineet Gupta @ 2018-12-19 21:36 UTC (permalink / raw) To: William Kucharski Cc: linux-arch, linux-mm, Peter Zijlstra, linux-snps-arc, linux-kernel On 12/19/18 12:46 PM, William Kucharski wrote: > I would rather see 256 as a #define somewhere rather than a magic number sprinkled > around arch/arc/kernel/troubleshoot.c. That bothered me as well, but I was too lazy to define one and the existing ones don't apply. PATH_MAX is 4K which will blow up the stack usage. > > Still, that's what the existing code does, so I suppose it's OK. I'll define one locally. > Otherwise the change looks good. Thx for taking a look. > Reviewed-by: William Kucharski <william.kucharski@oracle.com> I'll add this to the patch. Thx, -Vineet ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] ARC: show_regs: avoid page allocator 2018-12-18 18:53 ` [PATCH 1/2] ARC: show_regs: avoid page allocator Vineet Gupta 2018-12-19 17:04 ` Eugeniy Paltsev 2018-12-19 20:46 ` William Kucharski @ 2018-12-20 12:57 ` Michal Hocko 2018-12-20 18:38 ` Vineet Gupta 2 siblings, 1 reply; 19+ messages in thread From: Michal Hocko @ 2018-12-20 12:57 UTC (permalink / raw) To: Vineet Gupta Cc: linux-snps-arc, linux-mm, linux-kernel, linux-arch, Peter Zijlstra On Tue 18-12-18 10:53:58, Vineet Gupta wrote: > Use on-stack smaller buffers instead of dynamic pages. > > The motivation for this change was to address lockdep splat when > signal handling code calls show_regs (with preemption disabled) and > ARC show_regs calls into sleepable page allocator. > > | potentially unexpected fatal signal 11. > | BUG: sleeping function called from invalid context at ../mm/page_alloc.c:4317 > | in_atomic(): 1, irqs_disabled(): 0, pid: 57, name: segv > | no locks held by segv/57. > | Preemption disabled at: > | [<8182f17e>] get_signal+0x4a6/0x7c4 > | CPU: 0 PID: 57 Comm: segv Not tainted 4.17.0+ #23 > | > | Stack Trace: > | arc_unwind_core.constprop.1+0xd0/0xf4 > | __might_sleep+0x1f6/0x234 > | __get_free_pages+0x174/0xca0 > | show_regs+0x22/0x330 > | get_signal+0x4ac/0x7c4 # print_fatal_signals() -> preempt_disable() > | do_signal+0x30/0x224 > | resume_user_mode_begin+0x90/0xd8 > > Despite this, lockdep still barfs (see next change), but this patch > still has merit as in we use smaller/localized buffers now and there's > less instructoh trace to sift thru when debugging pesky issues. But show_regs is called from contexts which might be called from deep call chains (e.g WARN). Is it safe to allocate such a large stack there? -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] ARC: show_regs: avoid page allocator 2018-12-20 12:57 ` Michal Hocko @ 2018-12-20 18:38 ` Vineet Gupta 0 siblings, 0 replies; 19+ messages in thread From: Vineet Gupta @ 2018-12-20 18:38 UTC (permalink / raw) To: Michal Hocko Cc: linux-snps-arc, linux-mm, linux-kernel, linux-arch, Peter Zijlstra On 12/20/18 4:57 AM, Michal Hocko wrote: >> Despite this, lockdep still barfs (see next change), but this patch >> still has merit as in we use smaller/localized buffers now and there's >> less instructoh trace to sift thru when debugging pesky issues. > But show_regs is called from contexts which might be called from deep > call chains (e.g WARN). Is it safe to allocate such a large stack there? ARC has 8K pages and 256 additional bytes of stack usage doesn't seem absurdly high to me ! -Vineet ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/2] ARC: show_regs: fix lockdep splat for good 2018-12-18 18:53 [PATCH 0/2] ARC show_regs fixes Vineet Gupta 2018-12-18 18:53 ` [PATCH 1/2] ARC: show_regs: avoid page allocator Vineet Gupta @ 2018-12-18 18:53 ` Vineet Gupta 2018-12-20 13:04 ` Michal Hocko 1 sibling, 1 reply; 19+ messages in thread From: Vineet Gupta @ 2018-12-18 18:53 UTC (permalink / raw) To: linux-snps-arc Cc: linux-mm, linux-kernel, linux-arch, Peter Zijlstra, Vineet Gupta signal handling core calls ARCH show_regs() with preemption disabled which causes __might_sleep functions such as mmput leading to lockdep splat. Workaround by re-enabling preemption temporarily. This may not be as bad as it sounds since the preemption disabling itself was introduced for a supressing smp_processor_id() warning in x86 code by commit 3a9f84d354ce ("signals, debug: fix BUG: using smp_processor_id() in preemptible code in print_fatal_signal()") Signed-off-by: Vineet Gupta <vgupta@synopsys.com> --- arch/arc/kernel/troubleshoot.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/arch/arc/kernel/troubleshoot.c b/arch/arc/kernel/troubleshoot.c index 2885bec71fb8..c650d3de13e1 100644 --- a/arch/arc/kernel/troubleshoot.c +++ b/arch/arc/kernel/troubleshoot.c @@ -177,6 +177,12 @@ void show_regs(struct pt_regs *regs) struct task_struct *tsk = current; struct callee_regs *cregs; + /* + * generic code calls us with preemption disabled, but some calls + * here could sleep, so re-enable to avoid lockdep splat + */ + preempt_enable(); + print_task_path_n_nm(tsk); show_regs_print_info(KERN_INFO); @@ -219,6 +225,8 @@ void show_regs(struct pt_regs *regs) cregs = (struct callee_regs *)current->thread.callee_reg; if (cregs) show_callee_regs(cregs); + + preempt_disable(); } void show_kernel_fault_diag(const char *str, struct pt_regs *regs, -- 2.7.4 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] ARC: show_regs: fix lockdep splat for good 2018-12-18 18:53 ` [PATCH 2/2] ARC: show_regs: fix lockdep splat for good Vineet Gupta @ 2018-12-20 13:04 ` Michal Hocko 2018-12-20 18:45 ` Vineet Gupta 0 siblings, 1 reply; 19+ messages in thread From: Michal Hocko @ 2018-12-20 13:04 UTC (permalink / raw) To: Vineet Gupta Cc: linux-snps-arc, linux-mm, linux-kernel, linux-arch, Peter Zijlstra On Tue 18-12-18 10:53:59, Vineet Gupta wrote: > signal handling core calls ARCH show_regs() with preemption disabled > which causes __might_sleep functions such as mmput leading to lockdep > splat. Workaround by re-enabling preemption temporarily. > > This may not be as bad as it sounds since the preemption disabling > itself was introduced for a supressing smp_processor_id() warning in x86 > code by commit 3a9f84d354ce ("signals, debug: fix BUG: using > smp_processor_id() in preemptible code in print_fatal_signal()") The commit you are referring to here sounds dubious in itself. We do not want to stick a preempt_disable just to silence a warning. show_regs is called from preemptible context at several places (e.g. __warn). Maybe this was not the case in 2009 when the change was introduced but this seems like a relict from the past. So can we fix the actual problem rather than build on top of it instead? Or maybe I am just missing something here. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] ARC: show_regs: fix lockdep splat for good 2018-12-20 13:04 ` Michal Hocko @ 2018-12-20 18:45 ` Vineet Gupta 2018-12-21 13:04 ` Michal Hocko 0 siblings, 1 reply; 19+ messages in thread From: Vineet Gupta @ 2018-12-20 18:45 UTC (permalink / raw) To: Michal Hocko Cc: linux-snps-arc, linux-mm, linux-kernel, linux-arch, Peter Zijlstra On 12/20/18 5:04 AM, Michal Hocko wrote: > On Tue 18-12-18 10:53:59, Vineet Gupta wrote: >> signal handling core calls ARCH show_regs() with preemption disabled >> which causes __might_sleep functions such as mmput leading to lockdep >> splat. Workaround by re-enabling preemption temporarily. >> >> This may not be as bad as it sounds since the preemption disabling >> itself was introduced for a supressing smp_processor_id() warning in x86 >> code by commit 3a9f84d354ce ("signals, debug: fix BUG: using >> smp_processor_id() in preemptible code in print_fatal_signal()") > The commit you are referring to here sounds dubious in itself. Indeed that was my thought as well, but it did introduce the preemption disabling logic aroung core calling show_regs() ! > We do not > want to stick a preempt_disable just to silence a warning. I presume you are referring to original commit, not my anti-change in ARC code, which is actually re-enabling it. > show_regs is > called from preemptible context at several places (e.g. __warn). Right, but do we have other reports which show this, perhaps not too many distros have CONFIG__PREEMPT enabled ? > Maybe > this was not the case in 2009 when the change was introduced but this > seems like a relict from the past. So can we fix the actual problem > rather than build on top of it instead? The best/correct fix is to remove the preempt diabling in core code, but that affects every arch out there and will likely trip dormant land mines, needed localized fixes like I'm dealing with now. -Vineet ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] ARC: show_regs: fix lockdep splat for good 2018-12-20 18:45 ` Vineet Gupta @ 2018-12-21 13:04 ` Michal Hocko 2018-12-21 13:27 ` Michal Hocko 2018-12-21 17:55 ` Vineet Gupta 0 siblings, 2 replies; 19+ messages in thread From: Michal Hocko @ 2018-12-21 13:04 UTC (permalink / raw) To: Vineet Gupta Cc: linux-snps-arc, linux-mm, linux-kernel, linux-arch, Peter Zijlstra On Thu 20-12-18 18:45:48, Vineet Gupta wrote: > On 12/20/18 5:04 AM, Michal Hocko wrote: > > On Tue 18-12-18 10:53:59, Vineet Gupta wrote: > >> signal handling core calls ARCH show_regs() with preemption disabled > >> which causes __might_sleep functions such as mmput leading to lockdep > >> splat. Workaround by re-enabling preemption temporarily. > >> > >> This may not be as bad as it sounds since the preemption disabling > >> itself was introduced for a supressing smp_processor_id() warning in x86 > >> code by commit 3a9f84d354ce ("signals, debug: fix BUG: using > >> smp_processor_id() in preemptible code in print_fatal_signal()") > > The commit you are referring to here sounds dubious in itself. > > Indeed that was my thought as well, but it did introduce the preemption disabling > logic aroung core calling show_regs() ! > > > We do not > > want to stick a preempt_disable just to silence a warning. > > I presume you are referring to original commit, not my anti-change in ARC code, > which is actually re-enabling it. Yes, but you are building on a broken concept I believe. What implications does re-enabling really have? Now you could reschedule and you can move to another CPU. Is this really safe? I believe that yes because the preemption disabling is simply bogus. Which doesn't sound like a proper justification, does it? > > show_regs is > > called from preemptible context at several places (e.g. __warn). > > Right, but do we have other reports which show this, perhaps not too many distros > have CONFIG__PREEMPT enabled ? I do not follow. If there is some path to require show_regs to run with preemption disabled while others don't then something is clearly wrong. > > Maybe > > this was not the case in 2009 when the change was introduced but this > > seems like a relict from the past. So can we fix the actual problem > > rather than build on top of it instead? > > The best/correct fix is to remove the preempt diabling in core code, but that > affects every arch out there and will likely trip dormant land mines, needed > localized fixes like I'm dealing with now. Yes, the fix might be more involved but I would much rather prefer a correct code which builds on solid assumptions. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] ARC: show_regs: fix lockdep splat for good 2018-12-21 13:04 ` Michal Hocko @ 2018-12-21 13:27 ` Michal Hocko 2018-12-21 17:55 ` Vineet Gupta 1 sibling, 0 replies; 19+ messages in thread From: Michal Hocko @ 2018-12-21 13:27 UTC (permalink / raw) To: Vineet Gupta Cc: linux-snps-arc, linux-mm, linux-kernel, linux-arch, Peter Zijlstra On Fri 21-12-18 14:04:04, Michal Hocko wrote: [...] > Yes, but you are building on a broken concept I believe. What > implications does re-enabling really have? Now you could reschedule and > you can move to another CPU. Is this really safe? I believe that yes > because the preemption disabling is simply bogus. Which doesn't sound > like a proper justification, does it? Well, thinking about it a bit more. What is the result of calling preempt_enable outside of preempt_disabled section? E.g. __warn which doesn't disable preemption AFAICS. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] ARC: show_regs: fix lockdep splat for good 2018-12-21 13:04 ` Michal Hocko 2018-12-21 13:27 ` Michal Hocko @ 2018-12-21 17:55 ` Vineet Gupta 2018-12-24 19:10 ` Michal Hocko 1 sibling, 1 reply; 19+ messages in thread From: Vineet Gupta @ 2018-12-21 17:55 UTC (permalink / raw) To: Michal Hocko Cc: linux-snps-arc, linux-mm, linux-kernel, linux-arch, Peter Zijlstra On 12/21/18 5:04 AM, Michal Hocko wrote: >> I presume you are referring to original commit, not my anti-change in ARC code, >> which is actually re-enabling it. > > Yes, but you are building on a broken concept I believe. Not sure where this is heading. Broken concept was introduced by disabling preemption around show_regs() to silence x86 smp_processor_id() splat in 2009. > What > implications does re-enabling really have ? Now you could reschedule and> you can move to another CPU. Is this really safe? From initial testing, none so far. show_regs() is simply pretty printing the passed pt_regs and decoding the current task, which agreed could move to a different CPU (likely will due to console/printk calls), but I don't see how that could mess up its mm or othe rinternal plumbing which it prints. > I believe that yes > because the preemption disabling is simply bogus. Which doesn't sound > like a proper justification, does it? [snip] > I do not follow. If there is some path to require show_regs to run with > preemption disabled while others don't then something is clearly wrong. [snip] > Yes, the fix might be more involved but I would much rather prefer a > correct code which builds on solid assumptions. Right so the first step is reverting the disabled semantics for ARC and do some heavy testing to make sure any fallouts are addressed etc. And if that works, then propagate this change to core itself. Low risk strategy IMO - agree ? Thx, -Vineet ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] ARC: show_regs: fix lockdep splat for good 2018-12-21 17:55 ` Vineet Gupta @ 2018-12-24 19:10 ` Michal Hocko 0 siblings, 0 replies; 19+ messages in thread From: Michal Hocko @ 2018-12-24 19:10 UTC (permalink / raw) To: Vineet Gupta Cc: linux-snps-arc, linux-mm, linux-kernel, linux-arch, Peter Zijlstra On Fri 21-12-18 09:55:34, Vineet Gupta wrote: > On 12/21/18 5:04 AM, Michal Hocko wrote: [...] > > Yes, the fix might be more involved but I would much rather prefer a > > correct code which builds on solid assumptions. > > Right so the first step is reverting the disabled semantics for ARC and do some > heavy testing to make sure any fallouts are addressed etc. And if that works, then > propagate this change to core itself. Low risk strategy IMO - agree ? Yeah, I would simply remove the preempt_disable and see what falls out. smp_processor_id could be converted to the raw version etc... -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-12-24 19:10 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-18 18:53 [PATCH 0/2] ARC show_regs fixes Vineet Gupta 2018-12-18 18:53 ` [PATCH 1/2] ARC: show_regs: avoid page allocator Vineet Gupta 2018-12-19 17:04 ` Eugeniy Paltsev 2018-12-19 17:36 ` Vineet Gupta 2018-12-20 1:16 ` Vineet Gupta 2018-12-20 13:30 ` Tetsuo Handa 2018-12-20 18:36 ` Vineet Gupta 2018-12-20 18:43 ` Vineet Gupta 2018-12-19 20:46 ` William Kucharski 2018-12-19 21:36 ` Vineet Gupta 2018-12-20 12:57 ` Michal Hocko 2018-12-20 18:38 ` Vineet Gupta 2018-12-18 18:53 ` [PATCH 2/2] ARC: show_regs: fix lockdep splat for good Vineet Gupta 2018-12-20 13:04 ` Michal Hocko 2018-12-20 18:45 ` Vineet Gupta 2018-12-21 13:04 ` Michal Hocko 2018-12-21 13:27 ` Michal Hocko 2018-12-21 17:55 ` Vineet Gupta 2018-12-24 19:10 ` Michal Hocko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).