only irq-safe atomic ops

The following patch implements i386 versions of atomic_inc and atomic_dec that are LOCK-less but provide IRQ-atomicity and act as a memory-barrier.

An applicable use would be data that needs to be IRQ-safe but not SMP-safe (or, more likely, is already SMP-safe for some other reason).

Additionally, these variants could prevent having to use preempt_disable/enable or "full" atomic ops around per-CPU data with a preemptible kernel.

Thanks, Robert.

Some background here - for the delayed allocation code which I'm cooking up I need to globally count the number of dirty pages in the machine. Currently that's done with atomic_inc(&nr_dirty_pages) in SetPageDirty().

But this counter (which is used for when-to-start-writeback decisions)
is unavoidably approximate. It would be nice to make it a per-CPU
array. So on the rare occasions when the dirty-page count is needed, I can just whizz across the per-cpu counters adding them all up.

But how to increment or decrement a per-cpu counter? The options are:

- per_cpu_integer++;

This is *probably* OK on all architectures. But there are no guarantees that the compiler won't play games, and that this operation is preempt-safe.

- preempt_disable(); per_cpu_counter++; preempt_enable();

A bit heavyweight for add-one-to-i.

- atomic_inc

A buslocked operation where it is not needed - we only need a preempt-locked operation here. But it's per-cpu data, and the buslocked rmw won't be too costly.

I can't believe how piddling this issue is :)

But if there's a general need for such a micro-optimisation then we need to:

1: Create <linux/atomic.h> (for heavens sake!)

2: In <linux/atomic.h>,

   #ifndef ARCH_HAS_ATOMIC_INQ_THINGIES
   #define atomic_inc_irq atomic_inc
   ...
   #endif

But for now, I suggest we not bother. I'll just use atomic_inc().

Quoted 3 lines · [email protected] wrote:

Is this part of some scheme to make the GPL support model actually pay?

No, no, no. It's all the uncommented code which brings in the dollars.

Quoted 2 lines

        c = smp_get_cpuid(); // disables preemption

Quoted 6 lines

        ...
        f(); // oops, me forgotee, this function also references cpuid
        ..
        x = ++local_cache[c]; // live dangerously
        smp_put_cpuid(); // G_d knows what that does now.

preempt_disable() nests. It's not a problem.

Quoted 1 line · Robert Love

For the situation Victor described:

	const int cpu = smp_get_cpuid();

	per_cpu_array_foo[cpu] = per_cpu_array_bar[cpu] +
					per_cpu_array_zot[cpu];

	smp_put_cpuid();

It's a nice interface - it says "pin down and return the current CPU ID".

Go for it! Only 900 odd calls to smp_processor_id in the kernel source last time I checked.

...

Question: if (from below) you are going to use atomic operations, why make it per-CPU at all? Just have one counter and atomic_inc and atomic_read it. You won't need a spin lock.

This would be atomic and thus preempt-safe on any sane arch I know, as long as we are dealing with a normal type int. Admittedly, however, we can't be sure what the compiler would do.

Thinking about it, you are probably going to be doing this:

        ++counter[smp_processor_id()];

and that is not preempt-safe since the whole operation certainly is not atomic. The current CPU could change between calculating it and referencing the array.

But, that wouldn't matter as long as you only
cared about the sum of the counters.

...

Quoted 6 lines

2: In <linux/atomic.h>,

   #ifndef ARCH_HAS_ATOMIC_INQ_THINGIES
   #define atomic_inc_irq atomic_inc
   ...
   #endif

I can think up a few more uses of the irq/memory-safe atomic ops, so I bet this isn't that bad of an idea. But no point doing it without a corresponding use.

Quoted 7 lines · Robert Love

Thinking about it, you are probably going to be doing this:

        ++counter[smp_processor_id()];

and that is not preempt-safe since the whole operation certainly is not atomic. The current CPU could change between calculating it and referencing the array.

yup. It'd probably work - the compiler should calculate the address and do a non-buslocked but IRQ-atomic increment on it. But no way can we rely on that happening.

Quoted 2 lines

But, that wouldn't matter as long as you only
cared about the sum of the counters.

If the compiler produced code equivalent to

	counter[smp_processor_id()] = counter[smp_processor_id()] + 1;

then the counter would get trashed - a context switch could cause CPUB to write CPUA's counter (+1) onto CPUB's counter. It's quite possibly illegal for the compiler to evaluate the array subscript twice in this manner. Dunno.

If the compiler produced code equivalent to:

	const int cpu = smp_processor_id();
	counter[cpu] = counter[cpu] + 1;

(which is much more likely) then a context switch would result in CPUB writing CPUA's updated counter onto CPUA's counter. Which will work a lot more often, until CPUA happens to be updating its counter at the same time.

So without preemption in the kernel
maybe 4 instructions: calculate cpuid, inc; all local no cache ping code is easy to read and understand.

with preemption in the kernel
a design problem. a slippery synchronization issue that involves the characteristic preemption error - code that works most of the time.

Quoted 4 lines · On Sat, 2002-02-23 at 06:38, [email protected] wrote:

So without preemption in the kernel
maybe 4 instructions: calculate cpuid, inc; all local no cache ping code is easy to read and understand.

Quoted 4 lines

with preemption in the kernel
a design problem. a slippery synchronization issue that involves the characteristic preemption error - code that works most of the time.

Or not. The topic of this thread was a micro-optimization. If we treat the variable as anything normal requiring synchronization under SMP, any of the standard solutions (atomic ops, etc.) work. If we want to get

fancy, we can disable preemption, use my atomic_irq ops, or just not care.

Quoted 6 lines · Robert Love

...

Question: if (from below) you are going to use atomic operations, why make it per-CPU at all? Just have one counter and atomic_inc and atomic_read it. You won't need a spin lock.

Oh that works fine. But then it's a global counter, so each time
a CPU marks a page dirty, the counter needs to be pulled out of
another CPU's cache. Which is not a thing I *need* to do.

As I said, it's a micro-issue. But it's a requirement which may pop up elsewhere.

Quoted 13 lines · Andrew Morton

Quoted 6 lines · Robert Love

...

Question: if (from below) you are going to use atomic operations, why make it per-CPU at all? Just have one counter and atomic_inc and atomic_read it. You won't need a spin lock.

Oh that works fine. But then it's a global counter, so each time
a CPU marks a page dirty, the counter needs to be pulled out of
another CPU's cache. Which is not a thing I *need* to do.

Quoted 4 lines

As I said, it's a micro-issue. But it's a requirement which may pop up elsewhere.

I can tell you that Irix has just such a global counter for the amount of delayed allocate pages - and it gets to be a major point of cache contention once you get to larger cpu counts. So avoiding that from the start would be good.

Right. Without preemption it is safe to do

        c = smp_get_cpuid();
       ...
        x = ++local_cache[c]
       ..

       y = ++different_local_cache[c];
      ..

Hi,

Quoted 6 lines · [email protected] wrote:

Right. Without preemption it is safe to do

        c = smp_get_cpuid();
       ...
        x = ++local_cache[c]
       ..

Quoted 2 lines

       y = ++different_local_cache[c];
      ..

Just add:

        smp_put_cpuid();

Is that so much worse?

ooh. me likee.

#define smp_get_cpuid() ({ preempt_disable(); smp_processor_id(); }) #define smp_put_cpuid() preempt_enable()

Does rml likee?

Quoted 6 lines · Andrew Morton

ooh. me likee.

#define smp_get_cpuid() ({ preempt_disable(); smp_processor_id(); }) #define smp_put_cpuid() preempt_enable()

Does rml likee?

Yah, that works.

Quoted 7 lines · Robert Love

Quoted 5 lines · Andrew Morton

ooh. me likee.

#define smp_get_cpuid() ({ preempt_disable(); smp_processor_id(); }) #define smp_put_cpuid() preempt_enable()

Quoted 3 lines

Quoted 1 line

Does rml likee?

Yah, that works.

OK, I still likee, but I was just thinking, if we are going to add have to add something why not consider the irq-safe atomic ops? It is certainly the most optimal.

Quoted 14 lines · Roman Zippel

Hi,

Quoted 8 lines · [email protected] wrote:

Right. Without preemption it is safe to do

        c = smp_get_cpuid();
       ...
        x = ++local_cache[c]
       ..

       y = ++different_local_cache[c];
      ..

Quoted 5 lines

Just add:

        smp_put_cpuid();

Is that so much worse?

ooh. me likee.

Is this part of some scheme to make the GPL support model actually pay?

        c = smp_get_cpuid(); // disables preemption

        ...
        f(); // oops, me forgotee, this function also references cpuid
        ..
        x = ++local_cache[c]; // live dangerously
        smp_put_cpuid(); // G_d knows what that does now.

This would be atomic and thus preempt-safe on any sane arch I know, as long as we are dealing with a normal type int. Admittedly, however, we can't be sure what the compiler would do.

With preemption this turns into a problem that is easier to solve with a lock or by not having per-cpu caches in the first place --

Quoted 2 lines · On Sat, Feb 23, 2002 at 12:06:48PM -0700, [email protected] wrote:

With preemption this turns into a problem that is easier to solve with a lock or by not having per-cpu caches in the first place --

whilst you're correct (pre-emption has made everything harder), there's not much point moaning about it, since it's in linus' kernel now. So why bother ?