[parisc-linux] atomic_t

[parisc-linux] atomic_t

[Matthew Wilcox]

Just thinking about atomic_set taking the spinlock to assure
exclusivity...  I thought we could eliminate it.

Here's the current code:

static __inline__ void __atomic_set(atomic_t *v, int i) 
{
        unsigned long flags;
        SPIN_LOCK_IRQSAVE(ATOMIC_HASH(v), flags);

        v->counter = i;

        SPIN_UNLOCK_IRQRESTORE(ATOMIC_HASH(v), flags);
}

static __inline__ int __atomic_add_return(int i, atomic_t *v)
{
        int ret;
        unsigned long flags;
        SPIN_LOCK_IRQSAVE(ATOMIC_HASH(v), flags);

        ret = (v->counter += i);

        SPIN_UNLOCK_IRQRESTORE(ATOMIC_HASH(v), flags);
        return ret;
}

The legitimate effect of having two processors do this at the same time
are:

atomic_t v = 3

case 1:

atomic_add_return(1, &v)
atomic_set(&v, 1)
result: v = 1, a_a_r returns 4.

or case 2:

atomic_set(&v, 1)
atomic_add_return(1, &v)
result: v = 2, a_a_r returns 2.

The spinlocks guarantee this behaviour, but let's look at what happens
if we remove the spinlock from atomic_set.  Now atomic_set is just an
assignment, and can occur at any point during atomic_add_ret.  We'll have
to drop to psuedo-assembler for this.

atomic_add_ret becomes:

a) lock
b) load v into register r
c) add 1 to r
d) store r to v
e) unlock
f) return r

atomic_set (store 1 to v) can appear to occur at any point -- the CPU
ref manual guarantees it won't cause any problems.

Before step b leads to case 2.  after step d leads to case 1.  between
steps b and d, it's as if the atomic_set _never_happened_.  It results
in v=4, a_a_r returns 4.

The question is whether any code relies on this behaviour.  It probably
does in some of the messier bits of networking :-(

-- 
Revolutions do not require corporate support.

Re: [parisc-linux] atomic_t

[Grant Grundler]

Matthew Wilcox wrote:
> Just thinking about atomic_set taking the spinlock to assure
> exclusivity...  I thought we could eliminate it.

You are only looking at interactions between atomic_set
and atomic_add_return. Aren't there potentialially other
types of interactions with other atomic operations?
But I don't know. Alan Cox?

...
> Before step b leads to case 2.  after step d leads to case 1.  between
> steps b and d, it's as if the atomic_set _never_happened_.  It results
> in v=4, a_a_r returns 4.

I agree with the analysis.

But someone thought it was time to reset the counter. And if
it "never happened", then whoever is looking for 'v == 1' will
never see it.

> The question is whether any code relies on this behaviour.  It probably
> does in some of the messier bits of networking :-(

Even if that code doesn't exist today, it might exist in the future.
I don't want to bet on corner cases with the semantics.
Those types of bugs are hard to reproduce and hard to debug.


thanks,
grant

Re: [parisc-linux] atomic_t

[Matthew Wilcox]

On Sun, Jan 13, 2002 at 09:09:09PM -0700, Grant Grundler wrote:
> You are only looking at interactions between atomic_set
> and atomic_add_return. Aren't there potentialially other
> types of interactions with other atomic operations?
> But I don't know. Alan Cox?

Not on PA... look at <asm/atomic.h>.  There's only 3 primitives, all
other atomic ops are defined in terms of those.

> > Before step b leads to case 2.  after step d leads to case 1.  between
> > steps b and d, it's as if the atomic_set _never_happened_.  It results
> > in v=4, a_a_r returns 4.
> 
> I agree with the analysis.
> 
> But someone thought it was time to reset the counter. And if
> it "never happened", then whoever is looking for 'v == 1' will
> never see it.

It's a race though; they can't guarantee to see it anyway.

> Even if that code doesn't exist today, it might exist in the future.
> I don't want to bet on corner cases with the semantics.
> Those types of bugs are hard to reproduce and hard to debug.

yeah, wasn't planning on committing.

-- 
Revolutions do not require corporate support.

Re: [parisc-linux] atomic_t

[John Marvin]

> >
> > But someone thought it was time to reset the counter. And if
> > it "never happened", then whoever is looking for 'v == 1' will
> > never see it.
>
> It's a race though; they can't guarantee to see it anyway.
>

Yes, it is a race in that you can't be guaranteed that you will ever see
the value equal to 1. But you should be guaranteed that it WAS reset.

For example:

    1) Initialize counter to 1.
    2) Increment counter by 1 each time event "X" occurs using
       atomic_add_return.
    3) Reset counter to 1 each time event "Y" occurs using atomic_set.
    4) Signal an error if the counter ever exceeds some threshold
       value, i.e. it is an error if too many "X" events occur without
       and intervening "Y" event.

In the above scenario you could not be guaranteed that you would ever see
the counter at one (or any particular value), but you could falsely
trigger the error condition if the resets were lost (by removing the
spinlocks from atomic_set).

John

Re: [parisc-linux] atomic_t

[Grant Grundler]

Matthew Wilcox wrote:
> Not on PA... look at <asm/atomic.h>.  There's only 3 primitives, all
> other atomic ops are defined in terms of those.

Ah ok - you are right. atomic_read() doesn't use the lock and
that only leaves the other two.

> > But someone thought it was time to reset the counter. And if
> > it "never happened", then whoever is looking for 'v == 1' will
> > never see it.
> 
> It's a race though; they can't guarantee to see it anyway.

hmmm...Most people write code with the intent of seeing *some*
event (eg v < 0).  Ie, if it didn't matter, then we don't need
the code.  I'd like to believe folks are usually more careful
than that.  I need to think about this some more though.

grant

Re: [parisc-linux] atomic_t

[Alan Cox]

> > exclusivity...  I thought we could eliminate it.
> 
> You are only looking at interactions between atomic_set
> and atomic_add_return. Aren't there potentialially other
> types of interactions with other atomic operations?
> But I don't know. Alan Cox?

We had a discussion about some of this on #kernel. Its not clear. The
x86 port "knows" that writes are going to be atomic. It means that

	CPU1			CPU2
value=4
	atomic_set=0		atomic_add +1

will result in an addition to the set value, or an addition then the
entire result discarded for the set value. It won't (except in obscure
hardware bug cases 8)) result in the set being lost during the add

ie you might see 0 after those operations, or you might see 1, but you
won't see 5 after both have completed.