Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1160712 > unrolled thread

Re: [PATCH] oom: split out forced OOM killer

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] oom: split out forced OOM killer Austin S Hemmelgarn <ahferroin7@gmail.com> - 2015-06-08 21:00 +0200
    Re: [PATCH] oom: split out forced OOM killer David Rientjes <rientjes@google.com> - 2015-06-08 21:50 +0200

#1160712 — Re: [PATCH] oom: split out forced OOM killer

[Multipart message — attachments visible in raw view] — view raw

On 2015-06-08 13:59, David Rientjes wrote:
> On Fri, 5 Jun 2015, Austin S Hemmelgarn wrote:
>
>>> I'm not sure what the benefit of this is, and it's adding more code.
>>> Having multiple pathways and requirements, such as constrained_alloc(), to
>>> oom kill a process isn't any clearer, in my opinion.  It also isn't
>>> intended to be optimized since the oom killer called from the page
>>> allocator and from sysrq aren't fastpaths.  To me, this seems like only a
>>> source code level change and doesn't make anything more clear but rather
>>> adds more code and obfuscates the entry path.
>>
>> At the very least, it does make the semantics of sysrq-f much nicer for admins
>> (especially the bit where it ignores the panic_on_oom setting, if the admin
>> wants the system to panic, he'll use sysrq-c).  There have been times I've had
>> to hit sysrq-f multiple times to get to actually kill anything, and this looks
>> to me like it would eliminate that rather annoying issue as well.
>>
>
> Are you saying there's a functional change with this patch/
>
I believe so (haven't actually read the patch itself, just the 
changelog), although it is only a change for certain configurations to a 
very specific and (I hope infrequently) used piece of functionality. 
Like I said above, if I wanted to crash my system, I'd be using sysrq-c; 
and if I'm using sysrq-f, I want _some_ task to die _now_.

[toc] | [next] | [standalone]

#1160742

On Mon, 8 Jun 2015, Austin S Hemmelgarn wrote:

> I believe so (haven't actually read the patch itself, just the changelog),
> although it is only a change for certain configurations to a very specific and
> (I hope infrequently) used piece of functionality. Like I said above, if I
> wanted to crash my system, I'd be using sysrq-c; and if I'm using sysrq-f, I
> want _some_ task to die _now_.
> 

This patch is not a functional change, so I don't interpret your feedback 
as any support of it being merged.

That said, you raise an interesting point of whether sysrq+f should ever 
trigger a panic due to panic_on_oom.  The case can be made that it should 
ignore panic_on_oom and require the use of another sysrq to panic the 
machine instead.  Sysrq+f could then be used to oom kill a process, 
regardless of panic_on_oom, and the panic only occurs if userspace did not 
trigger the kill or the kill itself will fail.

I think we should pursue that direction.

This patch also changes the text which is output to the kernel log on 
panic, which we use to parse for machines that have crashed due to no 
killable memcg processes, so NACK on this patch.  There's also no reason 
to add more source code to try to make things cleaner when it just 
obfuscates the oom killer code more than it needs to (we don't need to 
optimize or have multiple entry points).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web