Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix compiler optimize thread local variable access #2918

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zcfh
Copy link

@zcfh zcfh commented Mar 18, 2025

What problem does this PR solve?

Issue Number:

Problem Summary:
bthread_usleep/bthread_yield 包含对 tls_task_group 的访问,在LTO模式下,会触发 #2156 提到的问题。在example下会出现 bthread=4294967808 sched_to itself!的错误,实际服务中会导致程序出core。

复现方式

  1. 使用编译器clang-17.0.6 (测试过 clang-11不会)
  2. 增加编译选项 -flto=thin,
  3. 新增链接选项 -fuse-ld=lld
  4. 编译 parallel_echo_c++
  5. 执行 ./parallel_echo_client --use_bthread=trueCtrl + C 退出,server可以不需要启动。

汇编对比

clang-11 + thinlto。会内联bthread_usleep,但不会做tls变量访问的优化,此时不会出现问题。

0000000000781ba0 <brpc::SocketMap::WatchConnections()>:
; brpc::SocketMap::WatchConnections():
; /home/wuminghui03/workspace/svn/brpc/src/brpc/socket_map.cpp:354
  ......
; /home/wuminghui03/workspace/svn/brpc/src/brpc/socket_map.cpp:359
  781c10: bf 40 42 0f 00               	movl	$0xf4240, %edi          # imm = 0xF4240
  781c15: e8 46 98 fc ff               	callq	0x74b460 <bthread_usleep>
  ......
  781ca0: 0f 84 6a ff ff ff            	je	0x781c10 <brpc::SocketMap::WatchConnections()+0x70>
  ......

000000000074b460 <bthread_usleep>:
; bthread_usleep():
  ......
; /home/wuminghui03/workspace/svn/brpc/src/bthread/bthread.cpp:525
  74b46b: 64 48 8b 04 25 38 fe ff ff   	movq	%fs:-0x1c8, %rax
  74b474: 48 89 45 f8                  	movq	%rax, -0x8(%rbp)
  ......

clang-17 + thinlto, 在循环外缓存了tls变量的地址,此时会出现问题

0000000000781bf0 <brpc::SocketMap::WatchConnections()>:
; brpc::SocketMap::WatchConnections():
; /home/wuminghui03/workspace/svn/brpc/src/brpc/socket_map.cpp:354
  ......
  781c4a: 64 48 8b 04 25 00 00 00 00   	movq	%fs:0x0, %rax
  781c53: 48 8d 80 38 fe ff ff         	leaq	-0x1c8(%rax), %rax
  781c5a: 48 89 85 48 ff ff ff         	movq	%rax, -0xb8(%rbp)
  781c61: 31 c0                        	xorl	%eax, %eax
  781c63: 48 89 45 a0                  	movq	%rax, -0x60(%rbp)
  781c67: 4c 8d 6d c8                  	leaq	-0x38(%rbp), %r13
  781c6b: 48 89 7d a8                  	movq	%rdi, -0x58(%rbp)
  781c6f: 90                           	nop
  781c70: 48 8b 85 48 ff ff ff         	movq	-0xb8(%rbp), %rax
; /home/wuminghui03/workspace/svn/brpc/src/bthread/bthread.cpp:525
  781c77: 48 8b 00                     	movq	(%rax), %rax
  781c7a: 48 89 45 d0                  	movq	%rax, -0x30(%rbp)
  ......
  781d2f: 0f 84 3b ff ff ff            	je	0x781c70 <brpc::SocketMap::WatchConnections()+0x80>
  ......

What is changed and the side effects?

Changed:

bthread_usleep 和 bthread_yield 禁用inline优化。

替代方案

使用 BAIDU_GET_VOLATILE_THREAD_LOCAL 宏的方式,访问 tls_task_group,但只要避免bthread_usleep,被inline,就不会做后续的优化。看各位有什么建议。

Side effects:

  • Performance effects(性能影响):
    对于非LTO模式下,无影响;
    对于LTO模式下,会让bthread_usleep 和 bthread_yield 不再被内联。
  • Breaking backward compatibility(向后兼容性):

Check List:

  • Please make sure your changes are compilable(请确保你的更改可以通过编译).
  • When providing us with a new feature, it is best to add related tests(如果你向我们增加一个新的功能, 请添加相关测试).
  • Please follow Contributor Covenant Code of Conduct.(请遵循贡献者准则).

@wwbmmm
Copy link
Contributor

wwbmmm commented Mar 18, 2025

为什么不使用 BAIDU_GET_VOLATILE_THREAD_LOCAL 宏的方式?

@zcfh
Copy link
Author

zcfh commented Mar 18, 2025

为什么不使用 BAIDU_GET_VOLATILE_THREAD_LOCAL 宏的方式?

这里我的考虑是,使用 BAIDU_GET_VOLATILE_THREAD_LOCAL 的方式,在非LTO模式下,多了一次函数调用。
只要让它在LTO模式下不inline,那编译器就不会去做优化,这样影响更小些。其他地方因为在一个函数内,会多次访问tls_task_group,所以才需要 BAIDU_GET_VOLATILE_THREAD_LOCAL。

当然,这只是我的考量,如果你们更建议以 BAIDU_GET_VOLATILE_THREAD_LOCAL 的方式,那就还是用 BAIDU_GET_VOLATILE_THREAD_LOCAL。@wwbmmm

@wwbmmm
Copy link
Contributor

wwbmmm commented Mar 18, 2025

我觉得问题的根本原因是编译器对TLS变量访问的优化,而不是内联优化,所以使用BAIDU_GET_VOLATILE_THREAD_LOCAL,是更根本的改法。
如果要区分一个函数内是一次调用还是多次调用,来使用不同的改法,这样维护成本会比较高。未来随着代码的改动,调用次数可能会发生变化,最好还是采用一种统一的改法。

`bthread_usleep`/`bthread_yield` contains access to TLS variables.
In LTO mode, exceptions may occur due to cross-module optimization.
For example, bthread_usleep is inlined into WatchConnections,
the compiler(clang-17.0.6) cache the address outside the loop,
triggering the error mentioned in apache#2156.
@zcfh zcfh force-pushed the fix-tls-variable-load branch from 5fcdca5 to e951ffb Compare March 18, 2025 11:43
@zcfh
Copy link
Author

zcfh commented Mar 18, 2025

重新提交了一版。
另外bthread.cpp内,还有其他访问tls_task_group的情况,我没做修改。因为从我的测试排查来看,它们没有出现类似的问题。还请帮忙确认一下这个结论是否正确。

@wwbmmm
Copy link
Contributor

wwbmmm commented Mar 19, 2025

LGTM

@zcfh
Copy link
Author

zcfh commented Mar 21, 2025

还有几个问题请教下。

  1. 还会有其他类似的场景吗?实现是在cpp里,只是非LTO模式下,不触发这个问题。
  2. 这个pr的合入后续还需要我这里做什么吗?

@wwbmmm
Copy link
Contributor

wwbmmm commented Mar 21, 2025

还有几个问题请教下。

  1. 还会有其他类似的场景吗?实现是在cpp里,只是非LTO模式下,不触发这个问题。

不确定,主要编译器的逻辑会不断迭代,不确定后面还会不会在别的场景触发问题,只能case by case解决了。

  1. 这个pr的合入后续还需要我这里做什么吗?

不用了,等其他开发者看看还有没有问题,没问题就合入

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants