Introduce checks for preemptable code for this_cpu_read/write()

The __const_udelay() code originally used raw_smp_processor_id() in its calculations for a delaying. Probably because if it were to migrate, it would take much longer to do so than the requested delay.

Switch from this_cpu_read() to __this_cpu_read() to document that the read is racy and we do not care.

Quoted 7 lines · Steven Rostedt

The __const_udelay() code originally used raw_smp_processor_id() in its calculations for a delaying. Probably because if it were to migrate, it would take much longer to do so than the requested delay.

Switch from this_cpu_read() to __this_cpu_read() to document that the read is racy and we do not care.

Is preemption disabled by all callers to __const_udelay?

From: Steven Rostedt <[email protected]>

The code in mod_state() is already made to handle the raciness of this_cpu_read(). Have the code use __this_cpu_read() instead so the debug code does not trigger warnings about it.

Quoted 5 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

The code in mod_state() is already made to handle the raciness of this_cpu_read(). Have the code use __this_cpu_read() instead so the debug code does not trigger warnings about it.

Why would there be a warning triggered? this_cpu_read should take care of disabling preemption for the read if needed. In fact the fallback case does do exactly that.

I think it would make more sence if __this_cpu_read() could be made to trigger a warning if used in context where preemption could be off.

Quoted 8 lines · Christoph Lameter

Quoted 5 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

The code in mod_state() is already made to handle the raciness of this_cpu_read(). Have the code use __this_cpu_read() instead so the debug code does not trigger warnings about it.

Quoted 3 lines

Why would there be a warning triggered? this_cpu_read should take care of disabling preemption for the read if needed. In fact the fallback case does do exactly that.

What does disabling preemption for just reading the cpu variable help anywhere? Once you read the variable, and enable preemption, you can migrate. Then that info you have could be useless.

The point is, most places use this_cpu_read(). Adding a "__" or "raw_" prefix usually means that less checks are done. Think of the copy_from_user(). You have __copy_from_user() that does less checks.

Most of the per_cpu() and get_cpu_var() code was blindly replaced with the this_cpu_*() variants. The original code had preemption disabled checks. Now they don't. Which means if you use the this_cpu_read() and then make some decision on it, that read may be useless and buggy.

We have smp_processor_id() that does the check (and the old per_cpu() and get_cpu_var()). Now we have this_cpu_read() which replaced most of the per_cpu() code in the kernel, making them vulnerable to bugs when preemption is enabled.

The effects on just reading are not that severe. We may calculate the address of the per cpu variable on one cpu and then be migrated to another processor. Then there is an access to the per cpu area of another processor which in many cases is harmless and will just cause the cacheline to go from exclusive mode into shared mode on the other cpu. The next write by the other cpu then brings it back to exclusive mode evicting the cacheline from the cpu we migrated to.

The same conventions are also used for RMV instructions like this_cpu_inc. In that case the situation is much more sever. Now we are locklessly incrementing a counter in the per cpu area of another cpu. That is a race condition that can corrupt the counter data.

Quoted 4 lines

The point is, most places use this_cpu_read(). Adding a "__" or "raw_" prefix usually means that less checks are done. Think of the copy_from_user(). You have __copy_from_user() that does less checks.

Why do you need any checks there? this_cpu_xxx operations should take care of preemption.

Quoted 10 lines

Most of the per_cpu() and get_cpu_var() code was blindly replaced with the this_cpu_*() variants. The original code had preemption disabled checks. Now they don't. Which means if you use the this_cpu_read() and then make some decision on it, that read may be useless and buggy.

We have smp_processor_id() that does the check (and the old per_cpu() and get_cpu_var()). Now we have this_cpu_read() which replaced most of the per_cpu() code in the kernel, making them vulnerable to bugs when preemption is enabled.

this_cpu_xx can only replace a get_cpu_var/per_cpu section where a single word is read or updated. If multiple per cpu variables are accessed and modified in one go then it is better to keep disabling preemption for the whole section and use the __this_cpu_xx operations.

If there are cases where this was not done correctly then lets fix those.

Quoted 7 lines · Christoph Lameter

The effects on just reading are not that severe. We may calculate the address of the per cpu variable on one cpu and then be migrated to another processor. Then there is an access to the per cpu area of another processor which in many cases is harmless and will just cause the cacheline to go from exclusive mode into shared mode on the other cpu. The next write by the other cpu then brings it back to exclusive mode evicting the cacheline from the cpu we migrated to.

Then what protects another process from touching that same CPU variable? Looks very error prone to me.

Quoted 4 lines

The same conventions are also used for RMV instructions like this_cpu_inc. In that case the situation is much more sever. Now we are locklessly incrementing a counter in the per cpu area of another cpu. That is a race condition that can corrupt the counter data.

If you read a per cpu variable with simple this_cpu_read() and then make a decision based on that variable, it may be useless in the process if you migrate.

Quoted 6 lines

Quoted 3 lines

The point is, most places use this_cpu_read(). Adding a "__" or "raw_" prefix usually means that less checks are done. Think of the copy_from_user(). You have __copy_from_user() that does less checks.

Why do you need any checks there? this_cpu_xxx operations should take care of preemption.

NO IT DOES NOT!!!! Look at the damn memcg bug and figure it out!

Quoted 4 lines

this_cpu_xx can only replace a get_cpu_var/per_cpu section where a single word is read or updated. If multiple per cpu variables are accessed and modified in one go then it is better to keep disabling preemption for the whole section and use the __this_cpu_xx operations.

And that is what __get_cpu_var() is for.

Quoted 1 line

If there are cases where this was not done correctly then lets fix those.

Lets not let this happen in the first place! There's absolutely no advantage of not disabling preemption for a period of time while per cpu variables are being used. If we annotate this properly, then the -rt kernel could handle this in other ways too, to not keep the latency of preemption off too long.

There are significant advantages for counters and things designed to operate in an environment where the OS can migrate a task at will. Vvmstat and the slub fastpaths exploit these things now and it will be possible with these operations to increase the performance of more subsystems that way. The page allocator comes to mind.

IT DOES!!!! The memcg bug is caused by someone using this_cpu_read and expecting the cpu not to change. this_cpu_xx ops stand on their own and do not guarantee access to the same per cpu area.

Quoted 5 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

The code in mod_state() is already made to handle the raciness of this_cpu_read(). Have the code use __this_cpu_read() instead so the debug code does not trigger warnings about it.

Why would there be a warning triggered? this_cpu_read should take care of disabling preemption for the read if needed. In fact the fallback case does do exactly that.

I think it would make more sence if __this_cpu_read() could be made to trigger a warning if used in context where preemption could be off.

The best thing is to remove the this_cpu_* hackery alltogether and just keep the __this_cpu_* versions (along with proper debugging) and git rid of the silly underscores.

Quoted 3 lines

I think it would make more sence if __this_cpu_read() could be made to trigger a warning if used in context where preemption could be off.

It should have had such a warning in the first place and the warning needs to yell about preemptible (i.e. unprotected) context and not the other way round.

But instead of just slapping smp_processor_id() checks into those functions we should add a more sensible debug interface like:

debug_check_percpu_access()

and the per cpu sections which require protection over a series (1 .. N) of this_cpu_* operations want to have

this_cpu_start()
this_cpu_end()

or similar annotations around them.

This allows us to do proper analysis of this_cpu usage and makes the code understandable.

Quoted 3 lines

Why would there be a warning triggered? this_cpu_read should take care of disabling preemption for the read if needed. In fact the fallback case does do exactly that.

And what exactly is the purpose of having a preempt_disable()/enable() pair in this_cpu_read()?

functions we should add a more sensible debug interface like:

debug_check_percpu_access()

and the per cpu sections which require protection over a series (1 .. N) of this_cpu_* operations want to have

this_cpu_start()
this_cpu_end()

or similar annotations around them.

This allows us to do proper analysis of this_cpu usage and makes the code understandable.

I see that __this_cpu_xx operations may not work as intended in preemptable contexts and there we could have more changes.

this_cpu ops are single instructions that do not guarantee any consistency with other this_cpu_ops. If you want consistency (same per cpu area data) then preemption needs to be disabled.

Quoted 2 lines · Christoph Lameter

I see that __this_cpu_xx operations may not work as intended in preemptable contexts and there we could have more changes.

Then why do you use it in slub.c?

in mm/slab.c slab_alloc():

redo:

/*
* Must read kmem_cache cpu data via this cpu ptr. Preemption is
* enabled. We may switch back and forth between cpus while
* reading from one cpu area. That does not matter as long
* as we end up on the original cpu again when doing the cmpxchg. */
c = __this_cpu_ptr(s->cpu_slab);

The __this_cpu_*() is in preempt enabled location. In fact, the three this_cpu_write()'s in acquire_slab() is done within a spinlock and thus with preemption disabled.

The point is, people get it wrong all the time. In fact, we should really require that ALL USES of this_cpu_*() must be with preemption disabled. Regardless. Because anytime you touch a per cpu variable,

Monkeying around with per cpu data is tricky. If you start doing it in preempt enabled code, you are most certainly about to get it wrong. Why have this super optimization. A preempt_disable() is a single operation that touches cache hot data.

Quoted 19 lines · Steven Rostedt

Quoted 2 lines · Christoph Lameter

I see that __this_cpu_xx operations may not work as intended in preemptable contexts and there we could have more changes.

Then why do you use it in slub.c?

in mm/slab.c slab_alloc():

redo:

/*
* Must read kmem_cache cpu data via this cpu ptr. Preemption is
* enabled. We may switch back and forth between cpus while
* reading from one cpu area. That does not matter as long
* as we end up on the original cpu again when doing the cmpxchg. */
c = __this_cpu_ptr(s->cpu_slab);

Quoted 3 lines

The __this_cpu_*() is in preempt enabled location. In fact, the three this_cpu_write()'s in acquire_slab() is done within a spinlock and thus with preemption disabled.

So the this_cpu_writes could become __this_cpu_writes

The __this_cpu_ptr operation in slab_alloc is special. We explicitly do address calculation and do not care from which per cpu area we fetch multiple per cpu data items because we can verify that we are on the same per cpu area during the this_cpu_cmpxchg_double operation.

Quoted 4 lines

The point is, people get it wrong all the time. In fact, we should really require that ALL USES of this_cpu_*() must be with preemption disabled. Regardless. Because anytime you touch a per cpu variable,

NO!! This defeats the whole purpose of this_cpu_ops and make the whole scheme utterly useless.

Quoted 5 lines

Monkeying around with per cpu data is tricky. If you start doing it in preempt enabled code, you are most certainly about to get it wrong. Why have this super optimization. A preempt_disable() is a single operation that touches cache hot data.

There are trivial cases like counter increments that are not a problem at all. Most use cases are those. More complex ones can be developed to avoid various overhead in performance critical sections of the kernel.

Quoted 2 lines

NO!! This defeats the whole purpose of this_cpu_ops and make the whole scheme utterly useless.

The thing is, the whole purpose was broken to begin with. Defeating a broken design is a good thing!

Quoted 4 lines

There are trivial cases like counter increments that are not a problem at all. Most use cases are those. More complex ones can be developed to avoid various overhead in performance critical sections of the kernel.

And adding a preempt_disable; this_cpu_inc(); preempt_enable; is not a bad thing either.

What benchmarks do you have that shows this helped in anything????

Quoted 2 lines · Thomas Gleixner

This allows us to do proper analysis of this_cpu usage and makes the code understandable.

Not sure what you want to check in this_cpu_xx operations. Why could there be issues with this_cpu_xx operations?

I see that __this_cpu_xx operations may not work as intended in preemptable contexts and there we could have more changes.

this_cpu ops are single instructions that do not guarantee any consistency with other this_cpu_ops. If you want consistency (same per cpu area data) then preemption needs to be disabled.

I want to get rid of them alltogether. They are crap by design and prone to be used wrong without a sensible way to detect that.

Right, and that's the main problem. We have no fricking way to debug this, so they should have never been there in the first place. And you simply CANNOT prevent people from getting this wrong w/o proper debugging and annotation. Even YOU and Tejun made bogus conversion w/o noticing, but you expect that others get it right?

So stop defending that trainwreck and help out with fixing the mess you created in a proper and debugable way!

Quoted 2 lines · Thomas Gleixner

I want to get rid of them alltogether. They are crap by design and prone to be used wrong without a sensible way to detect that.

Counter increments have been done this way for a very long time. This is a pretty well established way of doing things in the kernel that was expanded and made better by avoid preempt disable/enable around these per cpu pointer increments.

Quoted 5 lines

Right, and that's the main problem. We have no fricking way to debug this, so they should have never been there in the first place. And you simply CANNOT prevent people from getting this wrong w/o proper debugging and annotation. Even YOU and Tejun made bogus conversion w/o noticing, but you expect that others get it right?

Bogus conversions?

Quoted 2 lines

So stop defending that trainwreck and help out with fixing the mess you created in a proper and debugable way!

You want debugging to ensure that this_cpu ops always access the intended per cpu area? But the point of these operations is to be used in kernel locations where the use of any per cpu area is satisfactory.

this_cpu_*() is patently wrong. It should be: random_cpu_*() or any_cpu_*(). This way you could have avoided confusion in the first place and made it entirely clear what the interfaces are about.

From: Steven Rostedt <[email protected]>

The code in memcg_check_events() calls this_cpu_read() on different variables without disabling preemption, and can cause the calculations to be done from two different CPU variables.

Disable preemption throughout the check to keep apples and oranges from becoming a mixed drink.

Quoted 8 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

The code in memcg_check_events() calls this_cpu_read() on different variables without disabling preemption, and can cause the calculations to be done from two different CPU variables.

Disable preemption throughout the check to keep apples and oranges from becoming a mixed drink.

Makes sense, thanks!

Since the atomic versions are no longer required with preemption disabled explicitely, could you also make the this_cpu ops in __memcg_event_check and __mem_cgroup_target_update non-atomic in the same go?

Quoted 10 lines · Johannes Weiner

Quoted 8 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

The code in memcg_check_events() calls this_cpu_read() on different variables without disabling preemption, and can cause the calculations to be done from two different CPU variables.

Disable preemption throughout the check to keep apples and oranges from becoming a mixed drink.

Quoted 6 lines

Makes sense, thanks!

Since the atomic versions are no longer required with preemption disabled explicitely, could you also make the this_cpu ops in __memcg_event_check and __mem_cgroup_target_update non-atomic in the same go?

Sorry, shouldn't be on you, can you fold this in?

Signed-off-by Johannes Weiner <[email protected]>

---

mm/memcontrol.c +4 -4

index b76011a..9d4ba65 100644
@@ -683,8 +683,8 @@ static bool __memcg_event_check(struct mem_cgroup *memcg, int target)
 {
 	unsigned long val, next;
 
-	val = this_cpu_read(memcg->stat->events[MEM_CGROUP_EVENTS_COUNT]);
-	next = this_cpu_read(memcg->stat->targets[target]);
+	val = __this_cpu_read(memcg->stat->events[MEM_CGROUP_EVENTS_COUNT]);
+	next = __this_cpu_read(memcg->stat->targets[target]);
 	/* from time_after() in jiffies.h */
 	return ((long)next - (long)val < 0);
 }

@@ -693,7 +693,7 @@ static void __mem_cgroup_target_update(struct mem_cgroup *memcg, int target)
 {
 	unsigned long val, next;
 
-	val = this_cpu_read(memcg->stat->events[MEM_CGROUP_EVENTS_COUNT]);
+	val = __this_cpu_read(memcg->stat->events[MEM_CGROUP_EVENTS_COUNT]);
 
 	switch (target) {
 	case MEM_CGROUP_TARGET_THRESH:

@@ -709,7 +709,7 @@ static void __mem_cgroup_target_update(struct mem_cgroup *memcg, int target)
 		return;
 	}
 
-	this_cpu_write(memcg->stat->targets[target], next);
+	__this_cpu_write(memcg->stat->targets[target], next);
 }
 
 /*

plain text document attachment (0003-memcg-Disable-preemption-in-memcg_check_events.patch)
From: Steven Rostedt <[email protected]>

The code in memcg_check_events() calls this_cpu_read() on different variables without disabling preemption, and can cause the calculations to be done from two different CPU variables.

Disable preemption throughout the check to keep apples and oranges from becoming a mixed drink.

Signed-off-by Johannes Weiner <[email protected]>

Signed-off-by Greg Thelen <[email protected]>

Acked-by KAMEZAWA Hiroyuki <[email protected]>

Acked-by Balbir Singh <[email protected]>

Cc Daisuke Nishimura <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 mm/memcontrol.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

mm/memcontrol.c +2 -0

index 3508777..a164c93 100644
@@ -718,6 +718,7 @@ static void __mem_cgroup_target_update(struct mem_cgroup *mem, int target)
  */
 static void memcg_check_events(struct mem_cgroup *mem, struct page *page)
 {
+	preempt_disable();
 	/* threshold event is triggered in finer grain than soft limit */
 	if (unlikely(__memcg_event_check(mem, MEM_CGROUP_TARGET_THRESH))) {
 		mem_cgroup_threshold(mem);

@@ -737,6 +738,7 @@ static void memcg_check_events(struct mem_cgroup *mem, struct page *page)
 		}
 #endif
 	}
+	preempt_enable();
 }
 
 static struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont)

From: Steven Rostedt <[email protected]>

The wake up code that triggers klogd does not really matter which CPU it enables the wake up on. Every CPU will be doing a printk_tick() and check the current CPU. As long as one of the CPUs triggers the wakeup we are fine. Use __this_cpu_write() instead of this_cpu_write() to show that we do not care.

Quoted 2 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

Quoted 5 lines

The wake up code that triggers klogd does not really matter which CPU it enables the wake up on. Every CPU will be doing a printk_tick() and check the current CPU. As long as one of the CPUs triggers the wakeup we are fine. Use __this_cpu_write() instead of this_cpu_write() to show that we do not care.

printk_needs_cpu() is always called from context where we have disabled interrupts therefore this is safe to do.

Reviewed-by Christoph Lameter <[email protected]>

Note, just because something is always in a location that preemption is disabled, does not mean it should use the __this_cpu*() variants.

Because if things change, it may become a problem later on.

Quoted 2 lines · Steven Rostedt

Note, just because something is always in a location that preemption is disabled, does not mean it should use the __this_cpu*() variants.

Why not? If preemption is disabled then the process cannot be migrated to another processor. And thus doing the address calculations and operations on variables step by step is okay.

Quoted 1 line

Because if things change, it may become a problem later on.

What things may change? Someone calls the function with preemption enabled?

I just found out that the this_cpu_*() functions do not perform the test to see if the usage is in atomic or not. Thus, the blind conversion of the per_cpu(*, smp_processor_id()) and the get_cpu_var() code to this_cpu_*() introduce the regression to detect the hard to find case where a per cpu variable is used in preempt code that migrates and causes bugs.

Quoted 6 lines · Steven Rostedt

I just found out that the this_cpu_*() functions do not perform the test to see if the usage is in atomic or not. Thus, the blind conversion of the per_cpu(*, smp_processor_id()) and the get_cpu_var() code to this_cpu_*() introduce the regression to detect the hard to find case where a per cpu variable is used in preempt code that migrates and causes bugs.

this_cpu_* function can be used either way. There is no point in checking for atomic or not since the this_cpu_* implementations are required to take care of disabling preemption. Those operations are generally safe to use regardless of the context.

It can be replaced by __this_cpu_* only when we know that the context prevents races through disabling preemption.

It is really confusing to know which version to use. I'm confused by the this_cpu_*() compared with __this_cpu_*(). I'm guessing that most places should use __this_cpu*(). But really this_cpu() should be the default, and the places that can have it outside of preemption should have another name. Maybe use the raw_this_cpu() or safe_this_cpu(), as there is an irqsafe_this_cpu(). Maybe make a preemptsafe_cpu_*(). There should only be a very few locations that are OK to have preemption enabled when calling the this_cpu() code. Lets have those have the funny names and not be the default "this_cpu_*()".

All this_cpu*() code, except the funny named ones, should make sure preemption is disabled, otherwise give a nasty warning. As that is usually a bug if you are using a per cpu variable and can migrate away. The next reference to that value may be incorrect.

Quoted 9 lines · Steven Rostedt

It is really confusing to know which version to use. I'm confused by the this_cpu_*() compared with __this_cpu_*(). I'm guessing that most places should use __this_cpu*(). But really this_cpu() should be the default, and the places that can have it outside of preemption should have another name. Maybe use the raw_this_cpu() or safe_this_cpu(), as there is an irqsafe_this_cpu(). Maybe make a preemptsafe_cpu_*(). There should only be a very few locations that are OK to have preemption enabled when calling the this_cpu() code. Lets have those have the funny names and not be the default "this_cpu_*()".

What's the latency hit on those very few locations if we simply put our collective foot down and not support a preemptable version of this_cpu_*()? "Yes, you *could* preempt here, but for our collective sanity that's not supported"...

Quoted 4 lines

All this_cpu*() code, except the funny named ones, should make sure preemption is disabled, otherwise give a nasty warning. As that is usually a bug if you are using a per cpu variable and can migrate away. The next reference to that value may be incorrect.

You get a much prettier diffstat if you just nuke the funny named ones. ;)

But of course it's early morning and I'm still caffeine-deficient and probably overlooking some crucial use case. ;)

Quoted 4 lines

What's the latency hit on those very few locations if we simply put our collective foot down and not support a preemptable version of this_cpu_*()? "Yes, you *could* preempt here, but for our collective sanity that's not supported"...

Full ack.

Quoted 6 lines · Thomas Gleixner

Quoted 4 lines

What's the latency hit on those very few locations if we simply put our collective foot down and not support a preemptable version of this_cpu_*()? "Yes, you *could* preempt here, but for our collective sanity that's not supported"...

Full ack.

Latency hit could be very significant in various critical kernel paths. Especially network subsystem, vm event counters etc.

Quoted 9 lines · Steven Rostedt

It is really confusing to know which version to use. I'm confused by the this_cpu_*() compared with __this_cpu_*(). I'm guessing that most places should use __this_cpu*(). But really this_cpu() should be the default, and the places that can have it outside of preemption should have another name. Maybe use the raw_this_cpu() or safe_this_cpu(), as there is an irqsafe_this_cpu(). Maybe make a preemptsafe_cpu_*(). There should only be a very few locations that are OK to have preemption enabled when calling the this_cpu() code. Lets have those have the funny names and not be the default "this_cpu_*()".

this_cpu_xx functions are made for those locations that have preemption enabled. If you can use those function (classic case is a per cpu counter increment in the network subsystem) then you can avoid preempt disable/enable or get_cpu/put_cpu.

Quoted 4 lines · Christoph Lameter

this_cpu_xx functions are made for those locations that have preemption enabled. If you can use those function (classic case is a per cpu counter increment in the network subsystem) then you can avoid preempt disable/enable or get_cpu/put_cpu.

If the variables are used for a very short time, then the latencies introduced by a simple:

	var = get_cpu_var(my_var);
	if (var)
		do_something_quick();
	put_cpu_var(my_var);

Otherwise if that do_something_quick(); migrates, it may be doing something it shouldn't be doing!

Quoted 15 lines · Steven Rostedt

Quoted 4 lines · Christoph Lameter

this_cpu_xx functions are made for those locations that have preemption enabled. If you can use those function (classic case is a per cpu counter increment in the network subsystem) then you can avoid preempt disable/enable or get_cpu/put_cpu.

If the variables are used for a very short time, then the latencies introduced by a simple:

	var = get_cpu_var(my_var);
	if (var)
		do_something_quick();
	put_cpu_var(my_var);

Quoted 2 lines

Otherwise if that do_something_quick(); migrates, it may be doing something it shouldn't be doing!

That is obviously a use case in which this_cpu_xx ops could not be used since we must stay on the same cpu. I think there is still a lot of confusion on your part. The example here shows it.

I personally believe that we shouldn't even have a modification version of the snapshot code. But perhaps it is fine for inc and dec and cmpxchg. Put "snapshot" in the name to document that this is a hack to make things work, and let all other users in the kernel use a name that is not confusing.

random_cpu_*() // Thomas's idea

Quoted 1 line · Steven Rostedt

random_cpu_*() // Thomas's idea

I like this one best..

But you forgot do deal with the irqsafe_cpu() crap, that's the same brainfart as this_cpu() but more expensive because it frobs IRQ state.

Quoted 4 lines · Peter Zijlstra

Quoted 1 line · Steven Rostedt

random_cpu_*() // Thomas's idea

I like this one best..

I like it too, but not really the most appropriate.

Quoted 1 line

Quoted 2 lines

But you forgot do deal with the irqsafe_cpu() crap, that's the same brainfart as this_cpu() but more expensive because it frobs IRQ state.

But irqsafe_cpu_*() doesn't really have any real meaning to me. That is something when I see it, I go and read the comments about it. It doesn't contain "this_cpu" which is something that seems to explain what it is, even though the obvious is not what it is.

Quoted 9 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 4 lines · Peter Zijlstra

Quoted 1 line · Steven Rostedt

random_cpu_*() // Thomas's idea

I like this one best..

I like it too, but not really the most appropriate.

Quoted 1 line

Quoted 7 lines

Quoted 2 lines

But you forgot do deal with the irqsafe_cpu() crap, that's the same brainfart as this_cpu() but more expensive because it frobs IRQ state.

But irqsafe_cpu_*() doesn't really have any real meaning to me. That is something when I see it, I go and read the comments about it. It doesn't contain "this_cpu" which is something that seems to explain what it is, even though the obvious is not what it is.

Throwing ideas from the IRC discussion into the mix (Paul McKenney and I came up with it at the same time):

preempt_protected_percpu_*()
irq_protected_percpu_*()

Seems to be quite self-explanatory.

Quoted 18 lines · Mathieu Desnoyers

Quoted 9 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 4 lines · Peter Zijlstra

Quoted 1 line · Steven Rostedt

random_cpu_*() // Thomas's idea

I like this one best..

I like it too, but not really the most appropriate.

Quoted 1 line

Quoted 7 lines

Quoted 2 lines

But you forgot do deal with the irqsafe_cpu() crap, that's the same brainfart as this_cpu() but more expensive because it frobs IRQ state.

But irqsafe_cpu_*() doesn't really have any real meaning to me. That is something when I see it, I go and read the comments about it. It doesn't contain "this_cpu" which is something that seems to explain what it is, even though the obvious is not what it is.

Quoted 8 lines

Throwing ideas from the IRC discussion into the mix (Paul McKenney and I came up with it at the same time):

preempt_protected_percpu_*()
irq_protected_percpu_*()

Seems to be quite self-explanatory.

For use where the per_cpu data is protected with preemption disabled? But isn't that the default case? Why make it hard to type for when you should use it in the normal case.

It should be hard to type when it is a hack. As I recommended on IRC, we probably should have it as:

use_this_if_you_really_do_not_care_what_cpu_you_are_on_but_are_anal_about_performance_cpu_*()

1) it is very self descriptive.
2) it would limit the usage as people wont like to have it in their code ;)

Not quite. What I was proposing more precisely:

- this_cpu_*() for the case where the caller needs to disable preemption. This is the default case. This is exactly what you proposed, with WARN_ON debug checks. This could even be "percpu_*()" now that I think of it. There is no real point in the "this_cpu" prefix.

- preempt_protected_percpu_*() and irq_protected_percpu_*() for statistics/slub use. Those primitives disable preemption or irq internally on non-x86 architectures. The caller of these primitives is not required to disable preemption nor irqs.

Quoted 8 lines · Mathieu Desnoyers

Not quite. What I was proposing more precisely:

- this_cpu_*() for the case where the caller needs to disable preemption. This is the default case. This is exactly what you proposed, with WARN_ON debug checks. This could even be "percpu_*()" now that I think of it. There is no real point in the "this_cpu" prefix.

Quoted 4 lines

- preempt_protected_percpu_*() and irq_protected_percpu_*() for statistics/slub use. Those primitives disable preemption or irq internally on non-x86 architectures. The caller of these primitives is not required to disable preemption nor irqs.

This is totally confusing. It suggests to me that the percpu requires preemption protected. You are coupling the implementation of the function too much with the name. The name should describe its use. What does "preempt_protected" mean? To me, it sounds like I should use this in preempt protected mode. Still way too confusing.

any_cpu_*() is still much more understanding. It means that we are manipulating a CPU variable, and we do not care which one.

Looking at the real use cases of this_cpu(), that seems to be exactly the use case for it. That is, we modify the cpu variable, maybe we get migrated, but in the end, we just read all the cpu variables and report the net sum. Thus the design POV is that we do not care what CPU variable we read/write. From an implementation point of view, it just happens to be an optimization that we try to read/write to the current cpu pointer. But in reality it doesn't matter what CPU variable we touch.

Do not confuse implementation and optimizations with design. The big picture design is that we do not care what CPU variable is touched. The name should reflect that.

That is obviously a use case in which this_cpu_xx ops could not be used since we must stay on the same cpu.

Quoted 4 lines · Christoph Lameter

this_cpu_xx functions are made for those locations that have preemption enabled. If you can use those function (classic case is a per cpu counter increment in the network subsystem) then you can avoid preempt disable/enable or get_cpu/put_cpu.

If the variables are used for a very short time, then the latencies introduced by a simple:

	var = get_cpu_var(my_var);
	if (var)
		do_something_quick();
	put_cpu_var(my_var);

Otherwise if that do_something_quick(); migrates, it may be doing something it shouldn't be doing!

Quoted 15 lines · Steven Rostedt

Quoted 4 lines · Christoph Lameter

this_cpu_xx functions are made for those locations that have preemption enabled. If you can use those function (classic case is a per cpu counter increment in the network subsystem) then you can avoid preempt disable/enable or get_cpu/put_cpu.

If the variables are used for a very short time, then the latencies introduced by a simple:

	var = get_cpu_var(my_var);
	if (var)
		do_something_quick();
	put_cpu_var(my_var);

Quoted 2 lines

Otherwise if that do_something_quick(); migrates, it may be doing something it shouldn't be doing!

This has the added advantage of making the calling function take the blame in latency traces, doesn't it?

[ Added Mathieu as I've been discussing this with him on IRC ]

My suggestion is to nuke the __this_cpu*() functions and have them called this_cpu*(). And change the locations that allow for preemption enabled to be called preemptsafe_cpu_*() just like the irqsafe_cpu*() are used.

Thoughts?

Quoted 1 line · * Steven Rostedt ([email protected]) wrote:

[ Added Mathieu as I've been discussing this with him on IRC ]

[...]

Quoted 6 lines

My suggestion is to nuke the __this_cpu*() functions and have them called this_cpu*(). And change the locations that allow for preemption enabled to be called preemptsafe_cpu_*() just like the irqsafe_cpu*() are used.

Thoughts?

I fully agreed with this proposal.

this_cpu_*() should be a sane default, which is cases that require preemption to be disabled. Warning about use without preemption disabled is important, because most users will _assume_ that they stay on the same CPU across multiple calls.

preemptsafe_cpu_*() is an optimisation made for those who know that they don't care about having consistent view of the variables across multiple operations, e.g. statistics, or deal with this explicitly, like SLUB. Typical use require either add_return or cmpxchg for validation.

In the past, we used to have something like raw_smp_processor_id() though, for sites where racy use does not matter. So probably having a raw_this_cpu_*() that behaves like "this_cpu_*()" proposed here, but does not check for preemption disabled would also be useful.

I really mean all other users of this_cpu_*(), including the cmpxchg and friends, still need to have preemption disabled.

Quoted 2 lines · Steven Rostedt

I really mean all other users of this_cpu_*(), including the cmpxchg and friends, still need to have preemption disabled.

This is argument against the basic design of this_cpu_ops. They were designed to avoid having to disable preemption for single operations on per cpu data. I think this shows a basic misunderstanding of what you are dealing with.

Quoted 2 lines · Steven Rostedt

I really mean all other users of this_cpu_*(), including the cmpxchg and friends, still need to have preemption disabled.

This is argument against the basic design of this_cpu_ops. They were designed to avoid having to disable preemption for single operations on per cpu data. I think this shows a basic misunderstanding of what you are dealing with.

BTW, Can you explain to me where the this_cpu_*() ops were designed to be used? The only places where "this_cpu_*()" is used in slub.c and page_alloc.c have irqs disabled on their use. I thought this was for slub and page_alloc?

Is this_cpu() made just for statistics? I see it used in the inode code for that, and some accounting in the namespace.c code.

Note and there's places all over the kernel that uses this_cpu_read() and thinks preemption should be disabled. Just look at arch/x86/mm/tlb.c:

	/* Caller has disabled preemption */
	sender = this_cpu_read(tlb_vector_offset);

Why the comment?

My argument is that this_cpu_* is just confusing. Rename your use case and keep this_cpu_*() as what you want __this_cpu_*() to be.

Quoted 2 lines

My argument is that this_cpu_* is just confusing. Rename your use case and keep this_cpu_*() as what you want __this_cpu_*() to be.

Thought about this a bit last night. I think the main issue are these this_cpu_read() and this_cpu_write() operations since people use those irresponsibly. It usually does not make sense to read a value from a random cpu nor does writing make sense. The situation is different for per cpu counter increments where it does not matter which cpus counter is incremented since we sum them up later anyways.

How about getting rid of this_cpu_read() and this_cpu_write() entirely?

Only allow __this_cpu_read and __this_cpu_write. There we check that the caller has disabled preemption.

For the rare special cases (are there any?) that are legitimate use cases for this_cpu_read/write we can use manual determination of per cpu pointers and then just do a load via the pointer?

Or alternatively give this_cpu_read and write special names that make their dangerousness clear.

In the case of slub there are only some this_cpu_write() things that can be __this_cpu_write without a problem.

The __this_cpu_ptr() can become this_cpu_ptr as far as I can tell. This should make it consistent so that we can check for disabled preemption for all __this_cpu thingies.

The problem I have with this, is that this does not help at all. We are back to the word "this_cpu" when you really do mean "any_cpu". We

Again, lets just bite the bullet and rename them to something that is understandable for everyone. This would make all of us happy.

I'm not against your code, I'm against the naming convention you decided to use. It makes it confusing to something that is complex and complex code needs to try to be a simple as possible.

Quoted 2 lines · Steven Rostedt

The problem I have with this, is that this does not help at all. We are back to the word "this_cpu" when you really do mean "any_cpu". We

We mean the current cpu while the instruction is executing.

Quoted 7 lines

Again, lets just bite the bullet and rename them to something that is understandable for everyone. This would make all of us happy.

I'm not against your code, I'm against the naming convention you decided to use. It makes it confusing to something that is complex and complex code needs to try to be a simple as possible.

I am certainly open to new ways of structuring the stuff and for any improvement possible. This whole scheme was developed in long discussions over many years. My initial proposal was to use cpu_inc/dec etc. I did what was possible in the context of the various opinions that people had on these matters when the implementations were discussed. The "you" does not apply. It is either "us" or "them" (if you want to distance yourself from this).

Quoted 4 lines · Steven Rostedt

BTW, Can you explain to me where the this_cpu_*() ops were designed to be used? The only places where "this_cpu_*()" is used in slub.c and page_alloc.c have irqs disabled on their use. I thought this was for slub and page_alloc?

Initially these were used for statistics that used per cpu counters. The slub thing was an outgrow of this.

Quoted 2 lines

Is this_cpu() made just for statistics? I see it used in the inode code for that, and some accounting in the namespace.c code.

That is the main use case yes.

Quoted 11 lines

Note and there's places all over the kernel that uses this_cpu_read() and thinks preemption should be disabled. Just look at arch/x86/mm/tlb.c:

	/* Caller has disabled preemption */
	sender = this_cpu_read(tlb_vector_offset);

Why the comment?

My argument is that this_cpu_* is just confusing. Rename your use case and keep this_cpu_*() as what you want __this_cpu_*() to be.

Thought about this a bit last night. I think the main issue are these this_cpu_read() and this_cpu_write() operations since people use those irresponsibly. It usually does not make sense to read a value from a random cpu nor does writing make sense. The situation is different for per cpu counter increments where it does not matter which cpus counter is incremented since we sum them up later anyways.

How about getting rid of this_cpu_read() and this_cpu_write() entirely?

Only allow __this_cpu_read and __this_cpu_write. There we check that the caller has disabled preemption.

For the rare special cases (are there any?) that are legitimate use cases for this_cpu_read/write we can use manual determination of per cpu pointers and then just do a load via the pointer?

Or alternatively give this_cpu_read and write special names that make their dangerousness clear.

In the case of slub there are only some this_cpu_write() things that can be __this_cpu_write without a problem.

The __this_cpu_ptr() can become this_cpu_ptr as far as I can tell. This should make it consistent so that we can check for disabled preemption for all __this_cpu thingies.

I just found out that the this_cpu_*() functions do not perform the test to see if the usage is in atomic or not. Thus, the blind conversion of the per_cpu(*, smp_processor_id()) and the get_cpu_var() code to this_cpu_*() introduce the regression to detect the hard to find case where a per cpu variable is used in preempt code that migrates and causes bugs.

Quoted 6 lines · Steven Rostedt

I just found out that the this_cpu_*() functions do not perform the test to see if the usage is in atomic or not. Thus, the blind conversion of the per_cpu(*, smp_processor_id()) and the get_cpu_var() code to this_cpu_*() introduce the regression to detect the hard to find case where a per cpu variable is used in preempt code that migrates and causes bugs.

Didn't preempt-rt recently get changed to not migrate in kernel-preempt regions. How about just fixing the normal preemption to not do this either.

Then all these complications wouldn't be necessary and a whole lot of code related to this could be removed too, and you would still have less bugs.

Actually, that's part of the issue. RT has made spin_locks not migrate. But this has also increased the overhead of those same spinlocks. I'm hoping to do away with the big hammer approach (although Thomas is less interested in this). I would like to have areas that require per-cpu variables to be annotated, and not have every spinlock disable preemption.

Quoted 8 lines · Andi Kleen

Quoted 6 lines · Steven Rostedt

I just found out that the this_cpu_*() functions do not perform the test to see if the usage is in atomic or not. Thus, the blind conversion of the per_cpu(*, smp_processor_id()) and the get_cpu_var() code to this_cpu_*() introduce the regression to detect the hard to find case where a per cpu variable is used in preempt code that migrates and causes bugs.

Quoted 3 lines

Didn't preempt-rt recently get changed to not migrate in kernel-preempt regions. How about just fixing the normal preemption to not do this either.

Actually, that's part of the issue. RT has made spin_locks not migrate. But this has also increased the overhead of those same spinlocks. I'm hoping to do away with the big hammer approach (although Thomas is less interested in this). I would like to have areas that require per-cpu variables to be annotated,

and not have every spinlock disable preemption.

Quoted 1 line

and not have every spinlock disable preemption.

That doesn't work, you're prone to deadlocks then. I guess you meant not disable migration on RT, right?

Quoted 9 lines · Steven Rostedt

Quoted 8 lines · Andi Kleen

Quoted 6 lines · Steven Rostedt

I just found out that the this_cpu_*() functions do not perform the test to see if the usage is in atomic or not. Thus, the blind conversion of the per_cpu(*, smp_processor_id()) and the get_cpu_var() code to this_cpu_*() introduce the regression to detect the hard to find case where a per cpu variable is used in preempt code that migrates and causes bugs.

Just for the record. I added some this_cpu_* debug checks to my filesystem eating 2.6.38-rt and guess what: They trigger right away in the FS code and without digging deeper I'm 100% sure, that this is the root cause of the problems I was hunting for weeks. Thanks for wasting my time and racking my nerves.

Can we please put that on the KS agenda? This definitely needs to be addressed urgently.

Well yes the misunderstanding of per cpu operations was one reason why I proposed the discussion on the subject of esoteric kernel synchronization. I do not think that it was accepted.

Quoted 4 lines · Christoph Lameter

Well yes the misunderstanding of per cpu operations was one reason why I proposed the discussion on the subject of esoteric kernel synchronization. I do not think that it was accepted.

I don't think anybody here misunderstands it, we're just all very angry that its causing so much problems.

__this_cpu doesn't have preempt debug checks, and there's a lot of this_cpu usage that really should have been __this_cpu.