search

sched-ext / scx

sched_ext schedulers and tools
https://bit.ly/scx_slack
GNU General Public License v2.0
692 stars 48 forks source link

scx_lavd: Adding READ_ONCE()/WRITE_ONCE() macros #322

Closed vax-r closed 1 month ago

vax-r commented 1 month ago

In order to prevent compiler from merging or refetching load/store operations or unwanted reordering, we take the implemetation of READ_ONCE()/WRITE_ONCE() from kernel sources and add them into scheds/include/scx/common.bpf.h.

Use WRITE_ONCE() in function flip_sys_cpu_util() to ensure the compiler doesn't perform unnecessary optimization so the modification of bit flipping can be visible to readers as fast as possible.

vax-r commented 1 month ago

This looks great in general. A few points:

  • Let's add a comment explaining that this was lifted from the kernel tree.
  • The use of WRITE_ONCE() doesn't have too much to do with the "speed" of the update but more correctness of it. Without WRITE_ONCE(), the compiler for example is allowed to generate split updates or skip them altogether.
  • It's often necessary and a best practice to pair WRITE_ONCE() usages with READ_ONCE(). If the variable is being updated in a way that the compiler may not be expecting, the compiler will most likely be able to make incorrect assumptions when reading it too. So, please also update the reading of the variable.

@htejun - Thanks for the suggestions ! I've made the corresponding changes , please take a look if you're available.

Btw, I want to ask what's the actual difference between the GNU built-in functions __sync_* and the READ_ONCE()/WRITE_ONCE() macros ? as I read the content in memory-barrier.txt , I thought READ_ONCE()/WRITE_ONCE() can make gaurantees on execution order, also the "happen-before" relationship between different statements . Can't it be consider as a memory barrier with relaxed memory order ?

vax-r commented 1 month ago

@htejun - The compile will failed after using WRITE_ONCE(__sys_cpu_util_idx, READ_ONCE(__sys_cpu_util_idx ^ 0x1)); , since the macro READ_ONCE() cannot take the address of an rvalue of type 'int' . What do you think if we add volatile qualifier back ? or instead use WRITE_ONCE(__sys_cpu_util_idx, READ_ONCE(__sys_cpu_util_idx) ^ 0x1); ?

Although you say a single READ_ONCE(__sys_cpu_util_idx) doesn't really do anything, it can guarantee the execution order of the loading operation for __sys_cpu_util_idx to be in front of the store operation of it , since these two statements exist dependency between them. That's my understanding, what do you think ?

htejun commented 1 month ago

Oh yeah, READ_ONCE() should only be on the variable itself but really no need to have it in the write path. There are no ordering concerns here.

vax-r commented 1 month ago

Oh yeah, READ_ONCE() should only be on the variable itself but really no need to have it in the write path. There are no ordering concerns here.

I see, I've made the requested changes. Please have a look while you're available, thanks !

multics69 commented 1 month ago

LGTM, thanks!