On 2/12/25 17:42, Alexander Atanasov wrote:
If you really want to read md->piwb to piwb variable BEFORE
ploop_write_cluster_sync, you should use READ_ONCE, or other
appropriately used memory barrier.
READ_ONCE is not a memory barrier - it is a COMPILER BARRIER.
https://docs.kernel.org/core-api/wrappers/memory-barriers.html
see COMPILER BARRIER section for indeep explanation. There you will
see that infact READ_ONCE forces variable to be re-read , and not
read once as in one time.
Unrelated to the patch, as you've proved that patch is ok. But we've
already had this discussion https://lists.openvz.org/pipermail/
devel/2025-January/081790.html , and I've provided you with citation
from documentation from "COMPILER BARRIER". READ_ONCE disables
different compiler optimisations, including that it does not allow
local variable to be optimized by instead directly using the global
variable which can be changed in concurrent thread, which looks from
the user perspective as if local variable is read twice from global
variable. So it actually makes reads "one time" reads.
No, it actually marks the variable volatile /i missed that/ and forces
access as a whole. Which results in variable to be read every time
instead of cached one to be used, so it is not a one time read.
The example from the docs:
(*) The compiler is within its rights to merge successive loads from
the same variable. Such merging can cause the compiler to "optimize"
the following code:
while (tmp = a)
do_something_with(tmp);
into the following code, which, although in some sense legitimate
for single-threaded code, is almost certainly not what the developer
intended:
if (tmp = a)
for (;;)
do_something_with(tmp);
Use READ_ONCE() to prevent the compiler from doing this to you:
while (tmp = READ_ONCE(a))
do_something_with(tmp);
READ_ONCE(a) -> forces read every time - cast to volatile
and the side efect is it can not split access in two
The last paragaph is what you missed:
"All that aside, it is never necessary to use READ_ONCE() and
WRITE_ONCE() on a variable that has been marked volatile. For example,
because 'jiffies' is marked volatile, it is never necessary to
say READ_ONCE(jiffies). The reason for this is that READ_ONCE() and
WRITE_ONCE() are implemented as volatile casts, which has no effect when
its argument is already marked volatile.
Please note that these compiler barriers have no direct effect on the
CPU, which may then reorder things however it wishes."
Yes we did but let's get on the same page.
Yes, I agree that the repeated READ_ONCE forces variable to update each
time. As I previously say "READ_ONCE disables different compiler
optimisations", so I don't argue that READ_ONCE does many things. But
ALSO the single READ_ONCE forces variable to never be reloaded more then
once. As in:
(*) The compiler is within its rights to reload a variable, for example,
in cases where high register pressure prevents the compiler from
keeping all data of interest in registers. The compiler might
therefore optimize the variable 'tmp' out of our previous example:
while (tmp = a)
do_something_with(tmp);
This could result in the following code, which is perfectly safe in
single-threaded code, but can be fatal in concurrent code:
while (a)
do_something_with(a);
For example, the optimized version of this code could result in
passing a zero to do_something_with() in the case where the variable
a was modified by some other CPU between the "while" statement and
the call to do_something_with().
Again, use READ_ONCE() to prevent the compiler from doing this:
while (tmp = READ_ONCE(a))
do_something_with(tmp);
Note that if the compiler runs short of registers, it might save
tmp onto the stack. The overhead of this saving and later restoring
is why compilers reload variables. Doing so is perfectly safe for
single-threaded code, so you need to tell the compiler about cases
where it is not safe.
--
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
