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
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
