| Message ID | 1347349912-15611-2-git-send-email-qemulist@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 09/11/2012 10:51 AM, Liu Ping Fan wrote: > From: Liu Ping Fan <pingfank@linux.vnet.ibm.com> > > If out of global lock, we will be challenged by SMP in low level, > so need atomic ops. > > This file is a wrapper of GCC atomic builtin. > > Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> > --- > include/qemu/atomic.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 63 insertions(+), 0 deletions(-) > create mode 100644 include/qemu/atomic.h > > diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h > new file mode 100644 > index 0000000..f17145d > --- /dev/null > +++ b/include/qemu/atomic.h > @@ -0,0 +1,63 @@ > +/* > + * Simple wrapper of gcc Atomic-Builtins > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + * > + */ > +#ifndef __QEMU_ATOMIC_H > +#define __QEMU_ATOMIC_H > + > +typedef struct Atomic { > + int counter; > +} Atomic; Best to mark counter 'volatile'. > + > +static inline void atomic_set(Atomic *v, int i) > +{ > + v->counter = i; > +} > + > +static inline int atomic_read(Atomic *v) > +{ > + return v->counter; > +} > So these two operations don't get mangled by the optimizer.
On 11 September 2012 08:51, Liu Ping Fan <qemulist@gmail.com> wrote: > + > +/** > + * * atomic_inc - increment atomic variable > + * * @v: pointer of type Atomic > + * * > + * * Atomically increments @v by 1. > + * */ Your editor has done something weird with these comments. -- PMM
On Tue, Sep 11, 2012 at 4:15 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 11 September 2012 08:51, Liu Ping Fan <qemulist@gmail.com> wrote: >> + >> +/** >> + * * atomic_inc - increment atomic variable >> + * * @v: pointer of type Atomic >> + * * >> + * * Atomically increments @v by 1. >> + * */ > > Your editor has done something weird with these comments. > Yes, will fix it. Thanx, pingfan > -- PMM
On Tue, Sep 11, 2012 at 4:04 PM, Avi Kivity <avi@redhat.com> wrote: > On 09/11/2012 10:51 AM, Liu Ping Fan wrote: >> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com> >> >> If out of global lock, we will be challenged by SMP in low level, >> so need atomic ops. >> >> This file is a wrapper of GCC atomic builtin. >> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> >> --- >> include/qemu/atomic.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 63 insertions(+), 0 deletions(-) >> create mode 100644 include/qemu/atomic.h >> >> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h >> new file mode 100644 >> index 0000000..f17145d >> --- /dev/null >> +++ b/include/qemu/atomic.h >> @@ -0,0 +1,63 @@ >> +/* >> + * Simple wrapper of gcc Atomic-Builtins >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or later. >> + * See the COPYING file in the top-level directory. >> + * >> + */ >> +#ifndef __QEMU_ATOMIC_H >> +#define __QEMU_ATOMIC_H >> + >> +typedef struct Atomic { >> + int counter; >> +} Atomic; > > Best to mark counter 'volatile'. > >> + >> +static inline void atomic_set(Atomic *v, int i) >> +{ >> + v->counter = i; >> +} >> + >> +static inline int atomic_read(Atomic *v) >> +{ >> + return v->counter; >> +} >> > > So these two operations don't get mangled by the optimizer. > Browsing linux code and reading lkml, find some similar material. But they have moved volatile from ->counter to function - atomic_read(). As to atomic_read(), I think it need to prevent optimizer from refetching issue, but as to atomic_set(), do we need ? Regards, pingfan > > > > -- > error compiling committee.c: too many arguments to function
On 09/13/2012 09:54 AM, liu ping fan wrote: >>> +typedef struct Atomic { >>> + int counter; >>> +} Atomic; >> >> Best to mark counter 'volatile'. >> >>> + >>> +static inline void atomic_set(Atomic *v, int i) >>> +{ >>> + v->counter = i; >>> +} >>> + >>> +static inline int atomic_read(Atomic *v) >>> +{ >>> + return v->counter; >>> +} >>> >> >> So these two operations don't get mangled by the optimizer. >> > Browsing linux code and reading lkml, find some similar material. But > they have moved volatile from ->counter to function - atomic_read(). > As to atomic_read(), I think it need to prevent optimizer from > refetching issue, but as to atomic_set(), do we need ? I think so, to prevent reordering.
Il 13/09/2012 10:14, Avi Kivity ha scritto: >>>> >>> +static inline void atomic_set(Atomic *v, int i) >>>> >>> +{ >>>> >>> + v->counter = i; >>>> >>> +} >>>> >>> + >>>> >>> +static inline int atomic_read(Atomic *v) >>>> >>> +{ >>>> >>> + return v->counter; >>>> >>> +} >>>> >>> >>> >> >>> >> So these two operations don't get mangled by the optimizer. >>> >> >> > Browsing linux code and reading lkml, find some similar material. But >> > they have moved volatile from ->counter to function - atomic_read(). >> > As to atomic_read(), I think it need to prevent optimizer from >> > refetching issue, but as to atomic_set(), do we need ? > I think so, to prevent reordering. Agreed. Alternatively, stick a barrier() before and after the assignment and read. But I don't really see the point in wrapping atomically-accessed variables in a struct. Are we going to add a variant for long, a variant for pointers, etc.? I already proposed my version of this at http://github.com/bonzini/qemu/commit/1b439343. Paolo
On 09/13/2012 11:19 AM, Paolo Bonzini wrote: > Il 13/09/2012 10:14, Avi Kivity ha scritto: >>>>> >>> +static inline void atomic_set(Atomic *v, int i) >>>>> >>> +{ >>>>> >>> + v->counter = i; >>>>> >>> +} >>>>> >>> + >>>>> >>> +static inline int atomic_read(Atomic *v) >>>>> >>> +{ >>>>> >>> + return v->counter; >>>>> >>> +} >>>>> >>> >>>> >> >>>> >> So these two operations don't get mangled by the optimizer. >>>> >> >>> > Browsing linux code and reading lkml, find some similar material. But >>> > they have moved volatile from ->counter to function - atomic_read(). >>> > As to atomic_read(), I think it need to prevent optimizer from >>> > refetching issue, but as to atomic_set(), do we need ? >> I think so, to prevent reordering. > > Agreed. Alternatively, stick a barrier() before and after the > assignment and read. > > But I don't really see the point in wrapping atomically-accessed > variables in a struct. Preventing accidental naked access (to be reported in patch review as "wardrobe malfunction"). > Are we going to add a variant for long, a > variant for pointers, etc.? template <typename T> ...; > I already proposed my version of this at > http://github.com/bonzini/qemu/commit/1b439343. Atomic operations are sufficiently rare and sufficiently important to warrant extra effort, so I prefer the explicitly typed variants.
Il 13/09/2012 10:23, Avi Kivity ha scritto: >> > But I don't really see the point in wrapping atomically-accessed >> > variables in a struct. > Preventing accidental naked access (to be reported in patch review as > "wardrobe malfunction"). Yeah, I understand that, but it's rare enough that I don't think it's worth the complication. With C++ and templates it would be a different story. Paolo
On Thu, Sep 13, 2012 at 4:19 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 13/09/2012 10:14, Avi Kivity ha scritto: >>>>> >>> +static inline void atomic_set(Atomic *v, int i) >>>>> >>> +{ >>>>> >>> + v->counter = i; >>>>> >>> +} >>>>> >>> + >>>>> >>> +static inline int atomic_read(Atomic *v) >>>>> >>> +{ >>>>> >>> + return v->counter; >>>>> >>> +} >>>>> >>> >>>> >> >>>> >> So these two operations don't get mangled by the optimizer. >>>> >> >>> > Browsing linux code and reading lkml, find some similar material. But >>> > they have moved volatile from ->counter to function - atomic_read(). >>> > As to atomic_read(), I think it need to prevent optimizer from >>> > refetching issue, but as to atomic_set(), do we need ? >> I think so, to prevent reordering. > > Agreed. Alternatively, stick a barrier() before and after the > assignment and read. > Yes, the linux just leave this barrier() as caller's choice at the exact point, not right in atomic_set(). And embed barrier() in atomic_set() can easy the caller's job. Thanks and regards, pingfan > But I don't really see the point in wrapping atomically-accessed > variables in a struct. Are we going to add a variant for long, a > variant for pointers, etc.? > > I already proposed my version of this at > http://github.com/bonzini/qemu/commit/1b439343. > > Paolo
Avi Kivity wrote: > On 09/13/2012 09:54 AM, liu ping fan wrote: > > >>> +typedef struct Atomic { > >>> + int counter; > >>> +} Atomic; > >> > >> Best to mark counter 'volatile'. > >> > >>> + > >>> +static inline void atomic_set(Atomic *v, int i) > >>> +{ > >>> + v->counter = i; > >>> +} > >>> + > >>> +static inline int atomic_read(Atomic *v) > >>> +{ > >>> + return v->counter; > >>> +} > >>> > >> > >> So these two operations don't get mangled by the optimizer. > >> > > Browsing linux code and reading lkml, find some similar material. But > > they have moved volatile from ->counter to function - atomic_read(). > > As to atomic_read(), I think it need to prevent optimizer from > > refetching issue, but as to atomic_set(), do we need ? > > I think so, to prevent reordering. Hi, I don't think volatile makes any difference to reordering here. The compiler is not going to move the atomic_set() store before or after another instruction on the same atomic variable anyway, just like it wouldn't do that for an ordinary assignment. If you're concerned about ordering with respect to other memory, then volatile wouldn't make much difference. barrier() before and after would. If you're copying Linux's semantics, Linux's atomic_set() doesn't include any barriers, nor imply any. atomic_read() uses volatile to ensure that each call re-reads the value, for example in a loop. (Same as ACCESS_ONCE()). If there was a call to atomic_set() in a loop, it doesn't guarantee that will be written each time. -- Jamie
liu ping fan wrote: > >> +static inline void atomic_set(Atomic *v, int i) > >> +{ > >> + v->counter = i; > >> +} Hi, When running on ARM Linux kernels prior to 2.6.32, userspace atomic_set() needs to use "clrex" or "strex" too. See Linux commit 200b812d, "Clear the exclusive monitor when returning from an exception". You can see ARM's atomic_set() used to use "strex", and warns it's important. The kernel patch allows atomic_set() to be simplified, and that includes for userspace, by putting clrex/strex in the exception return path instead. However, someone may run QEMU on a kernel before 2.6.32, which isn't that old. (E.g. my phone is running 2.6.28). Otherwise you can have this situation: Initially: a = 0. Thread atomic_inc(&a, 1) = ldrex, add, [strex interrupted] Interrupted by signal handler atomic_set(&a, 3) = str Signal return Resume thread = strex (succeeds because CPU-local exclusive-flag still set) Result: a = 1, should be impossible when the signal triggered, and information about the signal is lost. A more realistic example would use atomic_compare_exchange(), to atomic-read-and-clear, atomic-read-and-dec-if-not-zero a variable set in a signal handler, however I've used atomic_inc() to illustrate because that's in your patch. Best, -- Jamie
On 19 September 2012 14:32, Jamie Lokier <jamie@shareable.org> wrote: > However, someone may run QEMU on a kernel before 2.6.32, which isn't > that old. (E.g. my phone is running 2.6.28). NB that ARM kernels that old have other amusing bugs, such as not saving the floating point registers when invoking signal handlers. I would be happy for QEMU to just say "your kernel is too old!"... -- PMM
Peter Maydell wrote: > On 19 September 2012 14:32, Jamie Lokier <jamie@shareable.org> wrote: > > However, someone may run QEMU on a kernel before 2.6.32, which isn't > > that old. (E.g. my phone is running 2.6.28). > > NB that ARM kernels that old have other amusing bugs, such > as not saving the floating point registers when invoking > signal handlers. Hi Peter, It's not that old (< 3 years). Granted that's not a nice one, but I'm under the impression it occurs only when the signal handler uses (VFP hardware) floating point. I.e. most programs don't do that, they keep the signal handlers simple (probably including QEMU). (I've read about other platforms that have similar issues using floating point in signal handlers; best avoided.) Anyway, people are running those kernels, someone will try to run QEMU on it unless... > I would be happy for QEMU to just say "your kernel is too old!"... I'd be quite happy with that as well, if you want to put a check in and refuse to run (like Glibc does). Less happy with obscure, rare failures of atomicity that are practically undebuggable, and easily fixed. Cheers, -- Jamie
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h new file mode 100644 index 0000000..f17145d --- /dev/null +++ b/include/qemu/atomic.h @@ -0,0 +1,63 @@ +/* + * Simple wrapper of gcc Atomic-Builtins + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + */ +#ifndef __QEMU_ATOMIC_H +#define __QEMU_ATOMIC_H + +typedef struct Atomic { + int counter; +} Atomic; + +static inline void atomic_set(Atomic *v, int i) +{ + v->counter = i; +} + +static inline int atomic_read(Atomic *v) +{ + return v->counter; +} + +static inline int atomic_return_and_add(int i, Atomic *v) +{ + int ret; + + ret = __sync_fetch_and_add(&v->counter, i); + return ret; +} + +static inline int atomic_return_and_sub(int i, Atomic *v) +{ + int ret; + + ret = __sync_fetch_and_sub(&v->counter, i); + return ret; +} + +/** + * * atomic_inc - increment atomic variable + * * @v: pointer of type Atomic + * * + * * Atomically increments @v by 1. + * */ +static inline void atomic_inc(Atomic *v) +{ + __sync_fetch_and_add(&v->counter, 1); +} + +/** + * * atomic_dec - decrement atomic variable + * * @v: pointer of type Atomic + * * + * * Atomically decrements @v by 1. + * */ +static inline void atomic_dec(Atomic *v) +{ + __sync_fetch_and_sub(&v->counter, 1); +} + +#endif