| Message ID | 7bfb7ffad5fe384fe3dbe6c8ca985f35b98b48bb.1737511963.git.jpoimboe@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | unwind, perf: sframe user space unwinding | expand |
On 22.01.2025 03:31, Josh Poimboeuf wrote: > When debugging sframe issues, the error messages aren't all that helpful > without knowing what file a corresponding .sframe section belongs to. > Prefix debug output strings with the file name. > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > diff --git a/kernel/unwind/sframe_debug.h b/kernel/unwind/sframe_debug.h > +static inline void dbg_init(struct sframe_section *sec) > +{ > + struct mm_struct *mm = current->mm; > + struct vm_area_struct *vma; > + > + guard(mmap_read_lock)(mm); > + vma = vma_lookup(mm, sec->sframe_start); > + if (!vma) > + sec->filename = kstrdup("(vma gone???)", GFP_KERNEL); > + else if (vma->vm_file) > + sec->filename = kstrdup_quotable_file(vma->vm_file, GFP_KERNEL); > + else if (!vma->vm_mm) This condition does not appear to work for vdso on s390. The following does: else if (in_range(sec->sframe_start, current->mm->context.vdso_base, vdso_text_size())) > + sec->filename = kstrdup("(vdso)", GFP_KERNEL); > + else > + sec->filename = kstrdup("(anonymous)", GFP_KERNEL); > +} Regards, Jens
On Thu, Jan 30, 2025 at 05:17:33PM +0100, Jens Remus wrote: > On 22.01.2025 03:31, Josh Poimboeuf wrote: > > When debugging sframe issues, the error messages aren't all that helpful > > without knowing what file a corresponding .sframe section belongs to. > > Prefix debug output strings with the file name. > > > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > > > diff --git a/kernel/unwind/sframe_debug.h b/kernel/unwind/sframe_debug.h > > > +static inline void dbg_init(struct sframe_section *sec) > > +{ > > + struct mm_struct *mm = current->mm; > > + struct vm_area_struct *vma; > > + > > + guard(mmap_read_lock)(mm); > > + vma = vma_lookup(mm, sec->sframe_start); > > + if (!vma) > > + sec->filename = kstrdup("(vma gone???)", GFP_KERNEL); > > + else if (vma->vm_file) > > + sec->filename = kstrdup_quotable_file(vma->vm_file, GFP_KERNEL); > > + else if (!vma->vm_mm) > > This condition does not appear to work for vdso on s390. I had a fix for this which somehow got dropped: diff --git a/kernel/unwind/sframe_debug.h b/kernel/unwind/sframe_debug.h index 3bb3c5574aee..045e9c0b16c9 100644 --- a/kernel/unwind/sframe_debug.h +++ b/kernel/unwind/sframe_debug.h @@ -67,6 +67,10 @@ static inline void dbg_init(struct sframe_section *sec) sec->filename = kstrdup("(vma gone???)", GFP_KERNEL); else if (vma->vm_file) sec->filename = kstrdup_quotable_file(vma->vm_file, GFP_KERNEL); + else if (vma->vm_ops && vma->vm_ops->name) + sec->filename = kstrdup(vma->vm_ops->name(vma), GFP_KERNEL); + else if (arch_vma_name(vma)) + sec->filename = kstrdup(arch_vma_name(vma), GFP_KERNEL); else if (!vma->vm_mm) sec->filename = kstrdup("(vdso)", GFP_KERNEL); else
On 04.02.2025 20:10, Josh Poimboeuf wrote: > On Thu, Jan 30, 2025 at 05:17:33PM +0100, Jens Remus wrote: >> On 22.01.2025 03:31, Josh Poimboeuf wrote: >>> diff --git a/kernel/unwind/sframe_debug.h b/kernel/unwind/sframe_debug.h >> >>> +static inline void dbg_init(struct sframe_section *sec) >>> +{ >>> + struct mm_struct *mm = current->mm; >>> + struct vm_area_struct *vma; >>> + >>> + guard(mmap_read_lock)(mm); >>> + vma = vma_lookup(mm, sec->sframe_start); >>> + if (!vma) >>> + sec->filename = kstrdup("(vma gone???)", GFP_KERNEL); >>> + else if (vma->vm_file) >>> + sec->filename = kstrdup_quotable_file(vma->vm_file, GFP_KERNEL); >>> + else if (!vma->vm_mm) >> >> This condition does not appear to work for vdso on s390. > > I had a fix for this which somehow got dropped: > > diff --git a/kernel/unwind/sframe_debug.h b/kernel/unwind/sframe_debug.h > index 3bb3c5574aee..045e9c0b16c9 100644 > --- a/kernel/unwind/sframe_debug.h > +++ b/kernel/unwind/sframe_debug.h > @@ -67,6 +67,10 @@ static inline void dbg_init(struct sframe_section *sec) > sec->filename = kstrdup("(vma gone???)", GFP_KERNEL); > else if (vma->vm_file) > sec->filename = kstrdup_quotable_file(vma->vm_file, GFP_KERNEL); > + else if (vma->vm_ops && vma->vm_ops->name) > + sec->filename = kstrdup(vma->vm_ops->name(vma), GFP_KERNEL); > + else if (arch_vma_name(vma)) > + sec->filename = kstrdup(arch_vma_name(vma), GFP_KERNEL); > else if (!vma->vm_mm) > sec->filename = kstrdup("(vdso)", GFP_KERNEL); > else Thanks! Your patch works fine on s390: test_vdso 1087 104.721004: 10000 task-clock:ppp: 3ffd11f1702 __cvdso_clock_gettime_data.constprop.0+0x52 ([vdso]) 3ffd11f14c6 __kernel_clock_gettime+0x16 ([vdso]) 3ff8f6f113e clock_gettime@@GLIBC_2.17+0x1e (/usr/lib64/libc.so.6) 3ff8f6e20e8 __gettimeofday+0x38 (/usr/lib64/libc.so.6) 100066e main+0x1e (/root/test/vdso/test_vdso) 3ff8f63459c __libc_start_call_main+0x8c (/usr/lib64/libc.so.6) 3ff8f63469e __libc_start_main@@GLIBC_2.34+0xae (/usr/lib64/libc.so.6) 100073a _start+0x3a (/root/test/vdso/test_vdso) Regards, Jens
diff --git a/include/linux/sframe.h b/include/linux/sframe.h index 2e70085a1e89..18a04d574090 100644 --- a/include/linux/sframe.h +++ b/include/linux/sframe.h @@ -10,7 +10,9 @@ struct sframe_section { struct rcu_head rcu; - +#ifdef CONFIG_DYNAMIC_DEBUG + const char *filename; +#endif unsigned long sframe_start; unsigned long sframe_end; unsigned long text_start; diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c index 66b920441692..f463123f9afe 100644 --- a/kernel/unwind/sframe.c +++ b/kernel/unwind/sframe.c @@ -295,14 +295,17 @@ int sframe_find(unsigned long ip, struct unwind_user_frame *frame) end: user_read_access_end(); - if (ret == -EFAULT) + if (ret == -EFAULT) { + dbg_sec("removing bad .sframe section\n"); WARN_ON_ONCE(sframe_remove_section(sec->sframe_start)); + } return ret; } static void free_section(struct sframe_section *sec) { + dbg_free(sec); kfree(sec); } @@ -313,7 +316,7 @@ static int sframe_read_header(struct sframe_section *sec) unsigned int num_fdes; if (copy_from_user(&shdr, (void __user *)sec->sframe_start, sizeof(shdr))) { - dbg("header usercopy failed\n"); + dbg_sec("header usercopy failed\n"); return -EFAULT; } @@ -321,18 +324,18 @@ static int sframe_read_header(struct sframe_section *sec) shdr.preamble.version != SFRAME_VERSION_2 || !(shdr.preamble.flags & SFRAME_F_FDE_SORTED) || shdr.auxhdr_len) { - dbg("bad/unsupported sframe header\n"); + dbg_sec("bad/unsupported sframe header\n"); return -EINVAL; } if (!shdr.num_fdes || !shdr.num_fres) { - dbg("no fde/fre entries\n"); + dbg_sec("no fde/fre entries\n"); return -EINVAL; } header_end = sec->sframe_start + SFRAME_HEADER_SIZE(shdr); if (header_end >= sec->sframe_end) { - dbg("header doesn't fit in section\n"); + dbg_sec("header doesn't fit in section\n"); return -EINVAL; } @@ -344,7 +347,7 @@ static int sframe_read_header(struct sframe_section *sec) fres_end = fres_start + shdr.fre_len; if (fres_start < fdes_end || fres_end > sec->sframe_end) { - dbg("inconsistent fde/fre offsets\n"); + dbg_sec("inconsistent fde/fre offsets\n"); return -EINVAL; } @@ -400,6 +403,8 @@ int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end, sec->text_start = text_start; sec->text_end = text_end; + dbg_init(sec); + ret = sframe_read_header(sec); if (ret) { dbg_print_header(sec); @@ -408,8 +413,8 @@ int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end, ret = mtree_insert_range(sframe_mt, sec->text_start, sec->text_end, sec, GFP_KERNEL); if (ret) { - dbg("mtree_insert_range failed: text=%lx-%lx\n", - sec->text_start, sec->text_end); + dbg_sec("mtree_insert_range failed: text=%lx-%lx\n", + sec->text_start, sec->text_end); goto err_free; } @@ -431,7 +436,7 @@ static int __sframe_remove_section(struct mm_struct *mm, struct sframe_section *sec) { if (!mtree_erase(&mm->sframe_mt, sec->text_start)) { - dbg("mtree_erase failed: text=%lx\n", sec->text_start); + dbg_sec("mtree_erase failed: text=%lx\n", sec->text_start); return -EINVAL; } diff --git a/kernel/unwind/sframe_debug.h b/kernel/unwind/sframe_debug.h index 055c8c8fae24..4d121cdbb760 100644 --- a/kernel/unwind/sframe_debug.h +++ b/kernel/unwind/sframe_debug.h @@ -10,26 +10,55 @@ #define dbg(fmt, ...) \ pr_debug("%s (%d): " fmt, current->comm, current->pid, ##__VA_ARGS__) +#define dbg_sec(fmt, ...) \ + dbg("%s: " fmt, sec->filename, ##__VA_ARGS__) + static __always_inline void dbg_print_header(struct sframe_section *sec) { unsigned long fdes_end; fdes_end = sec->fdes_start + (sec->num_fdes * sizeof(struct sframe_fde)); - dbg("SEC: sframe:0x%lx-0x%lx text:0x%lx-0x%lx " - "fdes:0x%lx-0x%lx fres:0x%lx-0x%lx " - "ra_off:%d fp_off:%d\n", - sec->sframe_start, sec->sframe_end, sec->text_start, sec->text_end, - sec->fdes_start, fdes_end, sec->fres_start, sec->fres_end, - sec->ra_off, sec->fp_off); + dbg_sec("SEC: sframe:0x%lx-0x%lx text:0x%lx-0x%lx " + "fdes:0x%lx-0x%lx fres:0x%lx-0x%lx " + "ra_off:%d fp_off:%d\n", + sec->sframe_start, sec->sframe_end, sec->text_start, sec->text_end, + sec->fdes_start, fdes_end, sec->fres_start, sec->fres_end, + sec->ra_off, sec->fp_off); +} + +static inline void dbg_init(struct sframe_section *sec) +{ + struct mm_struct *mm = current->mm; + struct vm_area_struct *vma; + + guard(mmap_read_lock)(mm); + vma = vma_lookup(mm, sec->sframe_start); + if (!vma) + sec->filename = kstrdup("(vma gone???)", GFP_KERNEL); + else if (vma->vm_file) + sec->filename = kstrdup_quotable_file(vma->vm_file, GFP_KERNEL); + else if (!vma->vm_mm) + sec->filename = kstrdup("(vdso)", GFP_KERNEL); + else + sec->filename = kstrdup("(anonymous)", GFP_KERNEL); +} + +static inline void dbg_free(struct sframe_section *sec) +{ + kfree(sec->filename); } #else /* !CONFIG_DYNAMIC_DEBUG */ #define dbg(args...) no_printk(args) +#define dbg_sec(args... ) no_printk(args) static inline void dbg_print_header(struct sframe_section *sec) {} +static inline void dbg_init(struct sframe_section *sec) {} +static inline void dbg_free(struct sframe_section *sec) {} + #endif /* !CONFIG_DYNAMIC_DEBUG */ #endif /* _SFRAME_DEBUG_H */
When debugging sframe issues, the error messages aren't all that helpful without knowing what file a corresponding .sframe section belongs to. Prefix debug output strings with the file name. Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> --- include/linux/sframe.h | 4 +++- kernel/unwind/sframe.c | 23 ++++++++++++-------- kernel/unwind/sframe_debug.h | 41 ++++++++++++++++++++++++++++++------ 3 files changed, 52 insertions(+), 16 deletions(-)