On 10/22/19 12:14 AM, Roman Gushchin wrote:
On Mon, Oct 21, 2019 at 04:18:26PM +0800, Honglei Wang wrote:
Seems it's not necessary to adjust the task state and revisit the
state of source and destination cgroups if the cgroups are not in
freeze state and the task itself is not frozen.
Signed-off-by: Honglei Wang <honglei.wang@xxxxxxxxxx>
Hello Honglei!
Overall the patch looks correct to me, but what's the goal of this change?
Do you see any performance difference in some tests?
Maybe you can use freezer tests from kselftests to show the difference?
Your patch will sightly change the behavior of the freezer if unfreezing
of a cgroup is racing with the task migration. The change is probably fine
(at least I can't imagine why not), and I'd totally support the patch,
if there is any performance benefit.
Thank you!
Hi Roman,
Thank you for your attention!
When I debug another problem, I just happen add some debug print which
show me there are many tasks be woke up when moving tasks from one
cgroup to another. After a bit more test, I find there are hundreds of
task waking up happen even when the kernel boot up.
All of these tasks are not in running state when they are moved into a
cgroup or moved from one to anther, and the movement itself is not the
signal to wake up these tasks. I feel it's waste that the whole wake-up
process have to be done for the tasks who are not supposed ready to put
into the runqueue...
Then I think further, if somebody want to move huge amount of tasks from
one cgroup to another OR from the child cgroup to its parent before
remove it, more such waste happens. I do a test which move 1000 tasks
from child to parent via a script:
without the code change:
real 0m0.037s
user 0m0.021s
sys 0m0.016s
with the code change:
real 0m0.028s
user 0m0.020s
sys 0m0.008s
it saves 50% time in system part (yes, 0.008s is not a big deal ;)).
Does it make sense to you?
Thank,
Honglei
---
kernel/cgroup/freezer.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/kernel/cgroup/freezer.c b/kernel/cgroup/freezer.c
index 8cf010680678..2dd66551d9a6 100644
--- a/kernel/cgroup/freezer.c
+++ b/kernel/cgroup/freezer.c
@@ -230,6 +230,15 @@ void cgroup_freezer_migrate_task(struct task_struct *task,
if (task->flags & PF_KTHREAD)
return;
+ /*
+ * It's not necessary to do changes if both of the src and dst cgroups
+ * are not freeze and task is not frozen.
^^^
are not freezing?
Will fix it in next version if we think this patch is necessary.
+ */
+ if (!test_bit(CGRP_FREEZE, &src->flags) &&
+ !test_bit(CGRP_FREEZE, &dst->flags) &&
+ !task->frozen)
+ return;
+
/*
* Adjust counters of freezing and frozen tasks.
* Note, that if the task is frozen, but the destination cgroup is not
--
2.17.0
