Re: [PATCH v8 03/20] xenctx: Add -n (--display-stack-pages) option to output larger stack

[Don Slutz <dslutz@verizon.com>]

On 04/01/14 09:25, George Dunlap wrote:
> On 04/01/2014 02:19 PM, Ian Campbell wrote:
>> On Thu, 2014-03-27 at 15:05 -0400, Don Slutz wrote:
>>> Important: This is the stack size (also known as stack limit) to
>>> display not the configured stack size.
>>>
>>> Note: use with caution (easy to get garbage).
>>>
>>> Below is a pictures of a configured 3 page stack, and where
>>> the SP currently is.  Each box is a page.
>>>
>>>                              -n 1    -n 2    -n 3
>>>
>>>         +------------------+
>>>         |                  |
>>>         |                  |
>>>         |                  |
>>>         |                  |
>>> SP --> |                  |   *       *       *
>>>         +------------------+             |          |
>>>         |                  |          |          |
>>>         |                  |          |          |
>>>         |                  |          |          |
>>>         |                  |          |          |
>>>         |                  |          *          |
>>>         +------------------+                    |
>>>         |                  |              |
>>>         |                  |              |
>>>         |                  |              |
>>>         |                  |              |
>>>         |                  |              *
>>>         +------------------+
>>>
>>> Display using "-n 3" since the used stack pages is 3.
>>
>> Stacks grow downwards on all of the architectures we support. Perhaps
>> that is why the rest of us find your diagrams so confusing?
>>

I expect so.  I can flip the picture (and add relative address) to help.  I was basing
the layout to match the stack output:

Call Trace:
                     [<ffffffff81346898>] io_serial_out+0x18 <--
ffff880032bb1310:   [<ffffffff81346f51>] serial8250_console_putchar+0x31
ffff880032bb1330:   [<ffffffff813428de>] uart_console_write+0x3e
ffff880032bb1338:   [<ffffffff8100bc0e>] apic_timer_interrupt+0xe
ffff880032bb1370:   [<ffffffff813472ad>] serial8250_console_write+0xbd
ffff880032bb13c0:   [<ffffffff8106b8f5>] __call_console_drivers+0x75
ffff880032bb13f0:   [<ffffffff8106b95a>] _call_console_drivers+0x4a
ffff880032bb1410:   [<ffffffff8106be6e>] release_console_sem+0x4e
ffff880032bb1450:   [<ffffffff8106c628>] vprintk+0x248
ffff880032bb14f0:   [<ffffffff814fd363>] printk+0x41
ffff880032bb3f20:   [<ffffffff8100204c>] do_one_initcall+0x3c
ffff880032bb3f50:   [<ffffffff810b0eb1>] sys_init_module+0xe1
ffff880032bb3f80:   [<ffffffff8100b0f2>] system_call_fastpath+0x16

Adding (my) boxes (note not fully to scale):

     +------------------------------------------------------------------------------+
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                     [<ffffffff81346898>] io_serial_out+0x18 <--              |
SP->| ffff880032bb1310:   [<ffffffff81346f51>] serial8250_console_putchar+0x31     |
     | ffff880032bb1330:   [<ffffffff813428de>] uart_console_write+0x3e             |
     | ffff880032bb1338:   [<ffffffff8100bc0e>] apic_timer_interrupt+0xe            |
     | ffff880032bb1370:   [<ffffffff813472ad>] serial8250_console_write+0xbd       |
     | ffff880032bb13c0:   [<ffffffff8106b8f5>] __call_console_drivers+0x75         |
     | ffff880032bb13f0:   [<ffffffff8106b95a>] _call_console_drivers+0x4a          |
     | ffff880032bb1410:   [<ffffffff8106be6e>] release_console_sem+0x4e            |
     | ffff880032bb1450:   [<ffffffff8106c628>] vprintk+0x248                       |
     | ffff880032bb14f0:   [<ffffffff814fd363>] printk+0x41                         |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     +------------------------------------------------------------------------------+
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     +------------------------------------------------------------------------------+
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     |                                                                              |
     | ffff880032bb3f20:   [<ffffffff8100204c>] do_one_initcall+0x3c                |
     | ffff880032bb3f50:   [<ffffffff810b0eb1>] sys_init_module+0xe1                |
     | ffff880032bb3f80:   [<ffffffff8100b0f2>] system_call_fastpath+0x16           |
     |                                                                              |
     +------------------------------------------------------------------------------+



>> I drew my example to you the way I did for a reason.
>>

Sorry, I did not notice that.


>> Also, should the other end of the *---* line from SP (the bottom in your
>> diagram above) not be aligned to a page boundary, after all -n works in
>> pages.
>>

I am happy to do it either way.  I think of the page breaks to not be part of memory.


>>
>>> @@ -664,6 +667,8 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width)
>>>
>>>       stack_limit = ((stack_pointer(ctx) + XC_PAGE_SIZE)
>>>                      & ~((guest_word_t) XC_PAGE_SIZE - 1));
>>> +    if ( xenctx.nr_stack_pages > 1 )
>>> +        stack_limit += (xenctx.nr_stack_pages - 1) * XC_PAGE_SIZE;
>>
>> The if here is still redundant.
>>

Will drop.

>>>       printf("\n");
>>>       printf("Stack:\n");
>>>       for (i=1; i<5 && stack < stack_limit; i++) {
>>> @@ -834,18 +839,24 @@ static void usage(void)
>>>           kernel_start);
>>>       printf("  -a, --all          display more registers\n");
>>>       printf("  -C, --all-vcpus    print info for all vcpus\n");
>>> +    printf("  -n PAGES, --display-stack-pages=PAGES\n");
>>> +    printf("                     Display N pages from the stack pointer. (default %d)\n",
>>> +           DEFAULT_NR_STACK_PAGES);
>>> +    printf("                     Changes stack limit.  Note: use with caution (easy\n");
>>> +    printf("                     to get garbage).\n");
>>
>> Doesn't it go without saying that if you go off the bottom of the stack
>> you will get garbage?
>
> Only if you know that the tool doesn't know where the end of the stack is.  (Although hopefully the wording, "Display N pages" should give you a hint.)
>
> But what is "changes stack limit" supposed to mean?
>

Partly it is used in the next patch's usage output:

  -l <lines>, --lines <lines>
                      change the number of lines output for Stack. (default 5)
                      Can be specified as MAX.  Note: Fewer lines will be output
                      if stack limit reached.


And it was the best term I know of to say in a few words:

The output of stack starts at the current SP and stops when the
end of N page(s) is/are reached.

    -Don Slutz



>  -George
>