Skip to content

Commit 358864a

Browse files
committed
common: take [[noreturn]] out of ceph_assert
The cold handlers of `ceph_assert` are marked as `noreturn`. As bypassing the `abort()` within them requires the ability to resume execution after the assert is hit, compilers are not allowed anymore to optimize these returns out. To estimate the impact on performance, let's analyze a snippet and the differences in the correpsonding machine codes. \### The subject ``` void PeeringState::clear_blocked_outgoing() { ceph_assert(orig_ctx); ceph_assert(rctx); messages_pending_flush = std::optional<BufferedRecoveryMessages>(); } ``` \### The hot, assert-free paths before the change ``` 0000000000e3cf90 <PeeringState::clear_blocked_outgoing()>: e3cf90: 48 83 ec 08 sub $0x8,%rsp e3cf94: 48 83 bf e0 00 00 00 cmpq $0x0,0xe0(%rdi) e3cf9b: 00 e3cf9c: 74 3d je e3cfdb <PeeringState::clear_blocked_outgoing()+0x4b> e3cf9e: 80 bf 40 01 00 00 00 cmpb $0x0,0x140(%rdi) e3cfa5: 74 28 je e3cfcf <PeeringState::clear_blocked_outgoing()+0x3f> e3cfa7: 80 bf 18 01 00 00 00 cmpb $0x0,0x118(%rdi) e3cfae: 75 08 jne e3cfb8 <PeeringState::clear_blocked_outgoing()+0x28> e3cfb0: 48 83 c4 08 add $0x8,%rsp e3cfb4: c3 retq ... e3cfb8: c6 87 18 01 00 00 00 movb $0x0,0x118(%rdi) e3cfbf: 48 8b bf f8 00 00 00 mov 0xf8(%rdi),%rdi e3cfc6: 48 83 c4 08 add $0x8,%rsp e3cfca: e9 01 bd ff ff jmpq e38cd0 <std::_Rb_tree<int, std::pair<int const, std::vector<boost::intrusive_ptr<Message>, std::allocator<boost::intrusive_ptr<Message> > > >, std::_Select1st<std::pair<int const, std::vector<boost::intrusive_ptr<Message>, std::allocator<boost::intrusive_ptr<Message> > > > >, std::less<int>, std::allocator<std::pair<int const, std::vector<boost::intrusive_ptr<Message>, std::allocator<boost::intrusive_ptr<Message> > > > > >::_M_erase(std::_Rb_tree_node<std::pair<int const, std::vector<boost::intrusive_ptr<Message>, std::allocator<boost::intrusive_ptr<Message> > > > >*) [clone .isra.0]> ``` \### The hot, assert-free paths after the change ``` 0000000000e424e0 <PeeringState::clear_blocked_outgoing()>: e424e0: 53 push %rbx e424e1: 48 83 bf e0 00 00 00 cmpq $0x0,0xe0(%rdi) e424e8: 00 e424e9: 48 89 fb mov %rdi,%rbx e424ec: 74 4a je e42538 <PeeringState::clear_blocked_outgoing()+0x58> e424ee: 80 bb 40 01 00 00 00 cmpb $0x0,0x140(%rbx) e424f5: 74 11 je e42508 <PeeringState::clear_blocked_outgoing()+0x28> e424f7: 80 bb 18 01 00 00 00 cmpb $0x0,0x118(%rbx) e424fe: 75 1d jne e4251d <PeeringState::clear_blocked_outgoing()+0x3d> e42500: 5b pop %rbx e42501: c3 retq ... e4251d: c6 83 18 01 00 00 00 movb $0x0,0x118(%rbx) e42524: 48 8b bb f8 00 00 00 mov 0xf8(%rbx),%rdi e4252b: 5b pop %rbx e4252c: e9 9f bc ff ff jmpq e3e1d0 <std::_Rb_tree<int, std::pair<int const, std::vector<boost::intrusive_ptr<Message>, std::allocator<boost::intrusive_ptr<Message> > > >, std::_Select1st<std::pair<int const, std::vector<boost::intrusive_ptr<Message>, std::allocator<boost::intrusive_ptr<Message> > > > >, std::less<int>, std::allocator<std::pair<int const, std::vector<boost::intrusive_ptr<Message>, std::allocator<boost::intrusive_ptr<Message> > > > > >::_M_erase(std::_Rb_tree_node<std::pair<int const, std::vector<boost::intrusive_ptr<Message>, std::allocator<boost::intrusive_ptr<Message> > > > >*) [clone .isra.0]> ``` The difference is the compiler replaced the `sub $0x8,%rsp` and `add $0x8,%rsp` instructions with `push %rbx` and `pop %rbd`, which – in addition to managing the stack pointer – also store the register's value. The rationale is the cold, `unlikely` path of the `ceph_assert` macro prepares the `assert_data_ctx` which is then passed via `%rdi` to the `__ceph_assert_fail(ceph::assert_data const&)`. As this cold handler n may return, the original value of register cannot be thrashed. On the `unlikely` paths this is visible as the extra `jmp`: ``` e42538: 48 8d 3d e1 32 29 01 lea 0x12932e1(%rip),%rdi # 20d5820 <PeeringState::clear_blocked_outgoing()::assert_data_ctx> e4253f: e8 26 83 b6 ff callq 9aa86a <ceph::__ceph_assert_fail(ceph::assert_data const&)> e42544: eb a8 jmp e424ee <PeeringState::clear_blocked_outgoing()+0xe> ``` In short: on the hot paths the cost is just the burden to not thrash the register for passing the agument to the cold `__ceph_assert_fail`. Fixes: https://tracker.ceph.com/issues/70476 Signed-off-by: Radoslaw Zarzynski <[email protected]>
1 parent b1e4a2b commit 358864a

File tree

1 file changed

+3
-6
lines changed

1 file changed

+3
-6
lines changed

src/include/ceph_assert.h

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,13 +50,10 @@ struct assert_data {
5050
const char *function;
5151
};
5252

53-
extern void __ceph_assert_fail(const char *assertion, const char *file, int line, const char *function)
54-
__attribute__ ((__noreturn__));
55-
extern void __ceph_assert_fail(const assert_data &ctx)
56-
__attribute__ ((__noreturn__));
53+
extern void __ceph_assert_fail(const char *assertion, const char *file, int line, const char *function);
54+
extern void __ceph_assert_fail(const assert_data &ctx);
5755

58-
extern void __ceph_assertf_fail(const char *assertion, const char *file, int line, const char *function, const char* msg, ...)
59-
__attribute__ ((__noreturn__));
56+
extern void __ceph_assertf_fail(const char *assertion, const char *file, int line, const char *function, const char* msg, ...);
6057
extern void __ceph_assert_warn(const char *assertion, const char *file, int line, const char *function);
6158

6259
[[noreturn]] void __ceph_abort(const char *file, int line, const char *func,

0 commit comments

Comments
 (0)