core: Atomic counters

[jnohlgard]

This PR adds atomic_inc and atomic_dec as discussed in #2302

I have created basic implementations for some of the CPUs as an example.

[kaspar030]

this should be a pair of state=disableIRQ()/restoreIRQ(state)

[jnohlgard]

I did not check if the msp430 platform was up to date with regards to IRQ functions, since the above atomic_set_return also used eINT/dINT.

I will change this to match the Cortex-M0 implementation.

[kaspar030]

The 'naive' implementation looks like a good candidate for a generic fallback implementation that could be used whenever an architecture doesn't define an optimized version.

Could you do something like, #ifndef ARCH_HAS_ATOMIC_INT; #define atomic_inc atomic_inc_generic; #endif?

[jnohlgard]

@kaspar030 I get the feeling that your suggested #ifdef will only make the platform configuration more verbose. Do we really have that many CPU architectures that we need this? Otherwise I suggest to just copy the generic implementation to each CPU family as a starting point, then write optimized versions for each CPU whenever someone feel like improving it.

[kaspar030]

@gebart Well, I prefer one #define ARCH_HAS_ATOMIC_INC in the per-cpu header than a copied generic version in the generic case. Seems more natural: initially, we don't have a specialized version. Without any change, the generic version will be used. Once someone 'overrides' the generic version, he adds the corresponding define.

That way, if any problem shows up, it's also easy to disable any specialized function by commenting out the define...

[jnohlgard]

@kaspar030 I updated the PR according to your suggestions. I also removed copies of the generic atomic_arch_set_return to reduce code duplication further. The generic interface is defined in core/atomic.c and protected by an #if ARCH_HAS_ATOMIC_INC != 0

Missing is an implementation for x86 (x86_atomic.c) because I am not familiar enough with x86 assembly language to implement it.

[jnohlgard]

should I remove the API CHANGE label? This PR only adds functions to the core API.

[haukepetersen]

I just stumbled upon a use case, where you need to increment/decrement a value atomically by a given number (e.g. inc by 3). What is your opinion for including this into the proposed API, say something like

int atomic_arch_inc(int *val, int howmuch);

?

[jnohlgard]

@haukepetersen What is your use case?

Maybe we should introduce @Kijewski 's suggestion of Compare And Swap?

Then you would write:

atomic_cas(&val, val, val+3);

My first use case is: Atomic counter as a counting semaphore to introduce inhibiting of low power modes when modules are using hardware devices which need the CPU to stay alive. i.e. a reference count on power modes. In this case we only need up/down counting in steps of 1.

[haukepetersen]

My use case is a simple usage counter, that keeps track of the number of threads that are being passed a (read-)pointer to a data structure. If a thread is done reading from it, it decrements the counter again (atomically, of course). If I send this pointer to 4 different threads, I have to increment it (atomically) 4 times before sending. This would be best looking with such function...

I like @Kijewski 's suggestion, works very well for me. Does it even make sense to just have this one function, as you can do everything we need with it?

[jnohlgard]

@haukepetersen , @Kijewski , it does make sense to implement atomic_cas.
Any objections to rewriting this code as @Kijewski 's suggestion in #2302 (comment) ?

I would be leaving atomic_cas as a public function which can be used for special cases of atomic variables, such as @haukepetersen 's use case, but the simplest way to use the API for everyday use would still be to call either atomic_inc/atomic_dec or atomic_set/atomic_reset.

[haukepetersen]

+1 for implementing atomic_cas.

But I would really think of slimming the interface down to this one function, for the sake of resource efficiency. This is a kernel function that is normally only used by people who know what they are doing, it's not an everyday interface as e.g. threading or mutexes...

[kaspar030]

@haukepetersen Well, if you need more than one line to do atomic_inc, something's wrong with the API.
And you need that if there's only atomic_cas, right?

When we're touching this, we should think about introducing special types for atomic values.

[jnohlgard]

@haukepetersen I liked the static inline version of the counters that @Kijewski wrote. Makes the code using it clean and simple to understand, without knowing the details of atomic_cas and why it has to be inside a loop

[haukepetersen]

ok, I guess you convinced me...

[jnohlgard]

@kaspar030 how do you propose these atomic types would be used?
If I wrote this in C++ I would overload a bunch of operators on them to keep the code clear and readable, but I guess in C we'll have to do something similar to byteorder.h, or am I missing something?

[jnohlgard]

simply doing the following will not warn if passed a regular int variable:

typedef int atomic_int_t;

How can I enforce strict type checking in C? Should I change atomic_int_t into a union instead?

[jnohlgard]

Rebased and updated with a generic atomic_cas function for all atomic operations as per @Kijewski 's suggestion.

TODO (in a separate PR): Update mutex.c etc to use atomic_int_t and atomic_set/clear instead of old atomic_set_return.

[Kijewski]

you need to restore the IRQs.

[jnohlgard]

Thank you, updated the PR accordingly.

[Kijewski]

%s,#endif // _ARM_CPU_H,#endif /* ARM_CPU_H_ */,

[jnohlgard]

done.

[Kijewski]

But for my last comment the PR should be ready to merge.

[jnohlgard]

@kaspar030 ACK still holds?

OK to squash?

[kaspar030]

Yes!

[jnohlgard]

Squashed and rebased, waiting for Travis.
@Kijewski Can I take your comment as an ACK?

[OlegHahm]

Travis fell asleep for arm7 build tests. Kicked it.

[OlegHahm]

ACK and go.