stack_used() not accurate?

stack_used() not accurate?

[Mike Snitzer]

[-- Attachment #1: Type: text/plain, Size: 475 bytes --]

Hi,

When I use the attached script for all ext3 call-chains stack_used()
is _always_ 481.  I'm running the latest systemtap.git on an RHEL5
x86_64 system (though older versions of systemtap showed the same
481).

I've tested switching the module from ext3 to others and 481 doesn't change.

Looking at tapset/context.stp the implementation for stack_used()
looks fine.  But given the constant 481 I'm seeing is systemtap
somehow showing systemtap-tainted stack usage?

Mike

[-- Attachment #2: stack_usage.stp --]
[-- Type: application/octet-stream, Size: 693 bytes --]


global stack_usage, stack_trace

probe module("ext3").function("*") {
	__stack_used = stack_used()
	if (__stack_used > stack_usage[tid(), execname()]) {
		stack_usage[tid(), execname()] = __stack_used
		stack_trace[tid()] = backtrace()
	}
}

probe timer.ms(10000) {
	printf("********************** %s *************************\n",
	       ctime(gettimeofday_s()))

	foreach ([task, name] in stack_usage+) {
		printf("tid: %8d  name: %15s  stack_used: %6d\n", task, name, stack_usage[task, name])
		print_stack(stack_trace[task])
		printf("\n")
	}

	printf("*************************************************************************\n")
}

probe end {
	delete stack_usage
	delete stack_trace
}

Re: stack_used() not accurate?

[Wenji Huang]

Mike Snitzer wrote:
> Hi,
> 
> When I use the attached script for all ext3 call-chains stack_used()
> is _always_ 481.  I'm running the latest systemtap.git on an RHEL5
> x86_64 system (though older versions of systemtap showed the same
> 481).
> 
> I've tested switching the module from ext3 to others and 481 doesn't change.
> 
> Looking at tapset/context.stp the implementation for stack_used()
> looks fine.  But given the constant 481 I'm seeing is systemtap
> somehow showing systemtap-tainted stack usage?
> 
> Mike

Hi Mike,

I did a test and found it kept the constant value. And I changed to 
another function which is copied and tailored from check_stack_usage(). 
The function is added since 2.6.23, but has no much dependency on kernel 
version. Seems it works fine on my boxes(x86, x86_64).

function stack_used_new:long() %{ /*pure */
  {
          unsigned long *n = end_of_stack(current);
          unsigned long free;
          while (*n == 0)
                  n++;
          free = (unsigned long)n - (unsigned long)end_of_stack(current);

          THIS->__retvalue = THREAD_SIZE - free;
}
%}

Regards,
Wenji

Re: stack_used() not accurate?

[Frank Ch. Eigler]

Wenji Huang <wenji.huang@oracle.com> writes:

> [...]  I did a test and found it kept the constant value.

If the kernel switches to a separate stack for int3 handling, that
could explain the constant value.

> function stack_used_new:long() %{ /*pure */
>  {
>          unsigned long *n = end_of_stack(current);
>          unsigned long free;
>          while (*n == 0)
>                  n++;
>          free = (unsigned long)n - (unsigned long)end_of_stack(current);
>          THIS->__retvalue = THREAD_SIZE - free;
> }
> %}

This searching for non-0 values seems dangerously unbounded.  Plus it
may measure the *maximum historical* rather current stack usage.


- FChE

Re: stack_used() not accurate?

[Mike Snitzer]

On Thu, May 29, 2008 at 10:48 AM, Frank Ch. Eigler <fche@redhat.com> wrote:
> Wenji Huang <wenji.huang@oracle.com> writes:
>
>> [...]  I did a test and found it kept the constant value.
>
> If the kernel switches to a separate stack for int3 handling, that
> could explain the constant value.

Thanks for the explanation.

>> function stack_used_new:long() %{ /*pure */
>>  {
>>          unsigned long *n = end_of_stack(current);
>>          unsigned long free;
>>          while (*n == 0)
>>                  n++;
>>          free = (unsigned long)n - (unsigned long)end_of_stack(current);
>>          THIS->__retvalue = THREAD_SIZE - free;
>> }
>> %}
>
> This searching for non-0 values seems dangerously unbounded.  Plus it
> may measure the *maximum historical* rather current stack usage.

Yes, when using Wenji's stack_used_new() I'm seeing free=0 quite
frequently; obviously that isn't useful/accurate.

I'll be exploring other implementations, but any suggestions would be
very welcome.

Mike

Re: stack_used() not accurate?

[Frank Ch. Eigler]

"Mike Snitzer" <snitzer@gmail.com> writes:

> [...]  I'll be exploring other implementations, but any suggestions
> would be very welcome.

THREAD_SIZE should give a good normal kernel stack size.  The
kprobe-context stack pointer would be better decoded from its pt_regs
rather than that embedded-c &a hack.

- FChE

Re: stack_used() not accurate?

[Mike Snitzer]

On Thu, May 29, 2008 at 12:29 PM, Frank Ch. Eigler <fche@redhat.com> wrote:
>
> "Mike Snitzer" <snitzer@gmail.com> writes:
>
>> [...]  I'll be exploring other implementations, but any suggestions
>> would be very welcome.
>
> THREAD_SIZE should give a good normal kernel stack size.  The
> kprobe-context stack pointer would be better decoded from its pt_regs
> rather than that embedded-c &a hack.

I came up with the following for x86_64:

%( arch == "x86_64" %?
function stack_used_new:long() %{
        unsigned long free = THREAD_SIZE;
        if (CONTEXT->regs) {
                u64 curbase = (u64)task_stack_page(current);
#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,24)
                unsigned long sp = CONTEXT->regs->sp;
#else
                unsigned long sp = CONTEXT->regs->rsp;
#endif
                free = sp - (curbase + sizeof(struct thread_info));
        }
        THIS->__retvalue = THREAD_SIZE - free;
%}
%)

I haven't looked at x86 yet.

Mike

Re: stack_used() not accurate?

[Mike Snitzer]

On Thu, May 29, 2008 at 5:23 PM, Mike Snitzer <snitzer@gmail.com> wrote:
> On Thu, May 29, 2008 at 12:29 PM, Frank Ch. Eigler <fche@redhat.com> wrote:
>>
>> "Mike Snitzer" <snitzer@gmail.com> writes:
>>
>>> [...]  I'll be exploring other implementations, but any suggestions
>>> would be very welcome.
>>
>> THREAD_SIZE should give a good normal kernel stack size.  The
>> kprobe-context stack pointer would be better decoded from its pt_regs
>> rather than that embedded-c &a hack.
>
> I came up with the following for x86_64:
>
> %( arch == "x86_64" %?
> function stack_used_new:long() %{
>        unsigned long free = THREAD_SIZE;
>        if (CONTEXT->regs) {
>                u64 curbase = (u64)task_stack_page(current);
> #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,24)
>                unsigned long sp = CONTEXT->regs->sp;
> #else
>                unsigned long sp = CONTEXT->regs->rsp;
> #endif
>                free = sp - (curbase + sizeof(struct thread_info));
>        }
>        THIS->__retvalue = THREAD_SIZE - free;
> %}
> %)
>
> I haven't looked at x86 yet.


Here is the x86 version (from irq_32.c's do_IRQ):

function stack_used_new:long() %{
        unsigned long free = THREAD_SIZE;
        if (CONTEXT->regs) {
                long sp;
                __asm__ __volatile__("andl %%esp,%0" :
                                        "=r" (sp) : "0" (THREAD_SIZE - 1));
                free = sp - sizeof(struct thread_info);
        }
        THIS->__retvalue = THREAD_SIZE - free;
%}


Along the way I've uncovered what seems to be an issue with
systemtap's print_stack() on x86.  There are, what appears to be, stap
mechanics and probe point details getting into the stack listing
(which mask portions of the real call-chain).

Here is the output I get from my stp script using rhel5u2's stap on x86:

tid:      342  name:       kjournald  stack_used:    888
 0xe0881efe : verify_chain+0x1/0x20 [ext3]
 0xe08828fe : ext3_get_branch+0x6a/0xba [ext3]
 0xdfab0400 : _einittext+0x1f3a5363/0x0
 0xdf8afe20 : _einittext+0x1f1a4d83/0x0
 0xdf8afe90 : _einittext+0x1f1a4df3/0x0
 0xe0882ae6 : ext3_get_blocks_handle+0xaf/0x8cc [ext3]
 0xdf8afde0 : _einittext+0x1f1a4d43/0x0
 0xdf8afe3c : _einittext+0x1f1a4d9f/0x0
 0xdf8afe1c : _einittext+0x1f1a4d7f/0x0
 0xe0acf78c : _stp_map_del_isi+0xe31c/0x0
[stap_520b1eed653befb9ee369b8b8ed8e489_571886]
 0xdfa4ce48 : _einittext+0x1f341dab/0x0
 0x0000d0da

Here is with a very recent git stap (rhel52-062-1-339-g2285f44) on rhel5u2 x86:

tid:      342  name:       kjournald  stack_used:    888
 0xe0881efe : verify_chain+0x1/0x20 [ext3]
 0xe08828fe : ext3_get_branch+0x6a/0xba [ext3]
 0xdfab0400 : packet_exit+0x1f390993/0x0
 0xdf8afe20 : packet_exit+0x1f1903b3/0x0
 0xdf8afe90 : packet_exit+0x1f190423/0x0
 0xe0882ae6 : ext3_get_blocks_handle+0xaf/0x8cc [ext3]
 0xdf8afde0 : packet_exit+0x1f190373/0x0
 0xdf8afe3c : packet_exit+0x1f1903cf/0x0
 0xdf8afe1c : packet_exit+0x1f1903af/0x0
 0xce2cdf8a : packet_exit+0xdbae51d/0x0
 0xc5ce2cdf : packet_exit+0x55c3272/0x0
 0x0000ce48

Here is the clean stack I've come to expect from using stap on x86_64:

tid:      539  name:       kjournald  stack_used:   1256
 0xffffffff8804f3a9 : verify_chain+0x1/0x1e [ext3]
 0xffffffff8804fe44 : ext3_get_branch+0x7a/0xd2 [ext3]
 0xffffffff8805005b : ext3_get_blocks_handle+0xbd/0x8e6 [ext3]
 0xffffffff8044cb5c : schedule+0x95c/0xa4d
 0xffffffff803005ea : elv_merged_request+0x30/0x39
 0xffffffff88050b70 : ext3_get_block+0xc2/0xe4 [ext3]
 0x00000ffffffff802

Mike

Re: stack_used() not accurate?

[Frank Ch. Eigler]

Hi -

On Fri, May 30, 2008 at 03:39:54PM -0400, Mike Snitzer wrote:
> On Thu, May 29, 2008 at 5:23 PM, Mike Snitzer <snitzer@gmail.com> wrote:
> > I came up with the following for x86_64:
> >
> > %( arch == "x86_64" %?
> > function stack_used_new:long() %{
> >        unsigned long free = THREAD_SIZE;
> >        if (CONTEXT->regs) {
> >                u64 curbase = (u64)task_stack_page(current);
> > #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,24)
> >                unsigned long sp = CONTEXT->regs->sp;
> > #else
> >                unsigned long sp = CONTEXT->regs->rsp;
> > #endif
> >                free = sp - (curbase + sizeof(struct thread_info));
> >        }
> >        THIS->__retvalue = THREAD_SIZE - free;
> > %}
> > %)

This looks OK, but REGS_SP(CONTEXT->regs) should do the right thing
without that #if stuff.  Actually, that may be enough to make this
function architecture-independent.


> Here is the x86 version (from irq_32.c's do_IRQ):
> 
> function stack_used_new:long() %{
>         unsigned long free = THREAD_SIZE;
>         if (CONTEXT->regs) {
>                 long sp;
>                 __asm__ __volatile__("andl %%esp,%0" :
>                                         "=r" (sp) : "0" (THREAD_SIZE - 1));
>                 free = sp - sizeof(struct thread_info);
>         }
>         THIS->__retvalue = THREAD_SIZE - free;
> %}

(This version doesn't use CONTEXT->regs, which it should.)


> Along the way I've uncovered what seems to be an issue with
> systemtap's print_stack() on x86.  [...]

Yeah, that's one of several smelly bits that we're working on.


- FChE

Re: stack_used() not accurate?

[Mike Snitzer]

On Fri, May 30, 2008 at 6:40 PM, Frank Ch. Eigler <fche@redhat.com> wrote:
> Hi -
>
> On Fri, May 30, 2008 at 03:39:54PM -0400, Mike Snitzer wrote:
>> On Thu, May 29, 2008 at 5:23 PM, Mike Snitzer <snitzer@gmail.com> wrote:
>> > I came up with the following for x86_64:
>> >
>> > %( arch == "x86_64" %?
>> > function stack_used_new:long() %{
>> >        unsigned long free = THREAD_SIZE;
>> >        if (CONTEXT->regs) {
>> >                u64 curbase = (u64)task_stack_page(current);
>> > #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,24)
>> >                unsigned long sp = CONTEXT->regs->sp;
>> > #else
>> >                unsigned long sp = CONTEXT->regs->rsp;
>> > #endif
>> >                free = sp - (curbase + sizeof(struct thread_info));
>> >        }
>> >        THIS->__retvalue = THREAD_SIZE - free;
>> > %}
>> > %)
>
> This looks OK, but REGS_SP(CONTEXT->regs) should do the right thing
> without that #if stuff.  Actually, that may be enough to make this
> function architecture-independent.

It's actually REG_SP(CONTEXT->regs) but thanks for the pointer.  It
could be that the function is now arch-independent but on x86 it would
appear there is something awry with CONTEXT->regs->sp.  Please see
below.

>> Here is the x86 version (from irq_32.c's do_IRQ):
>>
>> function stack_used_new:long() %{
>>         unsigned long free = THREAD_SIZE;
>>         if (CONTEXT->regs) {
>>                 long sp;
>>                 __asm__ __volatile__("andl %%esp,%0" :
>>                                         "=r" (sp) : "0" (THREAD_SIZE - 1));
>>                 free = sp - sizeof(struct thread_info);
>>         }
>>         THIS->__retvalue = THREAD_SIZE - free;
>> %}
>
> (This version doesn't use CONTEXT->regs, which it should.)

Agreed, but unfortunately if I try to use the value stored in
CONTEXT->regs->esp I don't get the actual stack pointer.  I had a look
at the assembly of the systemtap compiled ko that uses __asm__ (like
above), it works and looks like:
mov    $0xfff,%eax
and    %esp,%eax
lea    0xffffffc8(%eax),%edx
mov    $0x1000,%eax
sub    %edx,%eax
mov    %eax,(%ebx)
movl   $0x0,0x4(%ebx)

whereas the following c-code does _not_ work:
if (CONTEXT->regs) {
    long sp = REG_SP(CONTEXT->regs) & (THREAD_SIZE - 1);
    free = sp - sizeof(struct thread_info);
}
THIS->__retvalue = THREAD_SIZE - free;

the associated assembly is:
mov    0x98(%edi),%eax
mov    0x34(%eax),%eax
and    $0xfff,%eax
lea    0xffffffc8(%eax),%edx
mov    $0x1000,%eax
sub    %edx,%eax
mov    %eax,(%ebx)
movl   $0x0,0x4(%ebx)

The C-code is doing what we'd expect (dereferencing the esp member of
struct pt_reg*):
crash> p &((struct pt_regs *)0)->esp
$2 = (long int *) 0x34

So, given that the calculated "free" is incorrect, this says to me
that the incorrect value is getting stored in CONTEXT->regs->esp on
x86 (or the value is stale)?

>> Along the way I've uncovered what seems to be an issue with
>> systemtap's print_stack() on x86.  [...]
>
> Yeah, that's one of several smelly bits that we're working on.

OK, good to know..

Mike

Re: stack_used() not accurate?

[Frank Ch. Eigler]

Hi -

On Mon, Jun 02, 2008 at 03:02:07PM -0400, Mike Snitzer wrote:
> [...]
> >>       __asm__ __volatile__("andl %%esp,%0" :
> >>                            "=r" (sp) : "0" (THREAD_SIZE - 1));
> 
> whereas the following c-code does _not_ work:

> if (CONTEXT->regs) {
>     long sp = REG_SP(CONTEXT->regs) & (THREAD_SIZE - 1);
> [...]
> 
> So, given that the calculated "free" is incorrect, this says to me
> that the incorrect value is getting stored in CONTEXT->regs->esp on
> x86 (or the value is stale)?

Why do you think the latter is incorrect?  The asm "esp"-related one
should give you the stack pointer at the moment of the probe handler
execution.  The CONTEXT->regs->esp value should give the stack pointer
at the moment of the probe hit - displaced in time (earlier) and
potentially in space (different stack).  This is the dimension of
imagination.  It is an area which we call the twilight zone.


- FChE

Re: stack_used() not accurate?

[Mike Snitzer]

On Mon, Jun 2, 2008 at 4:29 PM, Frank Ch. Eigler <fche@redhat.com> wrote:
> Hi -
>
> On Mon, Jun 02, 2008 at 03:02:07PM -0400, Mike Snitzer wrote:
>> [...]
>> >>       __asm__ __volatile__("andl %%esp,%0" :
>> >>                            "=r" (sp) : "0" (THREAD_SIZE - 1));
>>
>> whereas the following c-code does _not_ work:
>
>> if (CONTEXT->regs) {
>>     long sp = REG_SP(CONTEXT->regs) & (THREAD_SIZE - 1);
>> [...]
>>
>> So, given that the calculated "free" is incorrect, this says to me
>> that the incorrect value is getting stored in CONTEXT->regs->esp on
>> x86 (or the value is stale)?
>
> Why do you think the latter is incorrect?  The asm "esp"-related one
> should give you the stack pointer at the moment of the probe handler
> execution.  The CONTEXT->regs->esp value should give the stack pointer
> at the moment of the probe hit - displaced in time (earlier) and
> potentially in space (different stack).  This is the dimension of
> imagination.  It is an area which we call the twilight zone.

I understand that CONTEXT->regs->esp has the properties you described;
completely explains why it is so important to use CONTEXT->regs.  But
the problem, on x86, is that if I use CONTEXT->regs->esp I get a
negative values for "free" or <100 Bytes free.  I know that not to be
the case...

Mike

Re: stack_used() not accurate?

[Jim Keniston]

On Mon, 2008-06-02 at 17:23 -0400, Mike Snitzer wrote:
> On Mon, Jun 2, 2008 at 4:29 PM, Frank Ch. Eigler <fche@redhat.com> wrote:
...
> >>
> >> So, given that the calculated "free" is incorrect, this says to me
> >> that the incorrect value is getting stored in CONTEXT->regs->esp on
> >> x86 (or the value is stale)?
> >
> > Why do you think the latter is incorrect?  The asm "esp"-related one
> > should give you the stack pointer at the moment of the probe handler
> > execution.  The CONTEXT->regs->esp value should give the stack pointer
> > at the moment of the probe hit - displaced in time (earlier) and
> > potentially in space (different stack).  This is the dimension of
> > imagination.  It is an area which we call the twilight zone.
> 
> I understand that CONTEXT->regs->esp has the properties you described;
> completely explains why it is so important to use CONTEXT->regs.  But
> the problem, on x86, is that if I use CONTEXT->regs->esp I get a
> negative values for "free" or <100 Bytes free.  I know that not to be
> the case...
> 
> Mike

Sorry, I haven't been following this thread for a while, so maybe this
has already been mentioned.  But keep in mind that on i386, when your
breakpoint trap happens in kernel code, esp and ss aren't saved on the
stack.  So regs->esp and regs->ss contain the top of the pre-trap stack,
and the pre-trap stack pointer is &regs->esp, not regs->esp.

Jim

Re: stack_used() not accurate?

[Mike Snitzer]

On Mon, Jun 2, 2008 at 5:44 PM, Jim Keniston <jkenisto@us.ibm.com> wrote:
> On Mon, 2008-06-02 at 17:23 -0400, Mike Snitzer wrote:
>> On Mon, Jun 2, 2008 at 4:29 PM, Frank Ch. Eigler <fche@redhat.com> wrote:
> ...
>> >>
>> >> So, given that the calculated "free" is incorrect, this says to me
>> >> that the incorrect value is getting stored in CONTEXT->regs->esp on
>> >> x86 (or the value is stale)?
>> >
>> > Why do you think the latter is incorrect?  The asm "esp"-related one
>> > should give you the stack pointer at the moment of the probe handler
>> > execution.  The CONTEXT->regs->esp value should give the stack pointer
>> > at the moment of the probe hit - displaced in time (earlier) and
>> > potentially in space (different stack).  This is the dimension of
>> > imagination.  It is an area which we call the twilight zone.
>>
>> I understand that CONTEXT->regs->esp has the properties you described;
>> completely explains why it is so important to use CONTEXT->regs.  But
>> the problem, on x86, is that if I use CONTEXT->regs->esp I get a
>> negative values for "free" or <100 Bytes free.  I know that not to be
>> the case...
>>
>> Mike
>
> Sorry, I haven't been following this thread for a while, so maybe this
> has already been mentioned.  But keep in mind that on i386, when your
> breakpoint trap happens in kernel code, esp and ss aren't saved on the
> stack.  So regs->esp and regs->ss contain the top of the pre-trap stack,
> and the pre-trap stack pointer is &regs->esp, not regs->esp.

OK, so something like this for x86?

function stack_used_new:long() %{
        long free = THREAD_SIZE;
        if (CONTEXT->regs) {
                long curbase = (long)task_stack_page(current);
                long *sp = (long *) &(REG_SP(CONTEXT->regs));
                free = (long)sp - (curbase + sizeof(struct thread_info));
        }
        THIS->__retvalue = THREAD_SIZE - free;
%}

The results look much more like I'd expect...

Re: stack_used() not accurate?

[Jim Keniston]

On Mon, 2008-06-02 at 18:27 -0400, Mike Snitzer wrote:
> On Mon, Jun 2, 2008 at 5:44 PM, Jim Keniston <jkenisto@us.ibm.com> wrote:
...
> >
> > Sorry, I haven't been following this thread for a while, so maybe this
> > has already been mentioned.  But keep in mind that on i386, when your
> > breakpoint trap happens in kernel code, esp and ss aren't saved on the
> > stack.  So regs->esp and regs->ss contain the top of the pre-trap stack,
> > and the pre-trap stack pointer is &regs->esp, not regs->esp.
> 
> OK, so something like this for x86?
> 
> function stack_used_new:long() %{
>         long free = THREAD_SIZE;
>         if (CONTEXT->regs) {
>                 long curbase = (long)task_stack_page(current);
>                 long *sp = (long *) &(REG_SP(CONTEXT->regs));
>                 free = (long)sp - (curbase + sizeof(struct thread_info));
>         }
>         THIS->__retvalue = THREAD_SIZE - free;
> %}
> 
> The results look much more like I'd expect...

Your calculation of free looks correct (although I'd make sp a long
rather than a long* to simplify things a bit).

But your return value seems to include the size of the struct
thread_info, which seems wrong.  I'd think you'd want
	THIS->__retvalue = (curbase + THREAD_SIZE) - sp;
or
	THIS->__retvalue = THREAD_SIZE - (sp & (THREAD_SIZE-1));
That seems more in keeping with the original implementation.

Jim

Re: stack_used() not accurate?

[Mike Snitzer]

On Mon, Jun 2, 2008 at 7:06 PM, Jim Keniston <jkenisto@us.ibm.com> wrote:
> On Mon, 2008-06-02 at 18:27 -0400, Mike Snitzer wrote:
>> On Mon, Jun 2, 2008 at 5:44 PM, Jim Keniston <jkenisto@us.ibm.com> wrote:
> ...
>> >
>> > Sorry, I haven't been following this thread for a while, so maybe this
>> > has already been mentioned.  But keep in mind that on i386, when your
>> > breakpoint trap happens in kernel code, esp and ss aren't saved on the
>> > stack.  So regs->esp and regs->ss contain the top of the pre-trap stack,
>> > and the pre-trap stack pointer is &regs->esp, not regs->esp.
>>
>> OK, so something like this for x86?
>>
>> function stack_used_new:long() %{
>>         long free = THREAD_SIZE;
>>         if (CONTEXT->regs) {
>>                 long curbase = (long)task_stack_page(current);
>>                 long *sp = (long *) &(REG_SP(CONTEXT->regs));
>>                 free = (long)sp - (curbase + sizeof(struct thread_info));
>>         }
>>         THIS->__retvalue = THREAD_SIZE - free;
>> %}
>>
>> The results look much more like I'd expect...
>
> Your calculation of free looks correct (although I'd make sp a long
> rather than a long* to simplify things a bit).

Yes, just need a cast to (long), thanks.

> But your return value seems to include the size of the struct
> thread_info, which seems wrong.  I'd think you'd want
>        THIS->__retvalue = (curbase + THREAD_SIZE) - sp;
> or
>        THIS->__retvalue = THREAD_SIZE - (sp & (THREAD_SIZE-1));
> That seems more in keeping with the original implementation.

Your 2nd suggestion always returns 4096.  As for your main concern,
struct thread_info is a "used" portion of the stack isn't it?

Mike

Re: stack_used() not accurate?

[Mike Snitzer]

On Mon, Jun 2, 2008 at 9:12 PM, Mike Snitzer <snitzer@gmail.com> wrote:
> On Mon, Jun 2, 2008 at 7:06 PM, Jim Keniston <jkenisto@us.ibm.com> wrote:
>> On Mon, 2008-06-02 at 18:27 -0400, Mike Snitzer wrote:
>>> On Mon, Jun 2, 2008 at 5:44 PM, Jim Keniston <jkenisto@us.ibm.com> wrote:
>> ...
>>> >
>>> > Sorry, I haven't been following this thread for a while, so maybe this
>>> > has already been mentioned.  But keep in mind that on i386, when your
>>> > breakpoint trap happens in kernel code, esp and ss aren't saved on the
>>> > stack.  So regs->esp and regs->ss contain the top of the pre-trap stack,
>>> > and the pre-trap stack pointer is &regs->esp, not regs->esp.
>>>
>>> OK, so something like this for x86?
>>>
>>> function stack_used_new:long() %{
>>>         long free = THREAD_SIZE;
>>>         if (CONTEXT->regs) {
>>>                 long curbase = (long)task_stack_page(current);
>>>                 long *sp = (long *) &(REG_SP(CONTEXT->regs));
>>>                 free = (long)sp - (curbase + sizeof(struct thread_info));
>>>         }
>>>         THIS->__retvalue = THREAD_SIZE - free;
>>> %}
>>>
>>> The results look much more like I'd expect...
>>
>> Your calculation of free looks correct (although I'd make sp a long
>> rather than a long* to simplify things a bit).
>
> Yes, just need a cast to (long), thanks.
>
>> But your return value seems to include the size of the struct
>> thread_info, which seems wrong.  I'd think you'd want
>>        THIS->__retvalue = (curbase + THREAD_SIZE) - sp;
>> or
>>        THIS->__retvalue = THREAD_SIZE - (sp & (THREAD_SIZE-1));
>> That seems more in keeping with the original implementation.
>
> Your 2nd suggestion always returns 4096.

Err, sorry; both of your suggestions yield identical results (I had a
typo in the code and stopped thinking momentarily).

Mike

[PATCH]Add lacked syscall.unshare to tapset

[Zhaolei]

Hi, everyone

Following patch adds support for lacked syscall.unshare.
If no objection, I will commit it.

Signed-off-by: "Zhaolei" zhaolei@cn.fujitsu.com

diff --git a/tapset/syscalls2.stp b/tapset/syscalls2.stp
index 0db5034..0f4e88e 100644
--- a/tapset/syscalls2.stp
+++ b/tapset/syscalls2.stp
@@ -2853,6 +2853,20 @@ probe syscall.unlink.return = kernel.function("sys_unlink").return {
  name = "unlink"
  retstr = returnstr(1)
 }
+
+# unshare ____________________________________________________
+# new function with 2.6.16
+# long sys_unshare(unsigned long unshare_flags)
+probe syscall.unshare = kernel.function("sys_unshare") ? {
+ name = "unshare"
+ unshare_flags = $unshare_flags
+ argstr = sprintf("%d", unshare_flags)
+}
+probe syscall.unshare.return = kernel.function("sys_unshare").return ? {
+ name = "unshare"
+ retstr = returnstr(1)
+}
+
 # uselib _____________________________________________________
 #
 # asmlinkage long

Regards
Zhaolei

Re: [PATCH]Add lacked syscall.unshare to tapset

[Mark Wielaard]

Hi Zhaolei,

On Tue, 2008-06-03 at 13:43 +0800, Zhaolei wrote:
> Following patch adds support for lacked syscall.unshare.
> If no objection, I will commit it.
>
> +# unshare ____________________________________________________
> +# new function with 2.6.16
> +# long sys_unshare(unsigned long unshare_flags)
> +probe syscall.unshare = kernel.function("sys_unshare") ? {
> + name = "unshare"
> + unshare_flags = $unshare_flags
> + argstr = sprintf("%d", unshare_flags)
> +}

Are these flags the same as one would use for clone/fork?
In that case you might want to use
  argstr = __fork_flags(unshare_flags)
__fork_flags() is defined in aux_syscalls.stp.

Cheers,

Mark

Re: [PATCH]Add lacked syscall.unshare to tapset

[Zhaolei]

Hello, Mark

Thank you for your advice.
It is a good idea to set flags as string in argstr.

In my opinion, sys_unshare support only following flags:

asmlinkage long sys_unshare(unsigned long unshare_flags)
{
    ...
    if (unshare_flags & ~(CLONE_THREAD|CLONE_FS|CLONE_NEWNS|CLONE_SIGHAND|
        CLONE_VM|CLONE_FILES|CLONE_SYSVSEM|
        CLONE_NEWUTS|CLONE_NEWIPC|CLONE_NEWUSER|
        CLONE_NEWNET))
    goto bad_unshare_out;
    ...
}

It is different from flags in __fork_flags.
I think it maybe necessary to add a new function like __unshare_flags in
aux_syscalls.stp.

Best Regards
Zhaolei

----- Original Message ----- 
From: "Mark Wielaard" <mwielaard@redhat.com>
To: "Zhaolei" <zhaolei@cn.fujitsu.com>
Cc: <systemtap@sourceware.org>
Sent: Tuesday, June 03, 2008 3:29 PM
Subject: Re: [PATCH]Add lacked syscall.unshare to tapset


> Hi Zhaolei,
> 
> On Tue, 2008-06-03 at 13:43 +0800, Zhaolei wrote:
>> Following patch adds support for lacked syscall.unshare.
>> If no objection, I will commit it.
>>
>> +# unshare ____________________________________________________
>> +# new function with 2.6.16
>> +# long sys_unshare(unsigned long unshare_flags)
>> +probe syscall.unshare = kernel.function("sys_unshare") ? {
>> + name = "unshare"
>> + unshare_flags = $unshare_flags
>> + argstr = sprintf("%d", unshare_flags)
>> +}
> 
> Are these flags the same as one would use for clone/fork?
> In that case you might want to use
>  argstr = __fork_flags(unshare_flags)
> __fork_flags() is defined in aux_syscalls.stp.
> 
> Cheers,
> 
> Mark
> 
> 
>

Re: [PATCH]Add lacked syscall.unshare to tapset

[Mark Wielaard]

Hi Zhaolei,

On Tue, 2008-06-03 at 16:14 +0800, Zhaolei wrote:
> Thank you for your advice.
> It is a good idea to set flags as string in argstr.
> 
> In my opinion, sys_unshare support only following flags:
> 
> asmlinkage long sys_unshare(unsigned long unshare_flags)
> {
>     ...
>     if (unshare_flags & ~(CLONE_THREAD|CLONE_FS|CLONE_NEWNS|CLONE_SIGHAND|
>         CLONE_VM|CLONE_FILES|CLONE_SYSVSEM|
>         CLONE_NEWUTS|CLONE_NEWIPC|CLONE_NEWUSER|
>         CLONE_NEWNET))
>     goto bad_unshare_out;
>     ...
> }
> 
> It is different from flags in __fork_flags.
> I think it maybe necessary to add a new function like __unshare_flags in
> aux_syscalls.stp.

If I read that fragment above correctly then not all the same flags are
supported, but you can still call unshare with them. The result will
just be that the syscall fails (because it doesn't support that
particular flag). So I don't think you need new function.

Cheers,

Mark

Re: [PATCH]Add lacked syscall.unshare to tapset

[Zhaolei]

Hello, Mark

>> It is different from flags in __fork_flags.
>> I think it maybe necessary to add a new function like __unshare_flags in
>> aux_syscalls.stp.
> 
> If I read that fragment above correctly then not all the same flags are
> supported, but you can still call unshare with them. The result will
> just be that the syscall fails (because it doesn't support that
> particular flag). So I don't think you need new function.
I checked the difference between __fork_flags and sys_unshare.
Nearly all unshare flags are supported by __fork_flags except CLONE_NEWUTS
and CLONE_NEWNET. They should be __fork_flags too.

What if I add CLONE_NEWUTS and CLONE_NEWNET into __fork_flags function and
use __fork_flags in probe syscall.unshare?

Best Regards
Zhaolei

Re: [PATCH]Add lacked syscall.unshare to tapset

[Mark Wielaard]

Hi Zhaolei,

On Tue, 2008-06-03 at 16:46 +0800, Zhaolei wrote:
> I checked the difference between __fork_flags and sys_unshare.
> Nearly all unshare flags are supported by __fork_flags except CLONE_NEWUTS
> and CLONE_NEWNET. They should be __fork_flags too.
> 
> What if I add CLONE_NEWUTS and CLONE_NEWNET into __fork_flags function and
> use __fork_flags in probe syscall.unshare?

That sounds like a good plan to me.

Thanks,

Mark

Re: stack_used() not accurate?

[Frank Ch. Eigler]

Jim Keniston <jkenisto@us.ibm.com> writes:

> [...]  But keep in mind that on i386, when your breakpoint trap
> happens in kernel code, esp and ss aren't saved on the stack.  So
> regs->esp and regs->ss contain the top of the pre-trap stack, and
> the pre-trap stack pointer is &regs->esp, not regs->esp.

We should hide away this peculiarity from tapsets / the runtime.

- FChE

Re: stack_used() not accurate?

[Masami Hiramatsu]

Hi Jim,

Jim Keniston wrote:
> Sorry, I haven't been following this thread for a while, so maybe this
> has already been mentioned.  But keep in mind that on i386, when your
> breakpoint trap happens in kernel code, esp and ss aren't saved on the
> stack.  So regs->esp and regs->ss contain the top of the pre-trap stack,
> and the pre-trap stack pointer is &regs->esp, not regs->esp.

That's right. However, if so, REG_SP(CONTEXT->regs) on x86 should
return &CONTEXT->regs->esp, because the macro means "return the value
of the stack pointer register". (and it must be prohibited to set
a value to SP on x86; it must break the content on the top of stack)

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: stack_used() not accurate?

[Jim Keniston]

On Tue, 2008-06-03 at 11:01 -0400, Masami Hiramatsu wrote:
> Hi Jim,
> 
> Jim Keniston wrote:
> > Sorry, I haven't been following this thread for a while, so maybe this
> > has already been mentioned.  But keep in mind that on i386, when your
> > breakpoint trap happens in kernel code, esp and ss aren't saved on the
> > stack.  So regs->esp and regs->ss contain the top of the pre-trap stack,
> > and the pre-trap stack pointer is &regs->esp, not regs->esp.
> 
> That's right. However, if so, REG_SP(CONTEXT->regs) on x86 should
> return &CONTEXT->regs->esp, because the macro means "return the value
> of the stack pointer register".

That's certainly a reasonable interpretation of what REG_SP is supposed
to mean.  Another reasonable interpretation is "the stack-pointer member
of pt_regs."  The latter interpretation actually has more subscribers in
the SystemTap source, as indicated by the use of &REG_SP(regs) in
stack-*.c.

Of course, REG_SP isn't used much at all (and as far as I know it's not
documented anywhere), so it shouldn't be hard to clear this up.  The
&REG_SP(regs) uses are in arch-specific code, so they could be changed
to &regs->something.

Anybody object to adopting Masami's interpretation?

Jim

Re: stack_used() not accurate?

[Frank Ch. Eigler]

Jim Keniston <jkenisto@us.ibm.com> writes:

> That's certainly a reasonable interpretation of what REG_SP is supposed
> to mean.

This makes sense to me.

> Another reasonable interpretation is "the stack-pointer member
> of pt_regs."  

If they knew the member per se, they could just use it.

> The latter interpretation actually has more subscribers in the
> SystemTap source, as indicated by the use of &REG_SP(regs) in
> stack-*.c.

I don't understand these.  Do ia64, ppc64, and s390 have the same
peculiarity as x86 in the pt_regs->*sp not actually containing the
trap-time stack pointer value?  Or is it just a coincidence that it
works, as in having the pt_regs structure itself be physically
allocated/placed on the probe context stack, so that the address
of any field will point somewhere into the stack?

- FChE

Re: stack_used() not accurate?

[Roland McGrath]

> > Another reasonable interpretation is "the stack-pointer member
> > of pt_regs."  
> 
> If they knew the member per se, they could just use it.

The issue is version skew on x86 where it's esp/rsp/sp.

Re: stack_used() not accurate?

[Masami Hiramatsu]

Roland McGrath wrote:
>>> Another reasonable interpretation is "the stack-pointer member
>>> of pt_regs."  
>> If they knew the member per se, they could just use it.
> 
> The issue is version skew on x86 where it's esp/rsp/sp.

Even though, why does someone want to access regs->esp itself
instead of &regs->esp?
IMHO, if users always access it with '&' on specific arches
and always do it without '&' on other arches, it is helpful that
the macro hides those difference.

Thanks,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com