[Linux-ia64] switch_stack position

[Linux-ia64] switch_stack position

[Keith Owens]

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?

Re: [Linux-ia64] switch_stack position

[David Mosberger]

>>>>> On Wed, 13 Dec 2000 16:03:17 +1100, Keith Owens <kaos@melbourne.sgi.com> said:

  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

Re: [Linux-ia64] switch_stack position

[Keith Owens]

On Wed, 13 Dec 2000 18:56:21 -0800, 
David Mosberger <davidm@hpl.hp.com> wrote:
>>>>>> On Wed, 13 Dec 2000 16:03:17 +1100, Keith Owens <kaos@melbourne.sgi.com> said:
>  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.

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.

>  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.

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.

>  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).

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.

Re: [Linux-ia64] switch_stack position

[David Mosberger]

>>>>> On Thu, 14 Dec 2000 14:46:20 +1100, Keith Owens <kaos@ocs.com.au> said:

  Keith> On Wed, 13 Dec 2000 18:56:21 -0800, David Mosberger
  Keith> <davidm@hpl.hp.com> wrote:
  >>>>>>> On Wed, 13 Dec 2000 16:03:17 +1100, Keith Owens
  >>>>>>> <kaos@melbourne.sgi.com> said:
  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> 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

Re: [Linux-ia64] switch_stack position

[Keith Owens]

On Wed, 13 Dec 2000 20:39:17 -0800, 
David Mosberger <davidm@hpl.hp.com> wrote:
>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.

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.

Re: [Linux-ia64] switch_stack position

[David Mosberger]

>>>>> On Thu, 14 Dec 2000 16:13:06 +1100, Keith Owens <kaos@ocs.com.au> said:

  Keith> On Wed, 13 Dec 2000 20:39:17 -0800, David Mosberger
  Keith> <davidm@hpl.hp.com> wrote:
  >> 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.

  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

Re: [Linux-ia64] switch_stack position

[Keith Owens]

On Wed, 13 Dec 2000 22:21:49 -0800, 
David Mosberger <davidm@hpl.hp.com> wrote:
>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.

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?

Re: [Linux-ia64] switch_stack position

[David Mosberger]

>>>>> On Thu, 14 Dec 2000 17:31:07 +1100, Keith Owens <kaos@ocs.com.au> said:

  Keith> On Wed, 13 Dec 2000 22:21:49 -0800, David Mosberger
  Keith> <davidm@hpl.hp.com> wrote:
  >> 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.

  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

Re: [Linux-ia64] switch_stack position

[Keith Owens]

On Wed, 13 Dec 2000 22:36:46 -0800, 
David Mosberger <davidm@hpl.hp.com> wrote:
>  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?

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.

Re: [Linux-ia64] switch_stack position

[David Mosberger]

>>>>> On Thu, 14 Dec 2000 17:44:29 +1100, Keith Owens <kaos@ocs.com.au> said:

  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

Re: [Linux-ia64] switch_stack position

[Keith Owens]

On Wed, 13 Dec 2000 22:56:52 -0800, 
David Mosberger <davidm@hpl.hp.com> wrote:
>>>>>> On Thu, 14 Dec 2000 17:44:29 +1100, Keith Owens <kaos@ocs.com.au> said:
>
>  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?

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.

Re: [Linux-ia64] switch_stack position

[David Mosberger]

>>>>> On Thu, 14 Dec 2000 18:08:19 +1100, Keith Owens <kaos@ocs.com.au> said:

  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

Re: [Linux-ia64] switch_stack position

[Keith Owens]

On Wed, 13 Dec 2000 23:20:04 -0800, 
David Mosberger <davidm@hpl.hp.com> wrote:
>>>>>> On Thu, 14 Dec 2000 18:08:19 +1100, Keith Owens <kaos@ocs.com.au> said:
>
>  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.

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.

>  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.

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.

Re: [Linux-ia64] switch_stack position

[David Mosberger]

>>>>> On Thu, 14 Dec 2000 18:30:56 +1100, Keith Owens <kaos@ocs.com.au> said:

  >>  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.

  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