[Devel] [RFC PATCH vz9 v6 02/62] dm-ploop: Use READ_ONCE/WRITE_ONCE to access md page data

[Alexander Atanasov]

On 21.01.25 9:04, Pavel Tikhomirov wrote:
> 
> 
> On 1/20/25 19:35, Alexander Atanasov wrote:
>> On 20.01.25 13:27, Pavel Tikhomirov wrote:
>>>
>>>
>>> On 1/20/25 19:09, Alexander Atanasov wrote:
>>>> On 20.01.25 12:54, Pavel Tikhomirov wrote:
>>>>>
>>>>>
>>>>> On 1/20/25 16:03, Alexander Atanasov wrote:
>>>>>> On 20.01.25 9:23, Pavel Tikhomirov wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 12/6/24 05:55, Alexander Atanasov wrote:
>>>>>>>> @@ -1090,9 +1090,9 @@ static int ploop_alloc_cluster(struct 
>>>>>>>> ploop *ploop, struct ploop_index_wb *piwb,
>>>>>>>>       clu -= piwb->page_id * PAGE_SIZE / sizeof(map_index_t) - 
>>>>>>>> PLOOP_MAP_OFFSET;
>>>>>>>>       to = piwb->kmpage;
>>>>>>>> -    if (to[clu]) {
>>>>>>>> +    if (READ_ONCE(to[clu])) {
>>>>>>>>           /* Already mapped by one of previous bios */
>>>>>>>> -        *dst_clu = to[clu];
>>>>>>>> +        *dst_clu = READ_ONCE(to[clu]);
>>>>>>>>           already_alloced = true;
>>>>>>>>       }
>>>>>>>
>>>>>>> The above hunk does not look good, we explicitly do 
>>>>>>> "READ_ONCE(to[clu])" twice, likely that's not what we want.
>>>>>>
>>>>>>
>>>>>> We can do without inner READ_ONCE but for consistency i think it 
>>>>>> is better. /it would be optimised by the compiler anyway/
>>>>>>
>>>>>> The idea is that if a clu is already mapped we read !=0 in the if,
>>>>>> inside we will always read the !=0 too, since discard is 
>>>>>> synchronized by the holes bitmap.
>>>>>
>>>>> That may be, if clu can't be un-mapped or somehow reset... Properly 
>>>>> validating it will require checking all 24 uses of ->kmpage.
>>>>
>>>>
>>>> md->kmpage is mapped at init time and unmapped at destroy time.
>>>> ( [01/62] dm-ploop: md_pages map all pages at creation time )
>>>
>>> What about kmap_local_page(piwb->bat_page)[clu], where it is set/ 
>>> unset? I don't see it set in ploop_alloc_md_page where we set md- 
>>> >kmpage in [01/62].
>>>
>>>> piwb->kmpage is gone in updated patches.
>>>
>>> This is from updated patch:
>>>
>>>          to = kmap_local_page(piwb->bat_page);
>>>          if (READ_ONCE(to[clu])) {
>>>                  /* Already mapped by one of previous bios */
>>>                  *dst_clu = READ_ONCE(to[clu]);
>>>                  already_alloced = true;
>>>          }
>>>
>>> We still have double-once situation.
>>
>> I havent updated yet (now i did)
>>
>>          to = kmap_local_page(piwb->bat_page);
>> -       if (to[clu]) {
>> +       tmp_clu = READ_ONCE(to[clu]);
>> +       if (tmp_clu) {
>>                  /* Already mapped by one of previous bios */
>> -               *dst_clu = to[clu];
>> +               *dst_clu = tmp_clu;
>>                  already_alloced = true;
>>          }
>>
>>
>>
>>>
>>> The rule of thumb of using READ_ONCE is "If we use READ_ONCE to read 
>>> some shared variable, we get value, and we need to re-use this value 
>>> later instead of re-reading the shared variable.".
>>
>> READ_ONCE enforces 32bit access to the variable, nothing less nothing 
>> more. Without ONCE compiler can split access to two 16 bit operations 
>> / pipeline optimisation/ so if two threads access in parallel they can 
>> get partially updated value.
>>
>> READ_ONCE does not prevent the compiler to use cached value. neither 
>> it forces new read.
>>
>> Use depends on what the function does and expects.
>> If the function expects and wants to see a change it can read as much 
>> as it needs using ONCE.
> 
> dm-ploop: Use READ_ONCE/WRITE_ONCE to access md page data
> 
> Documentation/memory-barriers.txt
> 
>  > (*) It _must_not_ be assumed that the compiler will do what you want
>  >     with memory references that are not protected by READ_ONCE() and
>  >     WRITE_ONCE().  Without them, the compiler is within its rights to
>  >     do all sorts of "creative" transformations, which are covered in
>  >     the COMPILER BARRIER section.
> 
> 
>  >        (*) 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.
> 
> That basically says that READ_ONCE prevents tmp from changing it's 
> value. In other words value for "tmp" is read from "a" "once", as the 
> name of the macro suggests it would.
> 
> note: resending it, as probably my mail client lost it...


No, ONCE does not mean read once, it means access at once.
When it is a 32 bit variable compiler can split the access into two 16 
bit accesses to optimize instruction pipeline which can result in 
reading an inconsistent value from two threads. _ONCE macros force it to 
do 32 bit access which is atomic.

There is no point to do it twice in a function because once the value is 
read it is assigned to a variable (which can be anonymous too) second 
_ONCE does not force it to be reread again in the same function, as it 
is not volatile.

In the case of the patch , first read guaranteed that inner will not 
change in value.



> 
>>
>>
>>>
>>>>
>>>>
>>>>> It would be much cleaner and simpler to do:
>>>>>
>>>>> tmp_var = READ_ONCE(smth)
>>>>> if (tmp_var) {
>>>>>      *ptr_var = tmp_var;
>>>>> }
>>>>
>>>> Ok.
>>>>
>>>>> I doubt that you will find any other example of doing READ_ONCE of 
>>>>> the same shared memory/variable twice in one function.
>>>>
>>>> There might be but this is irrelevant.
>>>>
>>>> in this case it can be done without temp variable.
>>>>
>>>> if (READ_ONCE(var)) {
>>>>        *ptr_var = var;
>>>> }
>>>>
>>>> *ptr_var = var; will be changed to *ptr_var = 
>>>> value_from_read_once(var);
>>>>
>>>> so it is the same
>>>>
>>>
>>> Sorry, but in my opinion there is a difference.
>>>
>>
> 

-- 
Regards,
Alexander Atanasov