Re: Fixed PR13146 by not allowing memory allocations to sleep (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-1.6-151-g8e794e9)

Re: Fixed PR13146 by not allowing memory allocations to sleep (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-1.6-151-g8e794e9)

[Mark Wielaard]

Hi David,

On Thu, 2011-09-01 at 14:39 +0000, dsmith@sourceware.org wrote:
> commit 8e794e9dfeea5a48b84442b92dea6c17b9ef75d1
> Author: David Smith <dsmith@redhat.com>
> Date:   Thu Sep 1 09:39:15 2011 -0500
> 
>     Fixed PR13146 by not allowing memory allocations to sleep.
>     
>     * runtime/alloc.c:  Turn off __GFP_WAIT when allocating memory.
>     * runtime/transport/ring_buffer.c (__stp_alloc_ring_buffer): Ditto.
>     * runtime/counter.c (_stp_counter_init): Handles memory allocation failure
>       better.
>     * runtime/task_finder.c (__stp_call_mmap_callbacks_with_addr): Better
>       cleanup if memory allocation fails.

Sorry for the late reply. But I am suspecting this commit of causing
accasional (though not very frequent) test failures that say:
  ERROR: context (size 8352) allocation failed

I suspect that is caused because the context is one of the larger
allocations at systemtap_module_init time. And because we are no longer
allowed to GFP_WAIT it is more likely to fail now. Would it be
possible/make sense to allow GFP_WAIT for allocations made from context
like systemtap_module_init() that may sleep because they are made from
user context?

Thanks,

Mark

Re: Fixed PR13146 by not allowing memory allocations to sleep (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-1.6-151-g8e794e9)

[Josh Stone]

On 09/27/2011 07:52 AM, Mark Wielaard wrote:
> Hi David,
> 
> On Thu, 2011-09-01 at 14:39 +0000, dsmith@sourceware.org wrote:
>> commit 8e794e9dfeea5a48b84442b92dea6c17b9ef75d1
>> Author: David Smith <dsmith@redhat.com>
>> Date:   Thu Sep 1 09:39:15 2011 -0500
>>
>>     Fixed PR13146 by not allowing memory allocations to sleep.
>>     
>>     * runtime/alloc.c:  Turn off __GFP_WAIT when allocating memory.
>>     * runtime/transport/ring_buffer.c (__stp_alloc_ring_buffer): Ditto.
>>     * runtime/counter.c (_stp_counter_init): Handles memory allocation failure
>>       better.
>>     * runtime/task_finder.c (__stp_call_mmap_callbacks_with_addr): Better
>>       cleanup if memory allocation fails.
> 
> Sorry for the late reply. But I am suspecting this commit of causing
> accasional (though not very frequent) test failures that say:
>   ERROR: context (size 8352) allocation failed
> 
> I suspect that is caused because the context is one of the larger
> allocations at systemtap_module_init time. And because we are no longer
> allowed to GFP_WAIT it is more likely to fail now. Would it be
> possible/make sense to allow GFP_WAIT for allocations made from context
> like systemtap_module_init() that may sleep because they are made from
> user context?

I agree, those contexts which can sleep, should.  Not only does this
make it more likely we'll get the memory we want, but also makes us
better citizens with the rest of the kernel.

Josh

Re: Fixed PR13146 by not allowing memory allocations to sleep (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-1.6-151-g8e794e9)

[Jim Keniston]

On Tue, 2011-09-27 at 15:03 -0700, Josh Stone wrote:
> On 09/27/2011 07:52 AM, Mark Wielaard wrote:
> > Hi David,
> > 
> > On Thu, 2011-09-01 at 14:39 +0000, dsmith@sourceware.org wrote:
> >> commit 8e794e9dfeea5a48b84442b92dea6c17b9ef75d1
> >> Author: David Smith <dsmith@redhat.com>
> >> Date:   Thu Sep 1 09:39:15 2011 -0500
> >>
> >>     Fixed PR13146 by not allowing memory allocations to sleep.
> >>     
> >>     * runtime/alloc.c:  Turn off __GFP_WAIT when allocating memory.
> >>     * runtime/transport/ring_buffer.c (__stp_alloc_ring_buffer): Ditto.
> >>     * runtime/counter.c (_stp_counter_init): Handles memory allocation failure
> >>       better.
> >>     * runtime/task_finder.c (__stp_call_mmap_callbacks_with_addr): Better
> >>       cleanup if memory allocation fails.
> > 
> > Sorry for the late reply. But I am suspecting this commit of causing
> > accasional (though not very frequent) test failures that say:
> >   ERROR: context (size 8352) allocation failed
> > 
> > I suspect that is caused because the context is one of the larger
> > allocations at systemtap_module_init time. And because we are no longer
> > allowed to GFP_WAIT it is more likely to fail now. Would it be
> > possible/make sense to allow GFP_WAIT for allocations made from context
> > like systemtap_module_init() that may sleep because they are made from
> > user context?
> 
> I agree, those contexts which can sleep, should.  Not only does this
> make it more likely we'll get the memory we want, but also makes us
> better citizens with the rest of the kernel.
> 
> Josh
> 

I haven't seen this explicitly mentioned wrt this thread or PR13146, but
uprobes and uretprobe handlers (which are called from the utrace
report_signal callback) can sleep.

Jim

Re: Fixed PR13146 by not allowing memory allocations to sleep (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-1.6-151-g8e794e9)

[Mark Wielaard]

On Tue, 2011-09-27 at 16:00 -0700, Jim Keniston wrote:
> On Tue, 2011-09-27 at 15:03 -0700, Josh Stone wrote:
> > On 09/27/2011 07:52 AM, Mark Wielaard wrote:
> > > I suspect that is caused because the context is one of the larger
> > > allocations at systemtap_module_init time. And because we are no longer
> > > allowed to GFP_WAIT it is more likely to fail now. Would it be
> > > possible/make sense to allow GFP_WAIT for allocations made from context
> > > like systemtap_module_init() that may sleep because they are made from
> > > user context?
> > 
> > I agree, those contexts which can sleep, should.  Not only does this
> > make it more likely we'll get the memory we want, but also makes us
> > better citizens with the rest of the kernel.
> > 
> I haven't seen this explicitly mentioned wrt this thread or PR13146, but
> uprobes and uretprobe handlers (which are called from the utrace
> report_signal callback) can sleep.

OK, I have not gone through all allocation points yet. But I did
introduce _gfp variants for the current _stp alloc.c functions. And use
them with GFP_KERNEL from module_init for the setup of the context, maps
and stats now. Since that is where I saw the spurious failures most
often and because that was easiest to proof "safe" (but please do double
check the commit, in case I did something stupid). Tested against
2.6.35.14-96.fc14.x86_64 and 2.6.9-101.ELsmp i686.

commit f816f9be4cdf8e4dcac0b11fad8924ab389449ee
Author: Mark Wielaard <mjw@redhat.com>
Date:   Wed Sep 28 13:18:18 2011 +0200

    Introduce gfp_mask variants of _stp allocation functions.
    
    * runtime/alloc.c: Add _stp_kmalloc_gfp(), _stp_kzalloc_gfp() and
      _stp_kmalloc_node_gfp() which take a gfp_mask in case you really
      know what you are doing.
    * translate.cxx (emit_module_init): Use with GFP_KERNEL for context
      alloc.
    * runtime/map.c: Use for init and new functions called at
      module_init time.
    * runtime/stat.c: Likewise.

We could now use that in other places if we really know it is safe.

Note that_stp_alloc_percpu() calls __alloc_percpu() which may sleep and
always uses GFP_KERNEL. I audited the places where we call it and they
all seem safe, but please double check current practice and be careful
when calling _stp_alloc_percpu() from random contexts.

BTW. Why is our default gfp_mask:
#define STP_ALLOC_FLAGS ((GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN) \
                         & ~__GFP_WAIT)
And not just GFP_ATOMIC or GFP_NOWAIT?

Cheers,

Mark

Re: Fixed PR13146 by not allowing memory allocations to sleep (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-1.6-151-g8e794e9)

[David Smith]

On 09/28/2011 07:11 AM, Mark Wielaard wrote:

> On Tue, 2011-09-27 at 16:00 -0700, Jim Keniston wrote:
>> On Tue, 2011-09-27 at 15:03 -0700, Josh Stone wrote:
>>> On 09/27/2011 07:52 AM, Mark Wielaard wrote:
>>>> I suspect that is caused because the context is one of the larger
>>>> allocations at systemtap_module_init time. And because we are no longer
>>>> allowed to GFP_WAIT it is more likely to fail now. Would it be
>>>> possible/make sense to allow GFP_WAIT for allocations made from context
>>>> like systemtap_module_init() that may sleep because they are made from
>>>> user context?
>>>
>>> I agree, those contexts which can sleep, should.  Not only does this
>>> make it more likely we'll get the memory we want, but also makes us
>>> better citizens with the rest of the kernel.
>>>
>> I haven't seen this explicitly mentioned wrt this thread or PR13146, but
>> uprobes and uretprobe handlers (which are called from the utrace
>> report_signal callback) can sleep.
> 
> OK, I have not gone through all allocation points yet. But I did
> introduce _gfp variants for the current _stp alloc.c functions. And use
> them with GFP_KERNEL from module_init for the setup of the context, maps
> and stats now. Since that is where I saw the spurious failures most
> often and because that was easiest to proof "safe" (but please do double
> check the commit, in case I did something stupid). Tested against
> 2.6.35.14-96.fc14.x86_64 and 2.6.9-101.ELsmp i686.
> 
> commit f816f9be4cdf8e4dcac0b11fad8924ab389449ee
> Author: Mark Wielaard <mjw@redhat.com>
> Date:   Wed Sep 28 13:18:18 2011 +0200
> 
>     Introduce gfp_mask variants of _stp allocation functions.
>     
>     * runtime/alloc.c: Add _stp_kmalloc_gfp(), _stp_kzalloc_gfp() and
>       _stp_kmalloc_node_gfp() which take a gfp_mask in case you really
>       know what you are doing.
>     * translate.cxx (emit_module_init): Use with GFP_KERNEL for context
>       alloc.
>     * runtime/map.c: Use for init and new functions called at
>       module_init time.
>     * runtime/stat.c: Likewise.
> 
> We could now use that in other places if we really know it is safe.
> 
> Note that_stp_alloc_percpu() calls __alloc_percpu() which may sleep and
> always uses GFP_KERNEL. I audited the places where we call it and they
> all seem safe, but please double check current practice and be careful
> when calling _stp_alloc_percpu() from random contexts.

Mark, thanks for finding and fixing this.  I started seeing some of
those messages right before I left for vacation and didn't have time to
look into it.

> BTW. Why is our default gfp_mask:
> #define STP_ALLOC_FLAGS ((GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN) \
>                          & ~__GFP_WAIT)
> And not just GFP_ATOMIC or GFP_NOWAIT?


Good question.  I added the '& ~__GFP_WAIT' part in the commit that
started this, and I was unsure of the logic of the rest.  I didn't have
time to look too much in the history of those flags and how we used
them.  It is possible the rest of those flags were done that way for
reasons that made sense back in the early 2.6 kernel days.

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

Re: Fixed PR13146 by not allowing memory allocations to sleep (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-1.6-151-g8e794e9)

[David Smith]

On 09/27/2011 06:00 PM, Jim Keniston wrote:

> I haven't seen this explicitly mentioned wrt this thread or PR13146, but
> uprobes and uretprobe handlers (which are called from the utrace
> report_signal callback) can sleep.


Jim,

For my information, can uprobe/uretprobe handlers sleep in the new
uprobes being proposed upstream?

(Note that in the new utrace-less task_finder I'm working on, handlers
can't sleep since they get called from a tracepoint, whose handlers
can't sleep.)

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

Re: Fixed PR13146 by not allowing memory allocations to sleep (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-1.6-151-g8e794e9)

[Jim Keniston]

On Wed, 2011-10-05 at 08:46 -0500, David Smith wrote:
> On 09/27/2011 06:00 PM, Jim Keniston wrote:
> 
> > I haven't seen this explicitly mentioned wrt this thread or PR13146, but
> > uprobes and uretprobe handlers (which are called from the utrace
> > report_signal callback) can sleep.
> 
> 
> Jim,
> 
> For my information, can uprobe/uretprobe handlers sleep in the new
> uprobes being proposed upstream?

I think yes.  But sorry, I've been out of the loop on Srikar's uprobes
implementation for quite a while, so you'll have to ask him.

Wrt uretprobes, I've seen a draft implementation a while back, but I
haven't noticed it being posted to LKML.

> (Note that in the new utrace-less task_finder I'm working on, handlers
> can't sleep since they get called from a tracepoint, whose handlers
> can't sleep.)
> 

Jim

Re: Fixed PR13146 by not allowing memory allocations to sleep (Was: [SCM] systemtap: system-wide probe/trace tool branch, master, updated. release-1.6-151-g8e794e9)

[Ananth N Mavinakayanahalli]

On Wed, Oct 05, 2011 at 08:46:43AM -0500, David Smith wrote:
> On 09/27/2011 06:00 PM, Jim Keniston wrote:
> 
> > I haven't seen this explicitly mentioned wrt this thread or PR13146, but
> > uprobes and uretprobe handlers (which are called from the utrace
> > report_signal callback) can sleep.
> 
> 
> Jim,
> 
> For my information, can uprobe/uretprobe handlers sleep in the new
> uprobes being proposed upstream?

Yes, they can sleep. The handlers get called on the path back to
userspace (do_notify_resume()) similar to what utrace used to do.

Ananth

Re: Fixed PR13146 by not allowing memory allocations to sleep

[Frank Ch. Eigler]

jistone wrote:

> [...]
> I agree, those contexts which can sleep, should.  Not only does this
> make it more likely we'll get the memory we want, but also makes us
> better citizens with the rest of the kernel.

Unfortunately, that's not quite sound policy either.  The memory
allocation aggressiveness pendulum has swung too far with the new
code, and now large data structures are allocated with plain
GFP_KERNEL.  On small-memory machines, this is found to OOM the
system, rather than let the stap module give up early.

An intermediate approach is needed; maybe __GFP_REPEAT & !__GFP_NOWAIT.

- FChE

Re: Fixed PR13146 by not allowing memory allocations to sleep

[Mark Wielaard]

On Tue, 2011-10-25 at 08:06 -0400, Frank Ch. Eigler wrote:
> jistone wrote:
> 
> > [...]
> > I agree, those contexts which can sleep, should.  Not only does this
> > make it more likely we'll get the memory we want, but also makes us
> > better citizens with the rest of the kernel.
> 
> Unfortunately, that's not quite sound policy either.  The memory
> allocation aggressiveness pendulum has swung too far with the new
> code, and now large data structures are allocated with plain
> GFP_KERNEL.  On small-memory machines, this is found to OOM the
> system, rather than let the stap module give up early.
> 
> An intermediate approach is needed; maybe __GFP_REPEAT & !__GFP_NOWAIT.

Probably the best approach is the introduce a specific
STP_ALLOC_FLAGS_WAIT, STP_ALLOC_FLAGS_NOWAIT in runtime/alloc.c to
replace the generic STP_ALLOC_FLAGS and GFP_KERNEL we are now using. The
first would be for "normal" allocations, which may wait, can fail, etc
(just don't trigger the oom-killer) and the later would be for critical
allocations in context that cannot wait.

I cannot find what precisely triggers/causes the oom-killer to kick in.
I hoped a GFP_WILLING_TO_WAIT_BUT_DONT_KILL_ANYBODY_JUST_BECAUSE_OF_ME
flag would exist, but it isn't clear to me which GFP flag/combination
that actually corresponds to.

Cheers,

Mark

Re: Fixed PR13146 by not allowing memory allocations to sleep

[Mark Wielaard]

On Tue, 2011-10-25 at 14:18 +0200, Mark Wielaard wrote:
> I cannot find what precisely triggers/causes the oom-killer to kick in.
> I hoped a GFP_WILLING_TO_WAIT_BUT_DONT_KILL_ANYBODY_JUST_BECAUSE_OF_ME
> flag would exist, but it isn't clear to me which GFP flag/combination
> that actually corresponds to.

Looking at the kernel sources it seems that flag is __GFP_NORETRY.
Though that isn't a guarantee, it depends on the memory backend. Still
it seems to prevent the oom-killer being triggered on my small memory
setup. So I created STP_ALLOC_SLEEP_FLAGS that includes that flag and
use that whenever we do _stp_alloc request from user context. Note that
this isn't a full "solution" to the oom-killer kicking in. There are
places where GFP_KERNEL is used that we don't control.

commit 6022ccd4cab89944ca390c23de13cdc54cf05b1f
Author: Mark Wielaard <mjw@redhat.com>
Date:   Mon Oct 31 10:39:10 2011 +0100

Introduce and use STP_ALLOC_SLEEP_FLAGS for use with gfp alloc
functions.
    
When in a context where we can wait we like to use normal GFP_KERNEL.
But this might trigger the oom-killer, which might kill out own stapio
process. To suppress this use __GFP_NORETRY. All _stp_[kmz]alloc_gfp
functions that are used in user context should use STP_ALLOC_SLEEP_FLAGS
if appropriate.
    
Note that the kernel itself may still use GFP_KERNEL. For example
__alloc_percpu() will always use GFP_KERNEL.

Re: Fixed PR13146 by not allowing memory allocations to sleep

[Frank Ch. Eigler]

mjw wrote:

> [...]
>> I cannot find what precisely triggers/causes the oom-killer to kick in.
>> I hoped a GFP_WILLING_TO_WAIT_BUT_DONT_KILL_ANYBODY_JUST_BECAUSE_OF_ME
>> flag would exist, but it isn't clear to me which GFP flag/combination
>> that actually corresponds to.
>
> Looking at the kernel sources it seems that flag is __GFP_NORETRY.
> [...]

We used to use __GFP_NORETRY for e.g. map allocation, since commit
132c23b4 some years ago.  So I think this just swings our pendulum
back to a previous point.

- FChE

Re: Fixed PR13146 by not allowing memory allocations to sleep

[Mark Wielaard]

On Mon, 2011-10-31 at 11:19 -0400, Frank Ch. Eigler wrote:
> mjw wrote:
> 
> > [...]
> >> I cannot find what precisely triggers/causes the oom-killer to kick in.
> >> I hoped a GFP_WILLING_TO_WAIT_BUT_DONT_KILL_ANYBODY_JUST_BECAUSE_OF_ME
> >> flag would exist, but it isn't clear to me which GFP flag/combination
> >> that actually corresponds to.
> >
> > Looking at the kernel sources it seems that flag is __GFP_NORETRY.
> > [...]
> 
> We used to use __GFP_NORETRY for e.g. map allocation, since commit
> 132c23b4 some years ago.  So I think this just swings our pendulum
> back to a previous point.

Ha good. The pendulum swung back to the middle again you mean?
The thing that triggered all this was that the previous commit added
~__GFP_WAIT which triggered all those failures for allocations that were
fine with waiting a bit because they were done in user context.

I am happy with the current solution. But maybe there are other people
now not happy again?

Cheers,

Mark