Patchwork [V3,01/11] atomic: introduce atomic operations

login
register
mail settings
Submitter pingfan liu
Date Sept. 11, 2012, 7:51 a.m.
Message ID <1347349912-15611-2-git-send-email-qemulist@gmail.com>
Download mbox | patch
Permalink /patch/183040/
State New
Headers show

Comments

pingfan liu - Sept. 11, 2012, 7:51 a.m.
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
Avi Kivity - Sept. 11, 2012, 8:04 a.m.
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.
Peter Maydell - Sept. 11, 2012, 8:15 a.m.
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
pingfan liu - Sept. 13, 2012, 6:53 a.m.
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
pingfan liu - Sept. 13, 2012, 6:54 a.m.
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
Avi Kivity - Sept. 13, 2012, 8:14 a.m.
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.
Paolo Bonzini - Sept. 13, 2012, 8:19 a.m.
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
Avi Kivity - Sept. 13, 2012, 8:23 a.m.
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.
Paolo Bonzini - Sept. 13, 2012, 8:29 a.m.
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
pingfan liu - Sept. 13, 2012, 8:45 a.m.
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
Jamie Lokier - Sept. 19, 2012, 1:16 p.m.
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
Jamie Lokier - Sept. 19, 2012, 1:32 p.m.
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
Peter Maydell - Sept. 19, 2012, 2:12 p.m.
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
Jamie Lokier - Sept. 19, 2012, 3:53 p.m.
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

Patch

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