| Message ID | 1418760534-18163-4-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Jan, On 17/12/14 10:05, Jan Beulich wrote: >>>> On 16.12.14 at 21:08, <julien.grall@linaro.org> wrote: >> --- a/xen/include/xen/compiler.h >> +++ b/xen/include/xen/compiler.h >> @@ -90,4 +90,18 @@ >> __asm__ ("" : "=r"(__ptr) : "0"(ptr)); \ >> (typeof(ptr)) (__ptr + (off)); }) >> >> +/* >> + * Prevent the compiler from merging or refetching accesses. The compiler >> + * is also forbidden from reordering successive instances of ACCESS_ONCE(), >> + * but only when the compiler is aware of some particular ordering. One way >> + * to make the compiler aware of ordering is to put the two invocations of >> + * ACCESS_ONCE() in different C statements. >> + * >> + * This macro does absolutely -nothing- to prevent the CPU from reordering, >> + * merging, or refetching absolutely anything at any time. Its main intended >> + * use is to mediate communication between process-level code and irq/NMI >> + * handlers, all running on the same CPU. >> + */ >> +#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x)) > > Any reason not to simply use {read,write}_atomic() instead, which we > already have? To avoid modifying Linux drivers when it's not necessary and doesn't harm. Regards,
Hi Andrew Cooper, On 17/12/2014 17:52, Andrew Cooper wrote: > On 17/12/14 17:10, Jan Beulich wrote: >>>>> Julien Grall <julien.grall@linaro.org> 12/17/14 1:55 PM >>> >>> On 17/12/14 10:05, Jan Beulich wrote: >>>>>> On 16.12.14 at 21:08, <julien.grall@linaro.org> wrote: >>>>> +#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x)) >>>> Any reason not to simply use {read,write}_atomic() instead, which we >>>> already have? >>> To avoid modifying Linux drivers when it's not necessary and doesn't harm. >> I realize that's the motivation, but I also view it as problematic to have two >> different constructs doing the same thing. Defining the new one in terms of >> the existing ones doesn't seem possible (or else I would suggest that in >> order for the connection to be obvious). We'll see what other maintainers >> think... > > Personally, I find the semantics of ACCESS_ONCE() more intuitive than > read/write_atomic(), and it is certainly more familiar to Linux developers. > > Furthermore, ACCESS_ONCE() doesn't force an mov instruction if the > compiler can identify a better instruction to use. > > There are only a handful of user users of read/write_atomic(). It would > not be hard to make a blanket switch, if we chose to go in that direction. Do you mean replacing read/write_atomic() by #define read_atomic(p) ACCESS_ONCE(p) #define write_atomic(p, x) (ACCESS_ONCE(p) = x) Regards,
Hi Andrew, On 17/12/2014 17:52, Andrew Cooper wrote: > On 17/12/14 17:10, Jan Beulich wrote: >>>>> Julien Grall <julien.grall@linaro.org> 12/17/14 1:55 PM >>> >>> On 17/12/14 10:05, Jan Beulich wrote: >>>>>> On 16.12.14 at 21:08, <julien.grall@linaro.org> wrote: >>>>> +#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x)) >>>> Any reason not to simply use {read,write}_atomic() instead, which we >>>> already have? >>> To avoid modifying Linux drivers when it's not necessary and doesn't harm. >> I realize that's the motivation, but I also view it as problematic to have two >> different constructs doing the same thing. Defining the new one in terms of >> the existing ones doesn't seem possible (or else I would suggest that in >> order for the connection to be obvious). We'll see what other maintainers >> think... > > Personally, I find the semantics of ACCESS_ONCE() more intuitive than > read/write_atomic(), and it is certainly more familiar to Linux developers. > > Furthermore, ACCESS_ONCE() doesn't force an mov instruction if the > compiler can identify a better instruction to use. > > There are only a handful of user users of read/write_atomic(). It would > not be hard to make a blanket switch, if we chose to go in that direction. Do you mean replacing read/write_atomic() by #define read_atomic(p) ACCESS_ONCE(p) #define write_atomic(p, x) (ACCESS_ONCE(p) = x) Regards,
Hi, On 17/12/14 17:52, Andrew Cooper wrote: > On 17/12/14 17:10, Jan Beulich wrote: >>>>> Julien Grall <julien.grall@linaro.org> 12/17/14 1:55 PM >>> >>> On 17/12/14 10:05, Jan Beulich wrote: >>>>>> On 16.12.14 at 21:08, <julien.grall@linaro.org> wrote: >>>>> +#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x)) >>>> Any reason not to simply use {read,write}_atomic() instead, which we >>>> already have? >>> To avoid modifying Linux drivers when it's not necessary and doesn't harm. >> I realize that's the motivation, but I also view it as problematic to have two >> different constructs doing the same thing. Defining the new one in terms of >> the existing ones doesn't seem possible (or else I would suggest that in >> order for the connection to be obvious). We'll see what other maintainers >> think... > > Personally, I find the semantics of ACCESS_ONCE() more intuitive than > read/write_atomic(), and it is certainly more familiar to Linux developers. > > Furthermore, ACCESS_ONCE() doesn't force an mov instruction if the > compiler can identify a better instruction to use. > > There are only a handful of user users of read/write_atomic(). It would > not be hard to make a blanket switch, if we chose to go in that direction. It seems the next version of Linux will start to replace ACCESS_ONCE by READ_ONCE/WRITE_ONCE. So I will drop this patch and declare ACCESS_ONCE in the SMMU driver. I will keep it as long as Linux is still using ACCESS_ONCE in this driver. Regards,
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h index 4b3472d..57eb7a6 100644 --- a/xen/include/xen/compiler.h +++ b/xen/include/xen/compiler.h @@ -90,4 +90,18 @@ __asm__ ("" : "=r"(__ptr) : "0"(ptr)); \ (typeof(ptr)) (__ptr + (off)); }) +/* + * Prevent the compiler from merging or refetching accesses. The compiler + * is also forbidden from reordering successive instances of ACCESS_ONCE(), + * but only when the compiler is aware of some particular ordering. One way + * to make the compiler aware of ordering is to put the two invocations of + * ACCESS_ONCE() in different C statements. + * + * This macro does absolutely -nothing- to prevent the CPU from reordering, + * merging, or refetching absolutely anything at any time. Its main intended + * use is to mediate communication between process-level code and irq/NMI + * handlers, all running on the same CPU. + */ +#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x)) + #endif /* __LINUX_COMPILER_H */
This macro can be used in drivers imported from Linux. Signed-off-by: Julien Grall <julien.grall@linaro.org> CC: Ian Jackson <ian.jackson@eu.citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Keir Fraser <keir@xen.org> --- xen/include/xen/compiler.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+)