On Wed, Oct 23, 2019 at 10:18:48AM +0800, Honglei Wang wrote: > > > 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... Hello Honglei! I'm deeply sorry, I've missed your e-mail being thinking about the proposal from Oleg and various edge cases. I don't think saving 50% cpu time on migrating 1000 tasks is important, but not waking tasks without a reason looks as a perfect justification for the patch. Please, fell free to use Acked-by: Roman Gushchin <guro@xxxxxx> after fixing the comment and adding some justification text to the commit message. Thank you! > > 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 > > >
