[Devel] [PATCH vz9] dm-ploop: fix and rework md updates

[Pavel Tikhomirov]

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.