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

scx_lavd: Enforce memory barrier in flip_sys_cpu_util #318

Merged
merged 1 commit into from
May 26, 2024

Conversation

vax-r
Copy link
Contributor

@vax-r vax-r commented May 26, 2024

Use the GNU built-in __sync_fetch_and_xor() to perform the XOR operation on global variable "__sys_cpu_util_idx" to ensure the operations visibility.

The built-in function "__sync_fetch_and_xor()" can provide both atomic operation and full memory barrier which is needed by every operation (especially store operation) on global variables.

@vax-r vax-r changed the title scx_lavd: Enforce memory in flip_sys_cpu_util scx_lavd: Enforce memory barrier in flip_sys_cpu_util May 26, 2024
Use the GNU built-in __sync_fetch_and_xor() to perform the XOR operation
on global variable "__sys_cpu_util_idx" to ensure the operations
visibility.

The built-in function "__sync_fetch_and_xor()" can provide both atomic
operation and full memory barrier which is needed by every operation
(especially store operation) on global variables.

Signed-off-by: I Hsin Cheng <[email protected]>
Copy link
Contributor

@multics69 multics69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Thanks!

@multics69 multics69 merged commit 0371cca into sched-ext:main May 26, 2024
1 check passed
@htejun
Copy link
Contributor

htejun commented May 26, 2024

This doesn't harm anything but also doesn't improve anything either. I think it'd be useful to clarify so that we don't end up adding unnecessary atomic ops which can give false sense of security and obfuscate the code.

Operations on global variables don't necessarily require atomic ops. Here, there is only one writer - the timer, the update itself is always atomic (no split ops), and the readings aren't interlocked in any way. For writer's POV, it makes no difference whether the update op is atomic or not as it's the only writer. From readers' POV, it doesn't make any difference either as whether the writer side is using sync op or not, all the reader can observe is the bit flipping at some point.

While it doesn't do any direct harm, I'd much prefer this PR to be reverted. Unnecessary synchronization constructs can be really confusing for readers as they have to guess and hunt for the non-existent reasons.

@htejun
Copy link
Contributor

htejun commented May 26, 2024

BTW, what we need here isn't sync op on the writer side but WRITE_ONCE() and READ_ONCE() pair to tell the compiler to avoid optimizing the writes and reads by assuming they would maintain a certain value. We should add them to common.bpf.h and use them instead.

@multics69
Copy link
Contributor

@htejun -- Thank you for the comment. I missed the timer will be run in the interrupt context so it will be automatically synched when returning back. I will revert the PR.

Yes, READ/WRITE_ONCE() will be very useful.

@vax-r
Copy link
Contributor Author

vax-r commented May 27, 2024

@htejun Thanks for the detailed explanation, I thought __sync ops had the same functionality as READ_ONCE()/WRITE_ONCE() , atomic store/load with relaxed memory order .

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.

3 participants