From: Tejun Heo <tj@kernel.org>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Roman Gushchin <guro@fb.com>, Roman Gushchin <guroan@gmail.com>,
Kernel Team <Kernel-team@fb.com>,
"cgroups@vger.kernel.org" <cgroups@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v8 0/7] freezer for cgroup v2
Date: Thu, 21 Feb 2019 09:34:22 -0800 [thread overview]
Message-ID: <20190221173422.GY50184@devbig004.ftw2.facebook.com> (raw)
In-Reply-To: <20190221162923.GA26064@redhat.com>
Hello, Oleg.
On Thu, Feb 21, 2019 at 05:29:24PM +0100, Oleg Nesterov wrote:
> But to me this is a reasonable trade-off because this way we do not add
> additional complexity to the kernel.
So, I really wanna avoid allowing userspace to cause D state sleeps.
It's not impossible to work around but becomes really nasty. For
example, imagine a memory pressure based userspace oom handler issuer
kills based on per-cgroup pressure metric (as oomd would do). It
might not necessarily have the insight that a victim cgroup is frozen
or whether it can move out its members to a different cgroup (in a lot
of cases that will cause a lot of confusion in management software).
The frozen state in cgroup1 is a new task state which is different
from all others and in a nasty way and it has been causing various
confusions and mistakes from its users. We really should make it
closer to the existing stop behaviors even if that means more
complexity in the implementation.
> Actually, "killable" is not that difficult afaics. "ptraceable" looks more
> problematic to me. Again, user-space can do
>
> 1. PTRACE_SEIZE
> 2. move the tracee to the root cgroup
> 3. do anything with the tracee
> 4. move it back
which is fine. The goal isn't trying to block userspace from doing
things that it explicitly wants to do. That's fine. We just want
things like killing and ptracing to behave similarly to other stopped
states (ie. avoid introducing completely new behaviors).
> But there is another case. If admin wants to freeze a cgroup then it is not
> clear why a user which can send SIGKILL to a frozen process should wake it up.
>
> ------------------------------------------------------------------------------
> Again, it is not that I hate the idea of killable/ptraceable freezer. Just I
> personally think it's not worth the trouble. Perhaps I am wrong, but so far
> I do not see a good implementation...
>
> And, apart from reading/writing the registers, what can ptrace do with a frozen
> tracee? This doesn't look like a "must have" feature to me.
>
> At least, may I ask you again to make (if possible) a separate patch which adds
> the ability to kill/ptrace?
ptrace support is a lot less important than kill for sure but if at
all possible I think it'd be better to have it because that makes the
frozen state closer to other stopped states and thus less surprising.
To summarize, the ideal result is the frozen state to be "stuck in
jobctl stop loop" but except for looping in it no different from
regular jobctl stop. Plus, register state examination is already
useful for frozen cgroups for debbugging purposes in itself.
Thanks.
--
tejun
next prev parent reply other threads:[~2019-02-21 17:34 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-19 22:02 [PATCH v8 0/7] freezer for cgroup v2 Roman Gushchin
2019-02-19 22:02 ` [PATCH v8 1/7] cgroup: rename freezer.c into legacy_freezer.c Roman Gushchin
2019-02-19 22:02 ` [PATCH v8 2/7] cgroup: implement __cgroup_task_count() helper Roman Gushchin
2019-02-19 22:02 ` [PATCH v8 3/7] cgroup: protect cgroup->nr_(dying_)descendants by css_set_lock Roman Gushchin
2019-02-19 22:02 ` [PATCH v8 4/7] cgroup: cgroup v2 freezer Roman Gushchin
2019-02-20 14:42 ` Oleg Nesterov
2019-02-20 22:14 ` Roman Gushchin
2019-02-21 16:44 ` Oleg Nesterov
2019-02-19 22:02 ` [PATCH v8 5/7] kselftests: cgroup: don't fail on cg_kill_all() error in cg_destroy() Roman Gushchin
2019-02-19 22:02 ` [PATCH v8 6/7] kselftests: cgroup: add freezer controller self-tests Roman Gushchin
2019-02-19 22:02 ` [PATCH v8 7/7] cgroup: document cgroup v2 freezer interface Roman Gushchin
2019-02-20 14:37 ` [PATCH v8 0/7] freezer for cgroup v2 Oleg Nesterov
2019-02-20 22:00 ` Roman Gushchin
2019-02-21 16:29 ` Oleg Nesterov
2019-02-21 17:34 ` Tejun Heo [this message]
2019-02-22 16:34 ` Oleg Nesterov
2019-02-22 18:17 ` Tejun Heo
2019-02-25 15:57 ` Oleg Nesterov
2019-03-05 17:27 ` Tejun Heo
2019-02-21 22:43 ` Roman Gushchin
2019-02-22 17:04 ` Oleg Nesterov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190221173422.GY50184@devbig004.ftw2.facebook.com \
--to=tj@kernel.org \
--cc=Kernel-team@fb.com \
--cc=cgroups@vger.kernel.org \
--cc=guro@fb.com \
--cc=guroan@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=oleg@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).