tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head: 3123109284176b1532874591f7c81f3837bbdc17
commit: d31ed5d767c0452b4f49846d80a0bfeafa3a4ded kbuild: Fixup the IBT kbuild changes
date: 12 days ago
config: x86_64-randconfig-a011-20220404 (https://download.01.org/0day-ci/archive/20220404/[email protected]/config)
compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
reproduce (this is a W=1 build):
# https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d31ed5d767c0452b4f49846d80a0bfeafa3a4ded
git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git fetch --no-tags linus master
git checkout d31ed5d767c0452b4f49846d80a0bfeafa3a4ded
# save the config file to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: __intel_wait_for_register_fw.cold()+0xce: relocation to !ENDBR: vlv_allow_gt_wake.cold+0x0
drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: gen6_alloc_va_range.cold()+0x1c6: relocation to !ENDBR: i915_vma_unpin.cold+0x0
>> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: fence_update.cold()+0x14a: relocation to !ENDBR: i915_vma_revoke_fence.cold+0x0
drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: eb_move_to_gpu.cold()+0x52: relocation to !ENDBR: i915_reset_gen7_sol_offsets.cold+0x0
>> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: __i915_gem_object_release_mmap_gtt.cold()+0xce: relocation to !ENDBR: i915_gem_mmap.cold+0x0
drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: i915_ttm_io_mem_pfn.cold()+0x52: relocation to !ENDBR: i915_ttm_delayed_free.cold+0x0
>> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: ttm_vm_close.cold()+0x52: relocation to !ENDBR: ttm_vm_open.cold+0x0
drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: ttm_vm_open.cold()+0x52: relocation to !ENDBR: i915_ttm_shrinker_release_pages.cold+0x0
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Tue, Apr 05, 2022 at 04:01:15PM +0200, Peter Zijlstra wrote:
> Subject: objtool/ibt: Allow _THIS_IP_ at: sym+len
> From: Peter Zijlstra <[email protected]>
> Date: Tue Apr 5 15:54:41 CEST 2022
>
> 0day robot reported:
>
> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: __intel_wait_for_register_fw.cold()+0xce: relocation to !ENDBR: vlv_allow_gt_wake.cold+0x0
>
> Which turns out to be GCC placing a _THIS_IP_ past the end of the
> function:
>
> 0000000000001d00 <__intel_wait_for_register_fw.cold>:
> ...
> 1dce: 48 c7 c7 00 00 00 00 mov $0x0,%rdi 1dd1: R_X86_64_32S .text.unlikely+0x1df8
> 1dd5: e8 00 00 00 00 call 1dda <__intel_wait_for_register_fw.cold+0xda> 1dd6: R_X86_64_PLT32 __trace_bprintk-0x4
> ...
> 1df6: 0f 0b ud2
>
> Add an exception for this one weird case...
>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
But objtool is complaining about a real problem (albeit with a cryptic
warning). I don't think we want to paper over that. See patch.
Also, are in-tree users of trace_printk() even allowed??
From: Josh Poimboeuf <[email protected]>
Subject: [PATCH] tracing: Fix _THIS_IP_ usage in trace_printk()
do_trace_printk() uses the _THIS_IP_ macro to save the current
instruction pointer as an argument to a called function. However,
because _THIS_IP_ relies on an empty label hack to get the IP, the
compiler is actually free to place the label anywhere in the function,
including at the very end -- which, since the label doesn't actually
have any code, is technically at the beginning of whatever function
happens to come next.
For example:
1d89: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
1d8c: R_X86_64_32S .text.unlikely+0x1d3a
1d90: e8 00 00 00 00 callq 1d95 <__intel_wait_for_register_fw.cold+0xd4>
1d91: R_X86_64_PLT32 __trace_bprintk-0x4
1d95: e8 00 00 00 00 callq 1d9a <__intel_wait_for_register_fw.cold+0xd9>
1d96: R_X86_64_PLT32 __sanitizer_cov_trace_pc-0x4
1d9a: bf 01 00 00 00 mov $0x1,%edi
1d9f: e8 00 00 00 00 callq 1da4 <__intel_wait_for_register_fw.cold+0xe3>
1da0: R_X86_64_PLT32 ftrace_dump-0x4
1da4: 31 f6 xor %esi,%esi
1da6: bf 09 00 00 00 mov $0x9,%edi
1dab: e8 00 00 00 00 callq 1db0 <__intel_wait_for_register_fw.cold+0xef>
1dac: R_X86_64_PLT32 add_taint-0x4
1db0: 90 nop
1db1: 0f 0b ud2
0000000000001db3 <vlv_allow_gt_wake.cold>:
In this case _THIS_IP_ causes the instruction at 0x1d89 to reference the
next function. This results in a semi-cryptic objtool warning:
warning: objtool: __intel_wait_for_register_fw.cold()+0xce: relocation to !ENDBR: vlv_allow_gt_wake.cold+0x
While _THIS_IP_ is inherently imprecise, we can at least coddle the
compiler into putting the label *before* the call by using _THIS_IP_
immediately before the call instead of as an argument to the call.
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
include/linux/kernel.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 08ba5995aa8b..c399b29840eb 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -390,13 +390,15 @@ do { \
static const char *trace_printk_fmt __used \
__section("__trace_printk_fmt") = \
__builtin_constant_p(fmt) ? fmt : NULL; \
+ unsigned long __ip; \
\
__trace_printk_check_format(fmt, ##args); \
\
+ __ip = _THIS_IP_; \
if (__builtin_constant_p(fmt)) \
- __trace_bprintk(_THIS_IP_, trace_printk_fmt, ##args); \
+ __trace_bprintk(__ip, trace_printk_fmt, ##args); \
else \
- __trace_printk(_THIS_IP_, fmt, ##args); \
+ __trace_printk(__ip, fmt, ##args); \
} while (0)
extern __printf(2, 3)
--
2.34.1
On Mon, Apr 04, 2022 at 12:33:19PM +0800, kernel test robot wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: 3123109284176b1532874591f7c81f3837bbdc17
> commit: d31ed5d767c0452b4f49846d80a0bfeafa3a4ded kbuild: Fixup the IBT kbuild changes
> date: 12 days ago
> config: x86_64-randconfig-a011-20220404 (https://download.01.org/0day-ci/archive/20220404/[email protected]/config)
> compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
> reproduce (this is a W=1 build):
> # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d31ed5d767c0452b4f49846d80a0bfeafa3a4ded
> git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> git fetch --no-tags linus master
> git checkout d31ed5d767c0452b4f49846d80a0bfeafa3a4ded
> # save the config file to linux build tree
> mkdir build_dir
> make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: __intel_wait_for_register_fw.cold()+0xce: relocation to !ENDBR: vlv_allow_gt_wake.cold+0x0
> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: gen6_alloc_va_range.cold()+0x1c6: relocation to !ENDBR: i915_vma_unpin.cold+0x0
> >> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: fence_update.cold()+0x14a: relocation to !ENDBR: i915_vma_revoke_fence.cold+0x0
> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: eb_move_to_gpu.cold()+0x52: relocation to !ENDBR: i915_reset_gen7_sol_offsets.cold+0x0
> >> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: __i915_gem_object_release_mmap_gtt.cold()+0xce: relocation to !ENDBR: i915_gem_mmap.cold+0x0
> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: i915_ttm_io_mem_pfn.cold()+0x52: relocation to !ENDBR: i915_ttm_delayed_free.cold+0x0
> >> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: ttm_vm_close.cold()+0x52: relocation to !ENDBR: ttm_vm_open.cold+0x0
> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: ttm_vm_open.cold()+0x52: relocation to !ENDBR: i915_ttm_shrinker_release_pages.cold+0x0
---
Subject: objtool/ibt: Allow _THIS_IP_ at: sym+len
From: Peter Zijlstra <[email protected]>
Date: Tue Apr 5 15:54:41 CEST 2022
0day robot reported:
drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: __intel_wait_for_register_fw.cold()+0xce: relocation to !ENDBR: vlv_allow_gt_wake.cold+0x0
Which turns out to be GCC placing a _THIS_IP_ past the end of the
function:
0000000000001d00 <__intel_wait_for_register_fw.cold>:
...
1dce: 48 c7 c7 00 00 00 00 mov $0x0,%rdi 1dd1: R_X86_64_32S .text.unlikely+0x1df8
1dd5: e8 00 00 00 00 call 1dda <__intel_wait_for_register_fw.cold+0xda> 1dd6: R_X86_64_PLT32 __trace_bprintk-0x4
...
1df6: 0f 0b ud2
Add an exception for this one weird case...
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
tools/objtool/check.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3198,6 +3198,8 @@ static void warn_noendbr(const char *msg
static void validate_ibt_dest(struct objtool_file *file, struct instruction *insn,
struct instruction *dest)
{
+ struct instruction *last, *next;
+
if (dest->func && dest->func == insn->func) {
/*
* Anything from->to self is either _THIS_IP_ or IRET-to-self.
@@ -3217,6 +3219,21 @@ static void validate_ibt_dest(struct obj
if (dest->noendbr)
return;
+ /*
+ * Occasionally, when the last instruction of the function is UD2, GCC
+ * manages to generate a text reference to the instruction after it.
+ */
+ last = next = insn;
+ for (;;) {
+ next = next_insn_same_sec(file, next);
+ if (!next || next->func != insn->func)
+ break;
+ last = next;
+ }
+ if (last->type == INSN_BUG &&
+ last->offset + last->len == dest->offset)
+ return;
+
warn_noendbr("", insn->sec, insn->offset, dest);
}
On Tue, Apr 05 2022 at 17:05, Josh Poimboeuf wrote:
> On Tue, Apr 05, 2022 at 04:01:15PM +0200, Peter Zijlstra wrote:
>
> But objtool is complaining about a real problem (albeit with a cryptic
> warning). I don't think we want to paper over that. See patch.
>
> Also, are in-tree users of trace_printk() even allowed??
See the comment in the header file you are patching:
* This is intended as a debugging tool for the developer only.
* Please refrain from leaving trace_printks scattered around in
* your code. (Extra memory is used for special buffers that are
* allocated when trace_printk() is used.)
....
> From: Josh Poimboeuf <[email protected]>
> Subject: [PATCH] tracing: Fix _THIS_IP_ usage in trace_printk()
>
> do_trace_printk() uses the _THIS_IP_ macro to save the current
> instruction pointer as an argument to a called function. However,
> because _THIS_IP_ relies on an empty label hack to get the IP, the
> compiler is actually free to place the label anywhere in the function,
> including at the very end -- which, since the label doesn't actually
> have any code, is technically at the beginning of whatever function
> happens to come next.
>
> For example:
>
> 1d89: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
> 1d8c: R_X86_64_32S .text.unlikely+0x1d3a
> 1d90: e8 00 00 00 00 callq 1d95 <__intel_wait_for_register_fw.cold+0xd4>
> 1d91: R_X86_64_PLT32 __trace_bprintk-0x4
> 1d95: e8 00 00 00 00 callq 1d9a <__intel_wait_for_register_fw.cold+0xd9>
> 1d96: R_X86_64_PLT32 __sanitizer_cov_trace_pc-0x4
> 1d9a: bf 01 00 00 00 mov $0x1,%edi
> 1d9f: e8 00 00 00 00 callq 1da4 <__intel_wait_for_register_fw.cold+0xe3>
> 1da0: R_X86_64_PLT32 ftrace_dump-0x4
> 1da4: 31 f6 xor %esi,%esi
> 1da6: bf 09 00 00 00 mov $0x9,%edi
> 1dab: e8 00 00 00 00 callq 1db0 <__intel_wait_for_register_fw.cold+0xef>
> 1dac: R_X86_64_PLT32 add_taint-0x4
> 1db0: 90 nop
> 1db1: 0f 0b ud2
>
> 0000000000001db3 <vlv_allow_gt_wake.cold>:
>
> In this case _THIS_IP_ causes the instruction at 0x1d89 to reference the
> next function. This results in a semi-cryptic objtool warning:
>
> warning: objtool: __intel_wait_for_register_fw.cold()+0xce: relocation to !ENDBR: vlv_allow_gt_wake.cold+0x
>
> While _THIS_IP_ is inherently imprecise, we can at least coddle the
> compiler into putting the label *before* the call by using _THIS_IP_
> immediately before the call instead of as an argument to the call.
>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Josh Poimboeuf <[email protected]>
> ---
> include/linux/kernel.h | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 08ba5995aa8b..c399b29840eb 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -390,13 +390,15 @@ do { \
> static const char *trace_printk_fmt __used \
> __section("__trace_printk_fmt") = \
> __builtin_constant_p(fmt) ? fmt : NULL; \
> + unsigned long __ip; \
> \
> __trace_printk_check_format(fmt, ##args); \
> \
> + __ip = _THIS_IP_; \
> if (__builtin_constant_p(fmt)) \
> - __trace_bprintk(_THIS_IP_, trace_printk_fmt, ##args); \
> + __trace_bprintk(__ip, trace_printk_fmt, ##args); \
> else \
> - __trace_printk(_THIS_IP_, fmt, ##args); \
> + __trace_printk(__ip, fmt, ##args); \
> } while (0)
>
> extern __printf(2, 3)
This covers the trace_printk() case which uses do_trace_printk(), but
the same problem exists in trace_puts() and ftrace_vprintk()...., no?
Thanks,
tglx
On Wed, 06 Apr 2022 02:46:22 +0200
Thomas Gleixner <[email protected]> wrote:
> This covers the trace_printk() case which uses do_trace_printk(), but
> the same problem exists in trace_puts() and ftrace_vprintk()...., no?
Hmm, I'm not even sure why ftrace_vprintk() is there. It seems redundant.
Arnaldo,
Was there a reason for it. The commit that added it isn't very descriptive.
commit 9011262a37cb438f0fa9394b5e83840db8f9680a
Author: Arnaldo Carvalho de Melo <[email protected]>
Date: Fri Jan 23 12:06:23 2009 -0200
ftrace: add ftrace_vprintk
Impact: new helper function
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
-- Steve
On Tue, Apr 05, 2022 at 10:32:51PM -0700, Josh Poimboeuf wrote:
> More broadly, this issue could theoretically happen in some other places
> throughout the kernel tree, since _THIS_IP_ is fundamentally unreliable
> as currently written.
>
> So we could look at making _THIS_IP_ more predictable.
>
> Inline asm would work better ("lea 0(%rip), %[rip]"), but then you need
> an arch-dependent implementation...
Well, there's a ton of _THIS_IP_ instances all around, and it would be
unfortunate to have them grow into actual code :/
On Tue, Apr 05, 2022 at 10:32:51PM -0700, Josh Poimboeuf wrote:
> On Wed, Apr 06, 2022 at 02:46:22AM +0200, Thomas Gleixner wrote:
> > On Tue, Apr 05 2022 at 17:05, Josh Poimboeuf wrote:
> > > On Tue, Apr 05, 2022 at 04:01:15PM +0200, Peter Zijlstra wrote:
> > >
> > > But objtool is complaining about a real problem (albeit with a cryptic
> > > warning). I don't think we want to paper over that. See patch.
> > >
> > > Also, are in-tree users of trace_printk() even allowed??
> >
> > See the comment in the header file you are patching:
> >
> > * This is intended as a debugging tool for the developer only.
> > * Please refrain from leaving trace_printks scattered around in
> > * your code. (Extra memory is used for special buffers that are
> > * allocated when trace_printk() is used.)
>
> So what do we do ... send a nastygram?
Best would be for Steve to send a patch removing them all:
arch/powerpc/kvm/book3s_xics.c:#define XICS_DBG(fmt...) trace_printk(fmt)
drivers/gpu/drm/i915/gt/intel_gtt.h:#define DBG(...) trace_printk(__VA_ARGS__)
drivers/gpu/drm/i915/i915_gem.h:#define GEM_TRACE(...) trace_printk(__VA_ARGS__)
drivers/gpu/drm/i915/i915_gem.h: trace_printk(__VA_ARGS__); \
drivers/hwtracing/stm/dummy_stm.c: trace_printk("[%u:%u] [pkt: %x/%x] (%llx)\n", master, channel,
drivers/infiniband/hw/hfi1/trace_dbg.h: trace_printk(fmt, ##__VA_ARGS__)
drivers/tty/n_tty.c:# define n_tty_trace(f, args...) trace_printk(f, ##args)
drivers/usb/early/xhci-dbc.c:#define xdbc_trace trace_printk
fs/ext4/inline.c: trace_printk("inode %lu\n", dir->i_ino);
fs/ext4/inline.c: trace_printk("de: off %u rlen %u name %.*s nlen %u ino %u\n",
All except i915 use CPP tokens that don't exist, local developer really
has to take effort to enable it. i915 crud depends on a CONFIG_ symbols
that all{yes,mod}config will happily set for you,
On Wed, Apr 06, 2022 at 02:46:22AM +0200, Thomas Gleixner wrote:
> On Tue, Apr 05 2022 at 17:05, Josh Poimboeuf wrote:
> > On Tue, Apr 05, 2022 at 04:01:15PM +0200, Peter Zijlstra wrote:
> >
> > But objtool is complaining about a real problem (albeit with a cryptic
> > warning). I don't think we want to paper over that. See patch.
> >
> > Also, are in-tree users of trace_printk() even allowed??
>
> See the comment in the header file you are patching:
>
> * This is intended as a debugging tool for the developer only.
> * Please refrain from leaving trace_printks scattered around in
> * your code. (Extra memory is used for special buffers that are
> * allocated when trace_printk() is used.)
So what do we do ... send a nastygram?
> > + __ip = _THIS_IP_; \
> > if (__builtin_constant_p(fmt)) \
> > - __trace_bprintk(_THIS_IP_, trace_printk_fmt, ##args); \
> > + __trace_bprintk(__ip, trace_printk_fmt, ##args); \
> > else \
> > - __trace_printk(_THIS_IP_, fmt, ##args); \
> > + __trace_printk(__ip, fmt, ##args); \
> > } while (0)
> >
> > extern __printf(2, 3)
>
> This covers the trace_printk() case which uses do_trace_printk(), but
> the same problem exists in trace_puts() and ftrace_vprintk()...., no?
Yes, though objtool didn't seem to complain about those yet. They
probably don't have the perfect storm required for the label to end up
at the end of the function. It might also need something like being
invoked from within a macro which then does BUG() (see GEM_BUG_ON).
More broadly, this issue could theoretically happen in some other places
throughout the kernel tree, since _THIS_IP_ is fundamentally unreliable
as currently written.
So we could look at making _THIS_IP_ more predictable.
Inline asm would work better ("lea 0(%rip), %[rip]"), but then you need
an arch-dependent implementation...
Or we could add a control dependency like the below ugliness...
Thoughts?
diff --git a/include/linux/instruction_pointer.h b/include/linux/instruction_pointer.h
index cda1f706eaeb..3f2f0ebecca0 100644
--- a/include/linux/instruction_pointer.h
+++ b/include/linux/instruction_pointer.h
@@ -2,7 +2,9 @@
#ifndef _LINUX_INSTRUCTION_POINTER_H
#define _LINUX_INSTRUCTION_POINTER_H
+unsigned long __this_ip(void);
+
#define _RET_IP_ (unsigned long)__builtin_return_address(0)
-#define _THIS_IP_ ({ __label__ __here; __here: (unsigned long)&&__here; })
+#define _THIS_IP_ __this_ip()
#endif /* _LINUX_INSTRUCTION_POINTER_H */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index da03c15ecc89..8674c7434ead 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3781,3 +3781,8 @@ void __printk_cpu_unlock(void)
}
EXPORT_SYMBOL(__printk_cpu_unlock);
#endif /* CONFIG_SMP */
+
+unsigned long __this_ip(void)
+{
+ return _RET_IP_;
+}
On Tue, 5 Apr 2022 22:32:51 -0700
Josh Poimboeuf <[email protected]> wrote:
> > See the comment in the header file you are patching:
> >
> > * This is intended as a debugging tool for the developer only.
> > * Please refrain from leaving trace_printks scattered around in
> > * your code. (Extra memory is used for special buffers that are
> > * allocated when trace_printk() is used.)
>
> So what do we do ... send a nastygram?
We already do. When anything with a trace_printk() is loaded, the dmesg
will display:
**********************************************************
** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **
** **
** trace_printk() being used. Allocating extra memory. **
** **
** This means that this is a DEBUG kernel and it is **
** unsafe for production use. **
** **
** If you see this message and you are not debugging **
** the kernel, report this immediately to your vendor! **
** **
** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **
**********************************************************
:-)
-- Steve
On Wed, 6 Apr 2022 09:29:13 +0200
Peter Zijlstra <[email protected]> wrote:
> Best would be for Steve to send a patch removing them all:
>
>
> arch/powerpc/kvm/book3s_xics.c:#define XICS_DBG(fmt...) trace_printk(fmt)
> drivers/gpu/drm/i915/gt/intel_gtt.h:#define DBG(...) trace_printk(__VA_ARGS__)
> drivers/gpu/drm/i915/i915_gem.h:#define GEM_TRACE(...) trace_printk(__VA_ARGS__)
> drivers/gpu/drm/i915/i915_gem.h: trace_printk(__VA_ARGS__); \
> drivers/hwtracing/stm/dummy_stm.c: trace_printk("[%u:%u] [pkt: %x/%x] (%llx)\n", master, channel,
> drivers/infiniband/hw/hfi1/trace_dbg.h: trace_printk(fmt, ##__VA_ARGS__)
> drivers/tty/n_tty.c:# define n_tty_trace(f, args...) trace_printk(f, ##args)
> drivers/usb/early/xhci-dbc.c:#define xdbc_trace trace_printk
> fs/ext4/inline.c: trace_printk("inode %lu\n", dir->i_ino);
> fs/ext4/inline.c: trace_printk("de: off %u rlen %u name %.*s nlen %u ino %u\n",
>
>
> All except i915 use CPP tokens that don't exist, local developer really
> has to take effort to enable it. i915 crud depends on a CONFIG_ symbols
> that all{yes,mod}config will happily set for you,
As I replied to Josh, when trace_printk() is used, you get nasty dmesg
reports, and those do show up in bug reports to vendors (I laugh every time
I see them.)
I'm not too worried if trace_printk() is called for debugging, and even
with CONFIG_ options.
I even have trace_printk() used for things like my ring_buffer benchmark
(which should never be run on production!)
Thus, if the code that has trace_printk() is truly for debugging, and not
something that would normally get applied in other people's trees, then
that actually falls into its use case.
-- Steve
On Wed, Apr 06, 2022 at 09:43:30AM +0200, Peter Zijlstra wrote:
> On Tue, Apr 05, 2022 at 10:32:51PM -0700, Josh Poimboeuf wrote:
> > More broadly, this issue could theoretically happen in some other places
> > throughout the kernel tree, since _THIS_IP_ is fundamentally unreliable
> > as currently written.
> >
> > So we could look at making _THIS_IP_ more predictable.
> >
> > Inline asm would work better ("lea 0(%rip), %[rip]"), but then you need
> > an arch-dependent implementation...
>
> Well, there's a ton of _THIS_IP_ instances all around, and it would be
> unfortunate to have them grow into actual code :/
What do you mean by growing into actual code? It's still just a single
instruction, as was the immediate load before.
Though, you pasted this on irc:
#define _THIS_IP_ ({ __label__ __here; __here: asm_volatile_goto ("":::: __here); (unsigned long)&&__here; })
which seems decent to me, though less than ideal because it grows an
ENDBR. But I like its arch-independence, so yeah, LGTM.
--
Josh
Em Tue, Apr 05, 2022 at 09:20:32PM -0400, Steven Rostedt escreveu:
> On Wed, 06 Apr 2022 02:46:22 +0200
> Thomas Gleixner <[email protected]> wrote:
>
> > This covers the trace_printk() case which uses do_trace_printk(), but
> > the same problem exists in trace_puts() and ftrace_vprintk()...., no?
>
> Hmm, I'm not even sure why ftrace_vprintk() is there. It seems redundant.
>
> Arnaldo,
>
> Was there a reason for it. The commit that added it isn't very descriptive.
Yeah, I was young and in a hurry.
- Arnaldo
> commit 9011262a37cb438f0fa9394b5e83840db8f9680a
> Author: Arnaldo Carvalho de Melo <[email protected]>
> Date: Fri Jan 23 12:06:23 2009 -0200
>
> ftrace: add ftrace_vprintk
>
> Impact: new helper function
>
> Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
> Signed-off-by: Ingo Molnar <[email protected]>
>
>
> -- Steve
--
- Arnaldo
On Wed, Apr 06, 2022 at 09:37:03AM -0700, Josh Poimboeuf wrote:
> On Wed, Apr 06, 2022 at 09:43:30AM +0200, Peter Zijlstra wrote:
> > On Tue, Apr 05, 2022 at 10:32:51PM -0700, Josh Poimboeuf wrote:
> > > More broadly, this issue could theoretically happen in some other places
> > > throughout the kernel tree, since _THIS_IP_ is fundamentally unreliable
> > > as currently written.
> > >
> > > So we could look at making _THIS_IP_ more predictable.
> > >
> > > Inline asm would work better ("lea 0(%rip), %[rip]"), but then you need
> > > an arch-dependent implementation...
> >
> > Well, there's a ton of _THIS_IP_ instances all around, and it would be
> > unfortunate to have them grow into actual code :/
>
> What do you mean by growing into actual code? It's still just a single
> instruction, as was the immediate load before.
Aah, indeed. I was somehow thinking we'd get extra instructions.
> Though, you pasted this on irc:
>
> #define _THIS_IP_ ({ __label__ __here; __here: asm_volatile_goto ("":::: __here); (unsigned long)&&__here; })
>
> which seems decent to me, though less than ideal because it grows an
> ENDBR. But I like its arch-independence, so yeah, LGTM.
I did send hjl an email about that extra endbr, because I really don't
like that. And jump_label (also using asm-goto) doesn't grow those endbr
instructions, so something is weird.