On Wed, 21 Nov 2012 21:05:20 +0000
Mel Gorman <mgorman@xxxxxxx> wrote:
> On Wed, Nov 21, 2012 at 12:15:59PM -0800, Andrew Morton wrote:
>
> ...
>
> > > -static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
> > > +static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
> > > nodemask_t *nodemask)
> > > {
> > > struct zone *zone;
> > > @@ -2224,13 +2227,20 @@ static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
> > > * processes to block on log_wait_commit().
> > > */
> > > if (current->flags & PF_KTHREAD)
> > > - return;
> > > + goto out;
> >
> > hm, well, back in the old days some kernel threads were killable via
> > signals. They had to opt-in to it by diddling their signal masks and a
> > few other things. Too lazy to check if there are still any such sites.
> >
>
> That check is against throttling rather than signal handling though. It
> could have been just left as "return".
My point is that there might still exist kernel threads which are killable
via signals. Those threads match your criteria here: don't throttle -
just let them run to exit().
If there are indeed missed opportunities here then they will be small
ones. And those threads probably only have signal_pending(), not
fatal_signal_pending(). Don't worry about it ;)
> >
> > > + /*
> > > + * If a fatal signal is pending, this process should not throttle.
> > > + * It should return quickly so it can exit and free its memory
> > > + */
> > > + if (fatal_signal_pending(current))
> > > + goto out;
> >
> > theresabug. It should return "true" here.
> >
>
> The intention here is that a process would
>
> 1. allocate, fail, enter direct reclaim
> 2. no signal pending, gets throttled because of low pfmemalloc reserves
> 3. a user kills -9 the throttled process. returns true and goes back
> to the page allocator
> 4. If that allocation fails again, it re-enters direct reclaim and tries
> to throttle. This time the fatal signal is pending but we know
> we must have already failed to make the allocation so this time false
> is rurned by throttle_direct_reclaim and it tries direct reclaim.
My spinning head fell on the floor and is now drilling its way to China.
> 5. direct reclaim frees something -- probably clean file-backed pages
> if the last allocation attempt had failed.
>
> so the fatal signal check should only prevent entering direct reclaim
> once. Maybe the comment sucks
Well it did say "Returns true if a fatal signal was received during
throttling.". That "during" was subtle.
> /*
> * If a fatal signal is pending, this process should not throttle.
> * It should return quickly so it can exit and free its memory. Note
> * that returning false here allows a process to enter direct reclaim.
> * Otherwise there is a risk that the process loops in the page
> * allocator, checking signals and never making forward progress
> */
>
> ?
It's still unclear why throttle_direct_reclaim() returns false if
fatal_signal_pending() *before* throttling, but true *after* throttling.
Why not always return true and just scram?
>
> ...
>
> Same comment about the potential looping. Otherwise I think it's ok.
Send me something sometime ;)
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>