Re: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-1.6-594-g12aa5f9

Re: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-1.6-594-g12aa5f9

[Mark Wielaard]

On Tue, 2011-12-13 at 22:32 +0000, fche@sourceware.org wrote:
> This is an automated email from the git hooks/post-receive script. It was
> generated because a ref change was pushed to the repository containing
> the project "systemtap: system-wide probe/trace tool".
>
> commit 12aa5f9c1d9ac3dc7003f9c0e0b8e34c23d67407
> Author: Frank Ch. Eigler <fche@redhat.com>
> Date:   Tue Dec 13 17:27:38 2011 -0500
> 
>     PR13489/PR11671: more lockdep antihistamines
>     
>     * runtime/task_finder_vma.c (__stp_tf_vma_new_entry): Use
>       STP_ALLOC_FLAGS for new vma entry memory allocation needs.
> 
> diff --git a/runtime/task_finder_vma.c b/runtime/task_finder_vma.c
> index 90a34ba..093d307 100644
> --- a/runtime/task_finder_vma.c
> +++ b/runtime/task_finder_vma.c
> @@ -44,6 +44,8 @@ static struct hlist_head *__stp_tf_vma_map;
>  
>  // __stp_tf_vma_new_entry(): Returns an newly allocated or NULL.
>  // Must only be called from user context.
> +// ... except, with inode-uprobes / task-finder2, it can be called from
> +// random tracepoints.  So we cannot sleep after all.
>  static struct __stp_tf_vma_entry *
>  __stp_tf_vma_new_entry(void)
>  {
> @@ -51,7 +53,7 @@ __stp_tf_vma_new_entry(void)
>         size_t size = sizeof (struct __stp_tf_vma_entry);
>  
>         entry = (struct __stp_tf_vma_entry *) _stp_kmalloc_gfp(size,
> -                                                       STP_ALLOC_SLEEP_FLAGS);
> +                                                               STP_ALLOC_FLAGS);
>         return entry;
>  }

Urgh that hurts. Especially on memory constraint systems having to do
non-sleeping allocations. Isn't there any way we can prevent this? Or at
least detect that we are using a task_finder that doesn't notify us in
user context? I think there are other places that assume the task_finder
only notifies us about updates in user context, for example so we can
check the build-id. Having a taks_finder that only operates in atomic
contexts is pretty limiting.

Cheers,

Mark

Re: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-1.6-594-g12aa5f9

[David Smith]

On 12/14/2011 03:42 AM, Mark Wielaard wrote:

> On Tue, 2011-12-13 at 22:32 +0000, fche@sourceware.org wrote:

...

>>         entry = (struct __stp_tf_vma_entry *) _stp_kmalloc_gfp(size,
>> -                                                       STP_ALLOC_SLEEP_FLAGS);
>> +                                                               STP_ALLOC_FLAGS);
> 
> Urgh that hurts. Especially on memory constraint systems having to do
> non-sleeping allocations. Isn't there any way we can prevent this? Or at
> least detect that we are using a task_finder that doesn't notify us in
> user context? I think there are other places that assume the task_finder
> only notifies us about updates in user context, for example so we can
> check the build-id. Having a taks_finder that only operates in atomic
> contexts is pretty limiting.


We could postpone the pain a bit by changing the above to:

#ifdef CONFIG_UTRACE
         entry = (struct __stp_tf_vma_entry *) _stp_kmalloc_gfp(size,
		STP_ALLOC_SLEEP_FLAGS);
#else
         entry = (struct __stp_tf_vma_entry *) _stp_kmalloc_gfp(size,
		STP_ALLOC_FLAGS);
#endif

This way only systems using the new code use the non-sleepable alloc.

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

Re: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-1.6-594-g12aa5f9

[Mark Wielaard]

On Wed, Dec 14, 2011 at 09:53:58AM -0600, David Smith wrote:
> On 12/14/2011 03:42 AM, Mark Wielaard wrote:
> 
> > On Tue, 2011-12-13 at 22:32 +0000, fche@sourceware.org wrote:
> 
> ...
> 
> >>         entry = (struct __stp_tf_vma_entry *) _stp_kmalloc_gfp(size,
> >> -                                                       STP_ALLOC_SLEEP_FLAGS);
> >> +                                                               STP_ALLOC_FLAGS);
> > 
> > Urgh that hurts. Especially on memory constraint systems having to do
> > non-sleeping allocations. Isn't there any way we can prevent this? Or at
> > least detect that we are using a task_finder that doesn't notify us in
> > user context? I think there are other places that assume the task_finder
> > only notifies us about updates in user context, for example so we can
> > check the build-id. Having a taks_finder that only operates in atomic
> > contexts is pretty limiting.
> 
> 
> We could postpone the pain a bit by changing the above to:
> 
> #ifdef CONFIG_UTRACE
>          entry = (struct __stp_tf_vma_entry *) _stp_kmalloc_gfp(size,
> 		STP_ALLOC_SLEEP_FLAGS);
> #else
>          entry = (struct __stp_tf_vma_entry *) _stp_kmalloc_gfp(size,
> 		STP_ALLOC_FLAGS);
> #endif
> 
> This way only systems using the new code use the non-sleepable alloc.

That seems a nice compromise. I pushed that change. Commit efe1470.

Thanks,

Mark