switch_stack position

IA64 code assumes that struct switch_stack always follows struct pt_regs on stack. This is not always true, unw_init_running() pushes switch_stack from anywhere, so pt_regs and switch_stack can be separate.

I am adding support for separate pt_regs and switch_stack by adding

	struct switch_stack *sw;
to struct thread and
	struct switch_stack *prev_sw;

to struct switch_stack. DO_SAVE_SWITCH_STACK and DO_LOAD_SWITCH_STACK track the position of the last switch_stack (LIFO), copy_thread sets prev_sw to NULL for a new process.

Besides fixing the incorrect assumption about the relative placement of pt_regs and switch_stack, this removes the need for kdb for ia64 to save switch_stack on every fault. Instead the switch_stack can be delayed until we know that kdb is actually going to do some work. It is a little more work for kdb to unwind from switch_stack back to the point that pt_regs was pushed but it will be much faster than DO_SAVE_SWITCH_STACK on every fault.

Before I spend too much time on this change, is there any obvious reason why separate pt_regs and switch_stack will not work, as long as I track where switch_stack is?

Keith> IA64 code assumes that struct switch_stack always follows Keith> struct pt_regs on stack.

Certainly not. There are a _few_ places where this is assumed, but it's certainly not true in general and IA-64 Linux certainly doesn't make that assumption in all but a few very specific places.

Keith> This is not always true, unw_init_running() pushes Keith> switch_stack from anywhere, so pt_regs and switch_stack can Keith> be separate.

Not to mention blocked threads...

Keith> I am adding support for separate pt_regs and switch_stack by Keith> adding struct switch_stack *sw; to struct thread and struct Keith> switch_stack *prev_sw; to struct switch_stack. Keith> DO_SAVE_SWITCH_STACK and DO_LOAD_SWITCH_STACK track the Keith> position of the last switch_stack (LIFO), copy_thread sets Keith> prev_sw to NULL for a new process.

Ouch.

Keith> Besides fixing the incorrect assumption about the relative Keith> placement of pt_regs and switch_stack,

You haven't shown a case where this assumption is made incorrectly.

Keith> this removes the need for kdb for ia64 to save switch_stack Keith> on every fault. Instead the switch_stack can be delayed Keith> until we know that kdb is actually going to do some work. It Keith> is a little more work for kdb to unwind from switch_stack Keith> back to the point that pt_regs was pushed but it will be much Keith> faster than DO_SAVE_SWITCH_STACK on every fault.

That may be a legitimate goal, but surely it doesn't warrant rewriting the context switch code. In fact, if you're willing to unwind, all the code you need is there already.

Keith> Before I spend too much time on this change, is there any Keith> obvious reason why separate pt_regs and switch_stack will not Keith> work, as long as I track where switch_stack is?

Like I said above, they already are separate. The only places where they are assumed to be consecutive is where (a) the switch stack is needed anyhow (clone(), for example) or where unwinding would be too costly (unaligned handler).

	--david

No big deal. 5 new instructions (1 extra bundle) in save_switch_stack, 3 new instructions (no extra bundles) in load_switch_stack. One assignment to 0 in copy_thread(). Plus one __u64 in struct switch_stack.

The problem is anything that wants to look at other processes on SMP; kdb and get_wchan are two examples. Currently kdb assumes that any process which is not current on _this_ cpu is blocked. But the process could be running on another cpu. kdb needs a safe way of getting the last switch_stack for any process or of determining that the process has no switch_stack and therefore cannot be reported.

My earlier mail was a bit strong, I had missed unw_init_running(), there are no comments on that function to say what it is doing. Nevertheless I have a need to safely find the switch_stack data on arbitrary processes. Tracking the position of the latest switch_stack via a LIFO chain is cheap for the benefits it gives.

Keith> On Wed, 13 Dec 2000 18:56:21 -0800, David Mosberger

Keith> I am adding support for separate pt_regs and switch_stack by Keith> adding struct switch_stack *sw; to struct thread and struct Keith> switch_stack *prev_sw; to struct switch_stack. Keith> DO_SAVE_SWITCH_STACK and DO_LOAD_SWITCH_STACK track the Keith> position of the last switch_stack (LIFO), copy_thread sets Keith> prev_sw to NULL for a new process.

Keith> No big deal. 5 new instructions (1 extra bundle) in Keith> save_switch_stack, 3 new instructions (no extra bundles) in Keith> load_switch_stack. One assignment to 0 in copy_thread(). Keith> Plus one __u64 in struct switch_stack.

The "Ouch" was meant as in "over my dead body". ;-)

Keith> The problem is anything that wants to look at other processes Keith> on SMP; kdb and get_wchan are two examples. Currently kdb Keith> assumes that any process which is not current on _this_ cpu Keith> is blocked. But the process could be running on another cpu. Keith> kdb needs a safe way of getting the last switch_stack for any Keith> process or of determining that the process has no Keith> switch_stack and therefore cannot be reported.

Note that the unwind code is written such that it cannot fault, even if the initial state on the stack is completely bogus. This is why it's safe to call get_wchan() on a process that's running.

	--david

Safe but it gives misleading results which confuse people doing debugging. I had the same problem on kdb for ia32. Doing backtrace of the running process on cpu 0 from cpu 1 (btp pid) gave different results compared to backtrace of the current process on cpu 0 (bt). Debuggers should not give different results for the same process just because you are looking from another cpu.

The solution is the same on ia32 and ia64. The debugger attempts to get all processes into a known state. That state is recorded in struct task. Then the debugger can look at any process from any cpu and see a consistent state.

Keith> On Wed, 13 Dec 2000 20:39:17 -0800, David Mosberger

Keith> Safe but it gives misleading results which confuse people Keith> doing debugging. I had the same problem on kdb for ia32. Keith> Doing backtrace of the running process on cpu 0 from cpu 1 Keith> (btp pid) gave different results compared to backtrace of the Keith> current process on cpu 0 (bt). Debuggers should not give Keith> different results for the same process just because you are Keith> looking from another cpu.

Sure, kdb should do it's job right, but kdb is certainly no reason to to hack the context switch code. Not for something that 99.9% of the users never even know existed.

	--david

One pointer in struct thread, one pointer in switch_stack, ~ 15 instructions (2-3 extra bundles) to set the pointers during save_switch_stack, load_switch_stack and copy_thread. Is that too big a change?

Keith> On Wed, 13 Dec 2000 22:21:49 -0800, David Mosberger

Keith> One pointer in struct thread, one pointer in switch_stack, ~ Keith> 15 instructions (2-3 extra bundles) to set the pointers Keith> during save_switch_stack, load_switch_stack and copy_thread. Keith> Is that too big a change?

It's not the amount of work the change that's involved, it's that the change makes no sense whatsoever. Surely you can find a better solution?

	--david

I need to look at an arbitrary process and determine its stack state accurately. If you can think of an method which does not require tracking the switch_stack (or current top of stack, it is almost the same thing) for all processes, let me know. I looked at this for a couple of weeks without finding a better solution.

Keith> I need to look at an arbitrary process and determine its Keith> stack state accurately. If you can think of an method which Keith> does not require tracking the switch_stack (or current top of Keith> stack, it is almost the same thing) for all processes, let me Keith> know. I looked at this for a couple of weeks without finding Keith> a better solution.

Since you need to stop the CPUs anyhow, why not just have the CPUs store their "current" values in a global array?

	--david

There is no guarantee that I can stop all the cpus. Sometimes the problem is so bad that the cpu will not accept the IPI. This is exactly the case when a debugger needs to be able to print the processor on the unresponsive cpu, or at the very least to say what state the process is in. kdb can detect unresponsive cpus and will not let you switch to them but you can still issue 'btp pid' to get some data for the offending cpu. And that needs the location of the last switch_stack for the process.

Keith> There is no guarantee that I can stop all the cpus. Keith> Sometimes the problem is so bad that the cpu will not accept Keith> the IPI. This is exactly the case when a debugger needs to Keith> be able to print the processor on the unresponsive cpu, or at Keith> the very least to say what state the process is in.

If the CPU is still running, there is no guarantee that the existence of a switch-stack implies that the process is stopped. For example, it may just be unwinding the stack via unw_init_running() and by the time kdb attempts to read the stack, the switch-stack may have disappeared again.

Keith> kdb can detect unresponsive cpus and will not let you switch Keith> to them but you can still issue 'btp pid' to get some data Keith> for the offending cpu. And that needs the location of the Keith> last switch_stack for the process.

Have you actually tried doing a "btp" on an IA-64 stack that isn't valid? I think you'll find that unw_unwind() returns -1 long before unwinding to user-space. That's probably as good an indication as any that the stack is still active.

	--david

Possible but extremely unlikely. If the cpu is not responding to IPI then it is unlikely to be unwinding. kdb IPI on ia32 uses NMI, I plan to try using NMI for ia64 as well. A cpu that does not respond to NMI is in a sick state.

The other problem with a global per cpu array is debugging the debugger. There is some support in recent version of SGI kdb for debugging kdb itself. That requires stack data for the original fault plus a separate set of stack data for the kdb error. At which point I need to store the current state plus the previous state, the stack is the best place for this.

But I don't want unwind to return -1 or produce garbage, which is what it occurs now when the stack state does not match the assumptions. I want it to use the last saved switch_stack for that process, it is the only clean starting point for debugging. A clean debug report is far better than producing a random backtrace and sending users off on a wild goose chase.

Keith> But I don't want unwind to return -1 or produce garbage, Keith> which is what it occurs now when the stack state does not Keith> match the assumptions.

What's so hard about writing a function that returns 1 if it was able to unwind the kernel stack completely and 0 otherwise?

	--david