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

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

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