Re: [RFC PATCH 1/4] hazptr: Add initial implementation of hazard pointers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2024-09-25 14:16, Boqun Feng wrote:
On Wed, Sep 25, 2024 at 01:59:06PM +0200, Mathieu Desnoyers wrote:
On 2024-09-25 12:45, Boqun Feng wrote:
On Wed, Sep 25, 2024 at 12:11:52PM +0200, Jonas Oberhauser wrote:


Am 9/25/2024 um 12:02 PM schrieb Boqun Feng:
Hi Jonas,

Of
course, if we are really worried about compilers being too "smart"

Ah, I see you know me better and better...

we can always do the comparison in asm code, then compilers don't know
anything of the equality between 'ptr' and 'head - head_offset'.
Yes, but then a simple compiler barrier between the comparison and returning
ptr would also do the trick, right? And maybe easier on the eyes.


The thing about putting a compiler barrier is that it will prevent all
compiler reorderings, and some of the reordering may contribute to
better codegen. (I know in this case, we have a smp_mb(), but still
compilers can move unrelated code upto the second load for optimization
purpose). Asm comparison is cheaper in this way. But TBH, compilers
should provide a way to compare pointer values without using the result
for pointer equality proof, if "convert to unsigned long" doesn't work,
some other ways should work.


Based on Documentation/RCU/rcu_dereference.rst :

-       Be very careful about comparing pointers obtained from
         rcu_dereference() against non-NULL values.  As Linus Torvalds
         explained, if the two pointers are equal, the compiler could
         substitute the pointer you are comparing against for the pointer
         obtained from rcu_dereference().  For example::

                 p = rcu_dereference(gp);
                 if (p == &default_struct)
                         do_default(p->a);

         Because the compiler now knows that the value of "p" is exactly
         the address of the variable "default_struct", it is free to
         transform this code into the following::

                 p = rcu_dereference(gp);
                 if (p == &default_struct)
                         do_default(default_struct.a);

         On ARM and Power hardware, the load from "default_struct.a"
         can now be speculated, such that it might happen before the
         rcu_dereference().  This could result in bugs due to misordering.

So I am not only concerned about compiler proofs here, as it appears
that the speculation done by the CPU can also cause issues on some
architectures.


Note that the issue is caused by compiler replacing one pointer with
the other, CPU speculation won't cause the issue solely, because all
architectures Linux supports honor address dependencies (except for
Alpha, but on Alpha, READ_ONCE() has a smp_mb() after the load). That
is: if the compiler generates code as the code is written, there should
be no problem.

I am starting to wonder if it would not be more robust to just bite
the bullet and add the inline asm helpers to do the pointer comparison
outside of the compiler knowledge for each architecture. This should
work fine in the short term, until compilers eventually give us a
"compare pointers without allowing the compiler to infer anything about
pointer equality".

Like so:

#include <stdio.h>

#define __str_1(x)  #x
#define __str(x)    __str_1(x)

/* x86-64 */
#define bne_ptr(_a, _b, _label) \
    asm goto ( \
        "cmpq %[a], %[b]\n\t" \
        "jne %l[" __str(_label) "]\n\t" \
        : : [a] "r" (_a), [b] "r" (_b) \
        : : _label)

int x;

int v[2];

int main(void)
{
    bne_ptr(v, v + 1, label_same);
    x = 1;
label_same:
    printf("%d\n", x);
    return 0;
}



Regards,
Boqun

Thanks,

Mathieu

Regards,
Boqun


Have fun,
     jonas


--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com


--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux