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

[Pavel Tikhomirov]

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.

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.

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

-- 
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.