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

[Alexander Atanasov]

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.


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