linux-kernel - Re: [PATCH 3/3] sched/fair: schedutil: explicit update only when required

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20180524134236.GA30654@e110439-lin>
Date:   Thu, 24 May 2018 14:42:36 +0100
From:   Patrick Bellasi <patrick.bellasi@....com>
To:     Joel Fernandes <joel@...lfernandes.org>
Cc:     linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Morten Rasmussen <morten.rasmussen@....com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Joel Fernandes <joelaf@...gle.com>,
        Todd Kjos <tkjos@...gle.com>, kernel-team@...roid.com,
        Steve Muckle <smuckle@...gle.com>
Subject: Re: [PATCH 3/3] sched/fair: schedutil: explicit update only when
 required

Hi Joel,
sorry for the late reply, this thread is a bit confusing because we
keep discussing while there was already a v2 posted on list.

However, here are few comments below...

[...]

> > > > > @@ -5456,10 +5443,12 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
> > > > >  		update_cfs_group(se);
> > > > >  	}
> > > > >  
> > > > > +	/* The task is no more visible from the root cfs_rq */
> > > > >  	if (!se)
> > > > >  		sub_nr_running(rq, 1);
> > > > >  
> > > > >  	util_est_dequeue(&rq->cfs, p, task_sleep);
> > > > > +	cpufreq_update_util(rq, 0);
> > > > 
> > > > One question about this change. In enqueue, throttle and unthrottle - you are
> > > > conditionally calling cpufreq_update_util incase the task was
> > > > visible/not-visible in the hierarchy.
> > > > 
> > > > But in dequeue you're unconditionally calling it. Seems a bit inconsistent.
> > > > Is this because of util_est or something? Could you add a comment here
> > > > explaining why this is so?
> > > 
> > > The big question I have is incase se != NULL, then its still visible at the
> > > root RQ level.
> > 
> > My understanding it that you get !se at dequeue time when we are
> > dequeuing a task from a throttled RQ. Isn't it?
> 
> I don't think so? !se means the RQ is not throttled.

Yes, I agree, I "just" forgot a "not" in the sentence above... my bad!

However, we are on the same page here.
 
> > Thus, this means you are dequeuing a throttled task, I guess for
> > example because of a migration.
> > However, the point is that a task dequeue from a throttled RQ _is
> > already_ not visible from the root RQ, because of the sub_nr_running()
> > done by throttle_cfs_rq().
> 
> Yes that's what I was wondering, so my point was if its already not visible,
> then why call schedutil. I felt call schedutil only if its visible like you
> were doing for the other paths.

Agree, as discussed in Vincent in v2, we should likely move these
schedutil updates at attach/detach time. This is when exectly we know
that the utilization has changed for a CPU.

... and that's what I'll propose in the upcoming v3 for this patch.

[...]

> I agree with your assessments below and about not calling cpufreq
> when CPU is about to idle.

Cool ;)

-- 
#include <best/regards.h>

Patrick Bellasi