scx_lavd: Enforce memory barrier in flip_sys_cpu_util

[vax-r]

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.

[htejun]

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]

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]

@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]

@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 .