Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1332665 > unrolled thread
| Started by | Jiri Slaby <jslaby@suse.cz> |
|---|---|
| First post | 2016-02-12 11:40 +0100 |
| Last post | 2016-02-19 13:10 +0100 |
| Articles | 13 — 4 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 00/33] Compile-time stack metadata validation Jiri Slaby <jslaby@suse.cz> - 2016-02-12 11:40 +0100
Re: [PATCH 00/33] Compile-time stack metadata validation Jiri Slaby <jslaby@suse.cz> - 2016-02-12 11:50 +0100
Re: [PATCH 00/33] Compile-time stack metadata validation Josh Poimboeuf <jpoimboe@redhat.com> - 2016-02-12 15:50 +0100
Re: [PATCH 00/33] Compile-time stack metadata validation Peter Zijlstra <peterz@infradead.org> - 2016-02-12 18:20 +0100
Re: [PATCH 00/33] Compile-time stack metadata validation Josh Poimboeuf <jpoimboe@redhat.com> - 2016-02-12 19:40 +0100
Re: [PATCH 00/33] Compile-time stack metadata validation Josh Poimboeuf <jpoimboe@redhat.com> - 2016-02-12 19:40 +0100
Re: [PATCH 00/33] Compile-time stack metadata validation Peter Zijlstra <peterz@infradead.org> - 2016-02-12 21:20 +0100
Re: [PATCH 00/33] Compile-time stack metadata validation Josh Poimboeuf <jpoimboe@redhat.com> - 2016-02-15 17:40 +0100
Re: [PATCH 00/33] Compile-time stack metadata validation Peter Zijlstra <peterz@infradead.org> - 2016-02-15 18:00 +0100
Re: [PATCH 00/33] Compile-time stack metadata validation Andi Kleen <andi@firstfloor.org> - 2016-02-15 21:10 +0100
Re: [PATCH 00/33] Compile-time stack metadata validation Josh Poimboeuf <jpoimboe@redhat.com> - 2016-02-15 21:10 +0100
[PATCH] sched/x86: Add stack frame dependency to __preempt_schedule[_notrace] Josh Poimboeuf <jpoimboe@redhat.com> - 2016-02-18 18:50 +0100
Re: [PATCH] sched/x86: Add stack frame dependency to __preempt_schedule[_notrace] Jiri Slaby <jslaby@suse.cz> - 2016-02-19 13:10 +0100
| From | Jiri Slaby <jslaby@suse.cz> |
|---|---|
| Date | 2016-02-12 11:40 +0100 |
| Subject | Re: [PATCH 00/33] Compile-time stack metadata validation |
| Message-ID | <r1jiO-5IA-19@gated-at.bofh.it> |
On 01/21/2016, 11:49 PM, Josh Poimboeuf wrote:
> This is v16 of the compile-time stack metadata validation patch set,
> along with proposed fixes for most of the warnings it found. It's based
> on the tip/master branch.
Hi,
with this config:
https://github.com/openSUSE/kernel-source/blob/master/config/x86_64/vanilla
I am seeing a lot of functions in C which do not have frame pointer setup/cleanup:
stacktool: drivers/scsi/hpsa.o: hpsa_scsi_do_simple_cmd.constprop.106()+0x79: call without frame pointer save/setup
stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x8: duplicate frame pointer save
stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x9: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x1: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x8: duplicate frame pointer save
stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x9: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lnet/lnet/lo.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lnet/lnet/nidstrings.o: cfs_print_nidlist()+0x220: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/lnet/peer.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x8: duplicate frame pointer save
stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x9: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lnet/lnet/router.o: lnet_find_net_locked()+0x8a: frame pointer state mismatch
stacktool: drivers/staging/lustre/lnet/lnet/router.o: lnet_find_net_locked()+0x8a: return without frame pointer restore
stacktool: drivers/staging/lustre/lustre/fid/fid_request.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/fld/lproc_fld.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/libcfs/libcfs_lock.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/libcfs/libcfs_mem.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x4: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x24: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x1a: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x1b: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x19: return without frame pointer restore
stacktool: drivers/staging/lustre/lustre/llite/../lclient/lcommon_misc.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/llite_mmap.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/lproc_llite.o: checksum_pages_store()+0x19e: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x5: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x9: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/rw.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x24: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x5: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x9: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/statahead.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/vvp_page.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/xattr_cache.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x1f: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x5: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/lmv/lmv_intent.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/lmv/lmv_obd.o: __lmv_fid_alloc()+0x185: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/lov/lov_io.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/lov/lovsub_dev.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/mdc/mdc_lib.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/mdc/mdc_locks.o: .text.unlikely: unexpected end of section
stacktool: drivers/staging/lustre/lustre/obdclass/debug.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/obdclass/genops.o: class_name2dev()+0xc7: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/obdclass/lustre_handles.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x4: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x0: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x1: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: osc_extent_search()+0x78: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: osc_extent_search()+0x78: return without frame pointer restore
stacktool: drivers/staging/lustre/lustre/osc/osc_dev.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/osc/osc_page.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/ptlrpc/connection.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x5: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x6: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/ptlrpc/llog_net.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_extent.o: ldlm_extent_shift_kms()+0x93: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_lock.o: ldlm_work_bl_ast_lock()+0x156: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_lock.o: ldlm_work_cp_ast_lock()+0xda: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x0: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x5: duplicate frame pointer save
stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x6: duplicate frame pointer setup
stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/ptlrpc/pack_generic.o: lustre_swab_mgs_nidtbl_entry()+0x89: frame pointer state mismatch
stacktool: drivers/staging/lustre/lustre/ptlrpc/pack_generic.o: lustre_swab_mgs_nidtbl_entry()+0x89: return without frame pointer restore
stacktool: drivers/staging/lustre/lustre/ptlrpc/sec_bulk.o: .text: unexpected end of section
stacktool: drivers/staging/lustre/lustre/ptlrpc/sec_config.o: .text: unexpected end of section
stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x70: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x92: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_free()+0xff: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_free()+0xf5: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_free()+0x11a: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_get()+0x225: call without frame pointer save/setup
stacktool: kernel/locking/percpu-rwsem.o: percpu_up_read()+0x27: call without frame pointer save/setup
stacktool: kernel/profile.o: do_profile_hits.isra.5()+0x139: call without frame pointer save/setup
stacktool: lib/nmi_backtrace.o: nmi_trigger_all_cpu_backtrace()+0x2b6: call without frame pointer save/setup
stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_recv()+0x58: call without frame pointer save/setup
stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_send()+0x58: call without frame pointer save/setup
stacktool: net/rds/ib_recv.o: rds_ib_attempt_ack()+0xc1: call without frame pointer save/setup
stacktool: net/rds/iw_recv.o: rds_iw_attempt_ack()+0xc1: call without frame pointer save/setup
stacktool: net/rds/iw_recv.o: rds_iw_recv_cq_comp_handler()+0x55: call without frame pointer save/setup
For example do_profile_hits.isra.5:
0000000000003360 <hpsa_scsi_do_simple_cmd.constprop.106>:
3360: e8 00 00 00 00 callq 3365 <hpsa_scsi_do_simple_cmd.constprop.106+0x5>
3361: R_X86_64_PC32 __fentry__-0x4
3365: 65 ff 05 00 00 00 00 incl %gs:0x0(%rip) # 336c <hpsa_scsi_do_simple_cmd.constprop.106+0xc>
3368: R_X86_64_PC32 __preempt_count-0x4
336c: 65 8b 0d 00 00 00 00 mov %gs:0x0(%rip),%ecx # 3373 <hpsa_scsi_do_simple_cmd.constprop.106+0x13>
336f: R_X86_64_PC32 cpu_number-0x4
3373: 48 63 c9 movslq %ecx,%rcx
3376: 48 8b 87 b8 4b 00 00 mov 0x4bb8(%rdi),%rax
337d: 48 8b 0c cd 00 00 00 mov 0x0(,%rcx,8),%rcx
3384: 00
3381: R_X86_64_32S __per_cpu_offset
3385: 8b 04 01 mov (%rcx,%rax,1),%eax
3388: 65 ff 0d 00 00 00 00 decl %gs:0x0(%rip) # 338f <hpsa_scsi_do_simple_cmd.constprop.106+0x2f>
338b: R_X86_64_PC32 __preempt_count-0x4
338f: 74 48 je 33d9 <hpsa_scsi_do_simple_cmd.constprop.106+0x79>
3391: 85 c0 test %eax,%eax
3393: 75 4d jne 33e2 <hpsa_scsi_do_simple_cmd.constprop.106+0x82>
3395: 55 push %rbp
3396: 48 89 e5 mov %rsp,%rbp
3399: 53 push %rbx
339a: 48 8d 5d d8 lea -0x28(%rbp),%rbx
339e: 48 83 ec 20 sub $0x20,%rsp
33a2: c7 45 d8 00 00 00 00 movl $0x0,-0x28(%rbp)
33a9: c7 45 e0 00 00 00 00 movl $0x0,-0x20(%rbp)
33b0: 48 8d 43 10 lea 0x10(%rbx),%rax
33b4: 48 89 9e 54 02 00 00 mov %rbx,0x254(%rsi)
33bb: 48 89 45 e8 mov %rax,-0x18(%rbp)
33bf: 48 89 45 f0 mov %rax,-0x10(%rbp)
33c3: e8 f8 ce ff ff callq 2c0 <__enqueue_cmd_and_start_io>
33c8: 48 89 df mov %rbx,%rdi
33cb: e8 00 00 00 00 callq 33d0 <hpsa_scsi_do_simple_cmd.constprop.106+0x70>
33cc: R_X86_64_PC32 wait_for_completion_io-0x4
33d0: 48 83 c4 20 add $0x20,%rsp
33d4: 31 c0 xor %eax,%eax
33d6: 5b pop %rbx
33d7: 5d pop %rbp
33d8: c3 retq
33d9: e8 00 00 00 00 callq 33de <hpsa_scsi_do_simple_cmd.constprop.106+0x7e>
33da: R_X86_64_PC32 ___preempt_schedule-0x4
33de: 85 c0 test %eax,%eax
33e0: 74 b3 je 3395 <hpsa_scsi_do_simple_cmd.constprop.106+0x35>
33e2: 48 8b 86 38 02 00 00 mov 0x238(%rsi),%rax
33e9: ba ff ff ff ff mov $0xffffffff,%edx
33ee: 66 89 50 02 mov %dx,0x2(%rax)
33f2: 31 c0 xor %eax,%eax
33f4: c3 retq
33f5: 90 nop
33f6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
33fd: 00 00 00
It there some compilation flag missing? -f flags when compiling that file are:
-falign-jumps=1
-falign-loops=1
-fconserve-stack
-fno-asynchronous-unwind-tables
-fno-common
-fno-delete-null-pointer-checks
-fno-inline-functions-called-once
-fno-omit-frame-pointer
-fno-optimize-sibling-calls
-fno-strict-aliasing
-fno-strict-overflow
-fno-var-tracking-assignments
-fstack-protector
-funit-at-a-time
thanks,
--
js
suse labs
[toc] | [next] | [standalone]
| From | Jiri Slaby <jslaby@suse.cz> |
|---|---|
| Date | 2016-02-12 11:50 +0100 |
| Message-ID | <r1jsu-5ND-15@gated-at.bofh.it> |
| In reply to | #1332665 |
On 02/12/2016, 11:36 AM, Jiri Slaby wrote: > It there some compilation flag missing? -f flags when compiling that file are: > -falign-jumps=1 > -falign-loops=1 > -fconserve-stack > -fno-asynchronous-unwind-tables > -fno-common > -fno-delete-null-pointer-checks > -fno-inline-functions-called-once > -fno-omit-frame-pointer > -fno-optimize-sibling-calls > -fno-strict-aliasing > -fno-strict-overflow > -fno-var-tracking-assignments > -fstack-protector > -funit-at-a-time Happens with: gcc (SUSE Linux) 5.3.1 20151207 [gcc-5-branch revision 231355] gcc-6 (SUSE Linux) 6.0.0 20160202 (experimental) [trunk revision 233076] > thanks, -- js suse labs
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-02-12 15:50 +0100 |
| Message-ID | <r1ncL-8iz-17@gated-at.bofh.it> |
| In reply to | #1332665 |
On Fri, Feb 12, 2016 at 11:36:24AM +0100, Jiri Slaby wrote:
> On 01/21/2016, 11:49 PM, Josh Poimboeuf wrote:
> > This is v16 of the compile-time stack metadata validation patch set,
> > along with proposed fixes for most of the warnings it found. It's based
> > on the tip/master branch.
>
> Hi,
>
> with this config:
> https://github.com/openSUSE/kernel-source/blob/master/config/x86_64/vanilla
>
> I am seeing a lot of functions in C which do not have frame pointer setup/cleanup:
Hi Jiri,
Thanks for testing.
> stacktool: drivers/scsi/hpsa.o: hpsa_scsi_do_simple_cmd.constprop.106()+0x79: call without frame pointer save/setup
This seems like a real frame pointer bug caused by the following line in
arch/x86/include/asm/preempt.h:
# define __preempt_schedule() asm ("call ___preempt_schedule")
The asm statement doesn't have the stack pointer as an output operand,
so gcc doesn't skips the frame pointer setup before calling.
However, I suspect the "bug" is intentional for optimization purposes.
> stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x8: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.o: cfs_cdebug_show.part.5.constprop.35()+0x9: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: ksocknal_connsock_decref()+0x1: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lnet/klnds/socklnd/socklnd.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x8: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: cfs_cdebug_show.part.1.constprop.16()+0x9: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lnet/lnet/lib-move.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lnet/lnet/lo.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lnet/lnet/nidstrings.o: cfs_print_nidlist()+0x220: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/lnet/peer.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x8: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lnet/lnet/router.o: cfs_cdebug_show.part.0.constprop.16()+0x9: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lnet/lnet/router.o: lnet_find_net_locked()+0x8a: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lnet/lnet/router.o: lnet_find_net_locked()+0x8a: return without frame pointer restore
> stacktool: drivers/staging/lustre/lustre/fid/fid_request.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/fld/lproc_fld.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/libcfs/libcfs_lock.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/libcfs/libcfs_mem.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/dir.o: obd_unpackmd()+0x4: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/file.o: md_intent_lock.part.28()+0x24: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x1a: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x1b: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/../lclient/glimpse.o: cl_io_get()+0x19: return without frame pointer restore
> stacktool: drivers/staging/lustre/lustre/llite/../lclient/lcommon_misc.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/llite_mmap.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/lproc_llite.o: checksum_pages_store()+0x19e: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x5: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/namei.o: ll_test_inode()+0x9: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/rw.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: md_revalidate_lock.part.26()+0x24: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x5: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: sa_args_fini()+0x9: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/statahead.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/vvp_page.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/xattr_cache.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x1f: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/llite/xattr.o: get_xattr_type()+0x5: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/lmv/lmv_intent.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/lmv/lmv_obd.o: __lmv_fid_alloc()+0x185: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/lov/lov_io.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/lov/lovsub_dev.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/mdc/mdc_lib.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/mdc/mdc_locks.o: .text.unlikely: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/obdclass/debug.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/obdclass/genops.o: class_name2dev()+0xc7: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/obdclass/lustre_handles.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/obdclass/obd_config.o: lustre_cfg_string()+0x4: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x0: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: __client_obd_list_lock()+0x1: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: osc_extent_search()+0x78: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/osc/osc_cache.o: osc_extent_search()+0x78: return without frame pointer restore
> stacktool: drivers/staging/lustre/lustre/osc/osc_dev.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/osc/osc_page.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/ptlrpc/connection.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x5: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/ptlrpc/import.o: deuuidify.constprop.8()+0x6: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/ptlrpc/llog_net.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_extent.o: ldlm_extent_shift_kms()+0x93: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_lock.o: ldlm_work_bl_ast_lock()+0x156: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/ldlm_lock.o: ldlm_work_cp_ast_lock()+0xda: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x0: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x5: duplicate frame pointer save
> stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: nrs_policy_register()+0x6: duplicate frame pointer setup
> stacktool: drivers/staging/lustre/lustre/ptlrpc/nrs.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/ptlrpc/pack_generic.o: lustre_swab_mgs_nidtbl_entry()+0x89: frame pointer state mismatch
> stacktool: drivers/staging/lustre/lustre/ptlrpc/pack_generic.o: lustre_swab_mgs_nidtbl_entry()+0x89: return without frame pointer restore
> stacktool: drivers/staging/lustre/lustre/ptlrpc/sec_bulk.o: .text: unexpected end of section
> stacktool: drivers/staging/lustre/lustre/ptlrpc/sec_config.o: .text: unexpected end of section
These staging driver issues are caused by stacktool getting confused by
gcc optimizations related to noreturn functions. I have it on the TODO
list to make the noreturn function detection more intelligent.
> stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x70: call without frame pointer save/setup
> stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x92: call without frame pointer save/setup
> stacktool: fs/mbcache.o: mb_cache_entry_free()+0xff: call without frame pointer save/setup
> stacktool: fs/mbcache.o: mb_cache_entry_free()+0xf5: call without frame pointer save/setup
> stacktool: fs/mbcache.o: mb_cache_entry_free()+0x11a: call without frame pointer save/setup
> stacktool: fs/mbcache.o: mb_cache_entry_get()+0x225: call without frame pointer save/setup
> stacktool: kernel/locking/percpu-rwsem.o: percpu_up_read()+0x27: call without frame pointer save/setup
> stacktool: kernel/profile.o: do_profile_hits.isra.5()+0x139: call without frame pointer save/setup
> stacktool: lib/nmi_backtrace.o: nmi_trigger_all_cpu_backtrace()+0x2b6: call without frame pointer save/setup
> stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_recv()+0x58: call without frame pointer save/setup
> stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_send()+0x58: call without frame pointer save/setup
> stacktool: net/rds/ib_recv.o: rds_ib_attempt_ack()+0xc1: call without frame pointer save/setup
> stacktool: net/rds/iw_recv.o: rds_iw_attempt_ack()+0xc1: call without frame pointer save/setup
> stacktool: net/rds/iw_recv.o: rds_iw_recv_cq_comp_handler()+0x55: call without frame pointer save/setup
These are all the same "call ___preempt_schedule" issue from above.
I'll need to look into it to figure out if it's a real bug or if it's a
"feature" we should ignore.
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2016-02-12 18:20 +0100 |
| Message-ID | <r1pxW-1vA-49@gated-at.bofh.it> |
| In reply to | #1332793 |
On Fri, Feb 12, 2016 at 08:45:43AM -0600, Josh Poimboeuf wrote:
> On Fri, Feb 12, 2016 at 11:36:24AM +0100, Jiri Slaby wrote:
>
> This seems like a real frame pointer bug caused by the following line in
> arch/x86/include/asm/preempt.h:
>
> # define __preempt_schedule() asm ("call ___preempt_schedule")
The purpose there is that:
preempt_enable();
turns into:
decl __percpu_prefix:__preempt_count
jnz 1f:
call ___preempt_schedule
1:
See arch/x86/include/asm/preempt.h:__preempt_count_dec_and_test()
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-02-12 19:40 +0100 |
| Message-ID | <r1qNl-2ew-25@gated-at.bofh.it> |
| In reply to | #1332945 |
On Fri, Feb 12, 2016 at 12:32:06PM -0600, Josh Poimboeuf wrote:
> On Fri, Feb 12, 2016 at 06:10:37PM +0100, Peter Zijlstra wrote:
> > On Fri, Feb 12, 2016 at 08:45:43AM -0600, Josh Poimboeuf wrote:
> > > On Fri, Feb 12, 2016 at 11:36:24AM +0100, Jiri Slaby wrote:
> > >
> > > This seems like a real frame pointer bug caused by the following line in
> > > arch/x86/include/asm/preempt.h:
> > >
> > > # define __preempt_schedule() asm ("call ___preempt_schedule")
> >
> > The purpose there is that:
> >
> > preempt_enable();
> >
> > turns into:
> >
> > decl __percpu_prefix:__preempt_count
> > jnz 1f:
> > call ___preempt_schedule
> > 1:
> >
> > See arch/x86/include/asm/preempt.h:__preempt_count_dec_and_test()
>
> Sorry, I'm kind of confused. Do you mean that's what preempt_enable()
> would turn into *without* the above define?
>
> What I actually see in the listing is:
>
> decl __percpu_prefix:__preempt_count
> je 1f:
> ....
> 1:
> call ___preempt_schedule
>
> So it puts the "call ___preempt_schedule" in the slow path.
>
> I also don't see how that would be related to the use of the asm
> statement in the __preempt_schedule() macro. Doesn't the use of
> unlikely() in preempt_enable() put the call in the slow path?
>
> #define preempt_enable() \
> do { \
> barrier(); \
> if (unlikely(preempt_count_dec_and_test())) \
> preempt_schedule(); \
> } while (0)
>
> Also, why is the thunk needed? Any reason why preempt_enable() can't be
> called directly from C?
Sorry, s/preempt_enable/preempt_schedule/ on that last sentence.
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-02-12 19:40 +0100 |
| Message-ID | <r1qNl-2ew-27@gated-at.bofh.it> |
| In reply to | #1332945 |
On Fri, Feb 12, 2016 at 06:10:37PM +0100, Peter Zijlstra wrote:
> On Fri, Feb 12, 2016 at 08:45:43AM -0600, Josh Poimboeuf wrote:
> > On Fri, Feb 12, 2016 at 11:36:24AM +0100, Jiri Slaby wrote:
> >
> > This seems like a real frame pointer bug caused by the following line in
> > arch/x86/include/asm/preempt.h:
> >
> > # define __preempt_schedule() asm ("call ___preempt_schedule")
>
> The purpose there is that:
>
> preempt_enable();
>
> turns into:
>
> decl __percpu_prefix:__preempt_count
> jnz 1f:
> call ___preempt_schedule
> 1:
>
> See arch/x86/include/asm/preempt.h:__preempt_count_dec_and_test()
Sorry, I'm kind of confused. Do you mean that's what preempt_enable()
would turn into *without* the above define?
What I actually see in the listing is:
decl __percpu_prefix:__preempt_count
je 1f:
....
1:
call ___preempt_schedule
So it puts the "call ___preempt_schedule" in the slow path.
I also don't see how that would be related to the use of the asm
statement in the __preempt_schedule() macro. Doesn't the use of
unlikely() in preempt_enable() put the call in the slow path?
#define preempt_enable() \
do { \
barrier(); \
if (unlikely(preempt_count_dec_and_test())) \
preempt_schedule(); \
} while (0)
Also, why is the thunk needed? Any reason why preempt_enable() can't be
called directly from C?
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2016-02-12 21:20 +0100 |
| Message-ID | <r1sm5-3kI-5@gated-at.bofh.it> |
| In reply to | #1333027 |
On Fri, Feb 12, 2016 at 12:32:06PM -0600, Josh Poimboeuf wrote:
> What I actually see in the listing is:
>
> decl __percpu_prefix:__preempt_count
> je 1f:
> ....
> 1:
> call ___preempt_schedule
>
> So it puts the "call ___preempt_schedule" in the slow path.
Ah yes indeed. Same difference though.
> I also don't see how that would be related to the use of the asm
> statement in the __preempt_schedule() macro. Doesn't the use of
> unlikely() in preempt_enable() put the call in the slow path?
Sadly no, unlikely() and asm_goto don't work well together. But the slow
path or not isn't the reason we do the asm call thing.
> #define preempt_enable() \
> do { \
> barrier(); \
> if (unlikely(preempt_count_dec_and_test())) \
> preempt_schedule(); \
> } while (0)
>
> Also, why is the thunk needed? Any reason why preempt_enable() can't be
> called directly from C?
That would make the call-site save registers and increase the size of
every preempt_enable(). By using the thunk we can do callee saved
registers and avoid blowing up the call site.
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-02-15 17:40 +0100 |
| Message-ID | <r2ulR-3v3-27@gated-at.bofh.it> |
| In reply to | #1333080 |
On Fri, Feb 12, 2016 at 09:10:11PM +0100, Peter Zijlstra wrote:
> On Fri, Feb 12, 2016 at 12:32:06PM -0600, Josh Poimboeuf wrote:
> > What I actually see in the listing is:
> >
> > decl __percpu_prefix:__preempt_count
> > je 1f:
> > ....
> > 1:
> > call ___preempt_schedule
> >
> > So it puts the "call ___preempt_schedule" in the slow path.
>
> Ah yes indeed. Same difference though.
>
> > I also don't see how that would be related to the use of the asm
> > statement in the __preempt_schedule() macro. Doesn't the use of
> > unlikely() in preempt_enable() put the call in the slow path?
>
> Sadly no, unlikely() and asm_goto don't work well together. But the slow
> path or not isn't the reason we do the asm call thing.
>
> > #define preempt_enable() \
> > do { \
> > barrier(); \
> > if (unlikely(preempt_count_dec_and_test())) \
> > preempt_schedule(); \
> > } while (0)
> >
> > Also, why is the thunk needed? Any reason why preempt_enable() can't be
> > called directly from C?
>
> That would make the call-site save registers and increase the size of
> every preempt_enable(). By using the thunk we can do callee saved
> registers and avoid blowing up the call site.
So is the goal to optimize for size? If I replace the calls to
__preempt_schedule[_notrace]() with real C calls and remove the thunks,
it only adds about 2k to vmlinux.
There are two ways to fix the warnings:
1. get rid of the thunks and call the C functions directly; or
2. add the stack pointer to the asm() statement output operand list to
ensure a stack frame gets created in the caller function before the
call. (Note this still allows the thunks to do callee saved registers.)
I like #1 better, but maybe I'm still missing the point of the thunks.
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2016-02-15 18:00 +0100 |
| Message-ID | <r2uFc-3Dw-19@gated-at.bofh.it> |
| In reply to | #1334587 |
On Mon, Feb 15, 2016 at 10:31:34AM -0600, Josh Poimboeuf wrote:
> On Fri, Feb 12, 2016 at 09:10:11PM +0100, Peter Zijlstra wrote:
> > On Fri, Feb 12, 2016 at 12:32:06PM -0600, Josh Poimboeuf wrote:
> > > What I actually see in the listing is:
> > >
> > > decl __percpu_prefix:__preempt_count
> > > je 1f:
> > > ....
> > > 1:
> > > call ___preempt_schedule
> > >
> > > So it puts the "call ___preempt_schedule" in the slow path.
> >
> > Ah yes indeed. Same difference though.
> >
> > > I also don't see how that would be related to the use of the asm
> > > statement in the __preempt_schedule() macro. Doesn't the use of
> > > unlikely() in preempt_enable() put the call in the slow path?
> >
> > Sadly no, unlikely() and asm_goto don't work well together. But the slow
> > path or not isn't the reason we do the asm call thing.
> >
> > > #define preempt_enable() \
> > > do { \
> > > barrier(); \
> > > if (unlikely(preempt_count_dec_and_test())) \
> > > preempt_schedule(); \
> > > } while (0)
> > >
> > > Also, why is the thunk needed? Any reason why preempt_enable() can't be
> > > called directly from C?
> >
> > That would make the call-site save registers and increase the size of
> > every preempt_enable(). By using the thunk we can do callee saved
> > registers and avoid blowing up the call site.
>
> So is the goal to optimize for size?
General performance impact of preempt_enable().
> If I replace the calls to
> __preempt_schedule[_notrace]() with real C calls and remove the thunks,
> it only adds about 2k to vmlinux.
That's less than I had expected, but probably still worth it.
And is that added text purely in the slow path? We really want to avoid
putting any more register pressure on the preempt_enable() call sites.
The single memop and Jcc is about as fast we can get and we spend quite
a bit of effort getting there.
> There are two ways to fix the warnings:
>
> 1. get rid of the thunks and call the C functions directly; or
>
> 2. add the stack pointer to the asm() statement output operand list to
> ensure a stack frame gets created in the caller function before the
> call. (Note this still allows the thunks to do callee saved registers.)
>
> I like #1 better, but maybe I'm still missing the point of the thunks.
Ingo, Linus?
[toc] | [prev] | [next] | [standalone]
| From | Andi Kleen <andi@firstfloor.org> |
|---|---|
| Date | 2016-02-15 21:10 +0100 |
| Message-ID | <r2xD4-5Wh-19@gated-at.bofh.it> |
| In reply to | #1334587 |
> > There are two ways to fix the warnings: > > > > 1. get rid of the thunks and call the C functions directly; or > > No. Not until gcc learns about per-function callibg conventions (so that it can > be marked as not clobbering registers). It does already for static functions in 5.x (with -fipa-ra). And with LTO it can be used even somewhat globally. Even older version supported it, for only for x86->SSE on 32bit, which is useless for the kernel. But the new IPA-RA propagates which registers are clobbered. That said it will probably be a long time until we can drop support for older compilers. So for now the manual method is still needed. -Andi
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-02-15 21:10 +0100 |
| Message-ID | <r2xD5-5Wh-23@gated-at.bofh.it> |
| In reply to | #1334587 |
On Mon, Feb 15, 2016 at 08:56:21AM -0800, Linus Torvalds wrote:
> On Feb 15, 2016 8:31 AM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
> >
> > So is the goal to optimize for size? If I replace the calls to
> > __preempt_schedule[_notrace]() with real C calls and remove the thunks,
> > it only adds about 2k to vmlinux.
>
> It adds nasty register pressure in some of the most critical parts of the
> kernel, and makes the asm code harder to read.
>
> And yes, I still read the asm. For performance reasons, and when decoding
> oopses.
>
> I realize that few other people care about generated code. That's sad.
>
> > There are two ways to fix the warnings:
> >
> > 1. get rid of the thunks and call the C functions directly; or
>
> No. Not until gcc learns about per-function callibg conventions (so that it
> can be marked as not clobbering registers).
>
> > 2. add the stack pointer to the asm() statement output operand list to
> > ensure a stack frame gets created in the caller function before the
> > call.
>
> That probably doesn't make things much worse. It probably makes least
> functions have a stack frame if they do preempt disable, but it might still
> be acceptable. Hard to say before I end up hurting this case again.
Oddly, this change (see patch below) actually seems to make things
faster in a lot of cases. For many smaller functions it causes the
stack frame creation to get moved out of the common path and into the
unlikely path.
For example, here's the original cyc2ns_read_end():
ffffffff8101f8c0 <cyc2ns_read_end>:
ffffffff8101f8c0: 55 push %rbp
ffffffff8101f8c1: 48 89 e5 mov %rsp,%rbp
ffffffff8101f8c4: 83 6f 10 01 subl $0x1,0x10(%rdi)
ffffffff8101f8c8: 75 08 jne ffffffff8101f8d2 <cyc2ns_read_end+0x12>
ffffffff8101f8ca: 65 48 89 3d e6 5a ff mov %rdi,%gs:0x7eff5ae6(%rip) # 153b8 <cyc2ns+0x38>
ffffffff8101f8d1: 7e
ffffffff8101f8d2: 65 ff 0d 77 c4 fe 7e decl %gs:0x7efec477(%rip) # bd50 <__preempt_count>
ffffffff8101f8d9: 74 02 je ffffffff8101f8dd <cyc2ns_read_end+0x1d>
ffffffff8101f8db: 5d pop %rbp
ffffffff8101f8dc: c3 retq
ffffffff8101f8dd: e8 1e 37 fe ff callq ffffffff81003000 <___preempt_schedule>
ffffffff8101f8e2: 5d pop %rbp
ffffffff8101f8e3: c3 retq
ffffffff8101f8e4: 66 66 66 2e 0f 1f 84 data16 data16 nopw %cs:0x0(%rax,%rax,1)
ffffffff8101f8eb: 00 00 00 00 00
And here's the same function with the patch:
ffffffff8101f8c0 <cyc2ns_read_end>:
ffffffff8101f8c0: 83 6f 10 01 subl $0x1,0x10(%rdi)
ffffffff8101f8c4: 75 08 jne ffffffff8101f8ce <cyc2ns_read_end+0xe>
ffffffff8101f8c6: 65 48 89 3d ea 5a ff mov %rdi,%gs:0x7eff5aea(%rip) # 153b8 <cyc2ns+0x38>
ffffffff8101f8cd: 7e
ffffffff8101f8ce: 65 ff 0d 7b c4 fe 7e decl %gs:0x7efec47b(%rip) # bd50 <__preempt_count>
ffffffff8101f8d5: 74 01 je ffffffff8101f8d8 <cyc2ns_read_end+0x18>
ffffffff8101f8d7: c3 retq
ffffffff8101f8d8: 55 push %rbp
ffffffff8101f8d9: 48 89 e5 mov %rsp,%rbp
ffffffff8101f8dc: e8 1f 37 fe ff callq ffffffff81003000 <___preempt_schedule>
ffffffff8101f8e1: 5d pop %rbp
ffffffff8101f8e2: c3 retq
ffffffff8101f8e3: 66 66 66 66 2e 0f 1f data16 data16 data16 nopw %cs:0x0(%rax,%rax,1)
ffffffff8101f8ea: 84 00 00 00 00 00
Notice that it moved the frame pointer setup code to the unlikely
___preempt_schedule() call path. Going through a sampling of the
differences in the asm, that's the most common change I see.
Otherwise it has no real effect on callers which already have stack
frames (though it does change the ordering of some 'mov's).
And it has the intended effect of fixing the following warnings by
ensuring these call sites have stack frames:
stacktool: drivers/scsi/hpsa.o: hpsa_scsi_do_simple_cmd.constprop.106()+0x79: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x70: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x92: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_free()+0xff: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_free()+0xf5: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_free()+0x11a: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_get()+0x225: call without frame pointer save/setup
stacktool: kernel/locking/percpu-rwsem.o: percpu_up_read()+0x27: call without frame pointer save/setup
stacktool: kernel/profile.o: do_profile_hits.isra.5()+0x139: call without frame pointer save/setup
stacktool: lib/nmi_backtrace.o: nmi_trigger_all_cpu_backtrace()+0x2b6: call without frame pointer save/setup
stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_recv()+0x58: call without frame pointer save/setup
stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_send()+0x58: call without frame pointer save/setup
stacktool: net/rds/ib_recv.o: rds_ib_attempt_ack()+0xc1: call without frame pointer save/setup
stacktool: net/rds/iw_recv.o: rds_iw_attempt_ack()+0xc1: call without frame pointer save/setup
stacktool: net/rds/iw_recv.o: rds_iw_recv_cq_comp_handler()+0x55: call without frame pointer save/setup
So that only adds a stack frame to 15 call sites out of ~5000 calls to
___preempt_schedule[_notrace]. All the others already had stack frames.
Any idea for a good benchmark to run with the patch?
> The alternative is to just teach the tools about the magic issue of a few
> things like this.
I think that would be problematic for our goal of making stack traces of
sleeping tasks reliable. If preempt_enable()'s caller doesn't first
create a stack frame, the caller of the caller will get skipped in the
stack trace.
---8<---
diff --git a/arch/x86/include/asm/preempt.h b/arch/x86/include/asm/preempt.h
index 01bcde8..2989aa6 100644
--- a/arch/x86/include/asm/preempt.h
+++ b/arch/x86/include/asm/preempt.h
@@ -94,10 +94,19 @@ static __always_inline bool should_resched(int preempt_offset)
#ifdef CONFIG_PREEMPT
extern asmlinkage void ___preempt_schedule(void);
-# define __preempt_schedule() asm ("call ___preempt_schedule")
+# define __preempt_schedule() \
+({ \
+ register void *__sp asm(_ASM_SP); \
+ asm volatile ("call ___preempt_schedule" : "+r"(__sp)); \
+})
+
extern asmlinkage void preempt_schedule(void);
extern asmlinkage void ___preempt_schedule_notrace(void);
-# define __preempt_schedule_notrace() asm ("call ___preempt_schedule_notrace")
+# define __preempt_schedule_notrace() \
+({ \
+ register void *__sp asm(_ASM_SP); \
+ asm volatile ("call ___preempt_schedule_notrace" : "+r"(__sp)); \
+})
extern asmlinkage void preempt_schedule_notrace(void);
#endif
--
Josh
[toc] | [prev] | [next] | [standalone]
| From | Josh Poimboeuf <jpoimboe@redhat.com> |
|---|---|
| Date | 2016-02-18 18:50 +0100 |
| Subject | [PATCH] sched/x86: Add stack frame dependency to __preempt_schedule[_notrace] |
| Message-ID | <r3ASf-CA-25@gated-at.bofh.it> |
| In reply to | #1334784 |
If __preempt_schedule() or __preempt_schedule_notrace() is referenced at
the beginning of a function, gcc can insert the asm inline "call
___preempt_schedule[_notrace]" instruction before setting up a stack
frame, which breaks frame pointer convention if CONFIG_FRAME_POINTER is
enabled and can result in bad stack traces.
Force a stack frame to be created if CONFIG_FRAME_POINTER is enabled by
listing the stack pointer as an output operand for the inline asm
statements.
Specifically this fixes the following stacktool warnings:
stacktool: drivers/scsi/hpsa.o: hpsa_scsi_do_simple_cmd.constprop.106()+0x79: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x70: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_find_first()+0x92: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_free()+0xff: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_free()+0xf5: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_free()+0x11a: call without frame pointer save/setup
stacktool: fs/mbcache.o: mb_cache_entry_get()+0x225: call without frame pointer save/setup
stacktool: kernel/locking/percpu-rwsem.o: percpu_up_read()+0x27: call without frame pointer save/setup
stacktool: kernel/profile.o: do_profile_hits.isra.5()+0x139: call without frame pointer save/setup
stacktool: lib/nmi_backtrace.o: nmi_trigger_all_cpu_backtrace()+0x2b6: call without frame pointer save/setup
stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_recv()+0x58: call without frame pointer save/setup
stacktool: net/rds/ib_cm.o: rds_ib_cq_comp_handler_send()+0x58: call without frame pointer save/setup
stacktool: net/rds/ib_recv.o: rds_ib_attempt_ack()+0xc1: call without frame pointer save/setup
stacktool: net/rds/iw_recv.o: rds_iw_attempt_ack()+0xc1: call without frame pointer save/setup
stacktool: net/rds/iw_recv.o: rds_iw_recv_cq_comp_handler()+0x55: call without frame pointer save/setup
So it only adds a stack frame to 15 call sites out of ~5000 calls to
___preempt_schedule[_notrace]. All the others already had stack frames.
Oddly, this change actually seems to make things faster in a lot of
cases. For many smaller functions it causes the stack frame creation to
get moved out of the common path and into the unlikely path.
For example, here's the original cyc2ns_read_end():
ffffffff8101f8c0 <cyc2ns_read_end>:
ffffffff8101f8c0: 55 push %rbp
ffffffff8101f8c1: 48 89 e5 mov %rsp,%rbp
ffffffff8101f8c4: 83 6f 10 01 subl $0x1,0x10(%rdi)
ffffffff8101f8c8: 75 08 jne ffffffff8101f8d2 <cyc2ns_read_end+0x12>
ffffffff8101f8ca: 65 48 89 3d e6 5a ff mov %rdi,%gs:0x7eff5ae6(%rip) # 153b8 <cyc2ns+0x38>
ffffffff8101f8d1: 7e
ffffffff8101f8d2: 65 ff 0d 77 c4 fe 7e decl %gs:0x7efec477(%rip) # bd50 <__preempt_count>
ffffffff8101f8d9: 74 02 je ffffffff8101f8dd <cyc2ns_read_end+0x1d>
ffffffff8101f8db: 5d pop %rbp
ffffffff8101f8dc: c3 retq
ffffffff8101f8dd: e8 1e 37 fe ff callq ffffffff81003000 <___preempt_schedule>
ffffffff8101f8e2: 5d pop %rbp
ffffffff8101f8e3: c3 retq
ffffffff8101f8e4: 66 66 66 2e 0f 1f 84 data16 data16 nopw %cs:0x0(%rax,%rax,1)
ffffffff8101f8eb: 00 00 00 00 00
And here's the same function with the patch:
ffffffff8101f8c0 <cyc2ns_read_end>:
ffffffff8101f8c0: 83 6f 10 01 subl $0x1,0x10(%rdi)
ffffffff8101f8c4: 75 08 jne ffffffff8101f8ce <cyc2ns_read_end+0xe>
ffffffff8101f8c6: 65 48 89 3d ea 5a ff mov %rdi,%gs:0x7eff5aea(%rip) # 153b8 <cyc2ns+0x38>
ffffffff8101f8cd: 7e
ffffffff8101f8ce: 65 ff 0d 7b c4 fe 7e decl %gs:0x7efec47b(%rip) # bd50 <__preempt_count>
ffffffff8101f8d5: 74 01 je ffffffff8101f8d8 <cyc2ns_read_end+0x18>
ffffffff8101f8d7: c3 retq
ffffffff8101f8d8: 55 push %rbp
ffffffff8101f8d9: 48 89 e5 mov %rsp,%rbp
ffffffff8101f8dc: e8 1f 37 fe ff callq ffffffff81003000 <___preempt_schedule>
ffffffff8101f8e1: 5d pop %rbp
ffffffff8101f8e2: c3 retq
ffffffff8101f8e3: 66 66 66 66 2e 0f 1f data16 data16 data16 nopw %cs:0x0(%rax,%rax,1)
ffffffff8101f8ea: 84 00 00 00 00 00
Notice that it moved the frame pointer setup code to the unlikely
___preempt_schedule() call path. Going through a sampling of the
differences in the asm, that's the most common change I see.
Otherwise it has no real effect on callers which already have stack
frames (though it does result in the reordering of some 'mov's).
Reported-by: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
arch/x86/include/asm/preempt.h | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/preempt.h b/arch/x86/include/asm/preempt.h
index 01bcde8..d397deb 100644
--- a/arch/x86/include/asm/preempt.h
+++ b/arch/x86/include/asm/preempt.h
@@ -94,10 +94,19 @@ static __always_inline bool should_resched(int preempt_offset)
#ifdef CONFIG_PREEMPT
extern asmlinkage void ___preempt_schedule(void);
-# define __preempt_schedule() asm ("call ___preempt_schedule")
+# define __preempt_schedule() \
+({ \
+ register void *__sp asm(_ASM_SP); \
+ asm volatile ("call ___preempt_schedule" : "+r"(__sp)); \
+})
+
extern asmlinkage void preempt_schedule(void);
extern asmlinkage void ___preempt_schedule_notrace(void);
-# define __preempt_schedule_notrace() asm ("call ___preempt_schedule_notrace")
+# define __preempt_schedule_notrace() \
+({ \
+ register void *__sp asm(_ASM_SP); \
+ asm volatile ("call ___preempt_schedule_notrace" : "+r"(__sp)); \
+})
extern asmlinkage void preempt_schedule_notrace(void);
#endif
--
2.4.3
[toc] | [prev] | [next] | [standalone]
| From | Jiri Slaby <jslaby@suse.cz> |
|---|---|
| Date | 2016-02-19 13:10 +0100 |
| Subject | Re: [PATCH] sched/x86: Add stack frame dependency to __preempt_schedule[_notrace] |
| Message-ID | <r3S2K-4Vi-25@gated-at.bofh.it> |
| In reply to | #1337586 |
On 02/18/2016, 06:41 PM, Josh Poimboeuf wrote:
> If __preempt_schedule() or __preempt_schedule_notrace() is referenced at
> the beginning of a function, gcc can insert the asm inline "call
> ___preempt_schedule[_notrace]" instruction before setting up a stack
> frame, which breaks frame pointer convention if CONFIG_FRAME_POINTER is
> enabled and can result in bad stack traces.
>
> Force a stack frame to be created if CONFIG_FRAME_POINTER is enabled by
> listing the stack pointer as an output operand for the inline asm
> statements.
>
> Specifically this fixes the following stacktool warnings:
>
> stacktool: drivers/scsi/hpsa.o: hpsa_scsi_do_simple_cmd.constprop.106()+0x79: call without frame pointer save/setup
...
> Reported-by: Jiri Slaby <jslaby@suse.cz>
This patch and adding lbug_with_loc to global_noreturns makes all
stacktool warnings go away here.
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
> arch/x86/include/asm/preempt.h | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/preempt.h b/arch/x86/include/asm/preempt.h
> index 01bcde8..d397deb 100644
> --- a/arch/x86/include/asm/preempt.h
> +++ b/arch/x86/include/asm/preempt.h
> @@ -94,10 +94,19 @@ static __always_inline bool should_resched(int preempt_offset)
>
> #ifdef CONFIG_PREEMPT
> extern asmlinkage void ___preempt_schedule(void);
> -# define __preempt_schedule() asm ("call ___preempt_schedule")
> +# define __preempt_schedule() \
> +({ \
> + register void *__sp asm(_ASM_SP); \
> + asm volatile ("call ___preempt_schedule" : "+r"(__sp)); \
> +})
> +
> extern asmlinkage void preempt_schedule(void);
> extern asmlinkage void ___preempt_schedule_notrace(void);
> -# define __preempt_schedule_notrace() asm ("call ___preempt_schedule_notrace")
> +# define __preempt_schedule_notrace() \
> +({ \
> + register void *__sp asm(_ASM_SP); \
> + asm volatile ("call ___preempt_schedule_notrace" : "+r"(__sp)); \
> +})
> extern asmlinkage void preempt_schedule_notrace(void);
> #endif
>
>
thanks,
--
js
suse labs
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web