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