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