lkml.org 
[lkml]   [2019]   [Aug]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 13/15] sched,fair: propagate sum_exec_runtime up the hierarchy
    From
    Date
    On Thu, 2019-08-29 at 19:20 +0200, Dietmar Eggemann wrote:
    > On 28/08/2019 15:14, Rik van Riel wrote:
    > > On Wed, 2019-08-28 at 09:51 +0200, Dietmar Eggemann wrote:
    > > > On 22/08/2019 04:17, Rik van Riel wrote:
    > > > > Now that enqueue_task_fair and dequeue_task_fair no longer
    > > > > iterate
    > > > > up
    > > > > the hierarchy all the time, a method to lazily propagate
    > > > > sum_exec_runtime
    > > > > up the hierarchy is necessary.
    > > > >
    > > > > Once a tick, propagate the newly accumulated exec_runtime up
    > > > > the
    > > > > hierarchy,
    > > > > and feed it into CFS bandwidth control.
    > > > >
    > > > > Remove the pointless call to account_cfs_rq_runtime from
    > > > > update_curr,
    > > > > which is always called with a root cfs_rq.
    > > >
    > > > But what about the call to account_cfs_rq_runtime() in
    > > > set_curr_task_fair()? Here you always call it with the root
    > > > cfs_rq.
    > > > Shouldn't this be called also in a loop over all se's until !se-
    > > > > parent
    > > > (like in propagate_exec_runtime() further below).
    > >
    > > I believe that call should be only on the cgroup
    > > cfs_rq, with account_cfs_rq_runtime figuring out
    > > whether more runtime needs to be obtained from
    > > further up in the hierarchy.
    >
    > So like this?
    >
    > @@ -10248,7 +10248,8 @@ static void set_curr_task_fair(struct rq *rq)
    >
    > set_next_entity(cfs_rq, se);
    > /* ensure bandwidth has been allocated on our new cfs_rq */
    > - account_cfs_rq_runtime(cfs_rq, 0);
    > + if (task_se_in_cgroup(se))
    > + account_cfs_rq_runtime(group_cfs_rq_of_parent(se),
    > 0);
    > }
    >
    > I fail to understand the second part of your sentence, and
    > how is this related to the code in propagate_exec_runtime():
    >
    > for_each_sched_entity(se) {
    >
    > propagate_exec_runtime() {
    >
    > if (parent)
    > account_cfs_rq_runtime(cfs_rq, diff);
    > }
    > }

    I am not sure how that would work for distributing
    runtime, since runtime would have to be distributed
    downwards and on demand, no?

    That seems like a very different code path than
    "upwards, and periodically".

    Then again, I have not worked out all the details
    of reimplementing CFS bandwidth yet...

    > > By default we should probably work under the assumption
    > > that account_cfs_rq_runtime() will succeed at the current
    > > level, and no gymnastics are required to obtain CPU time.
    >
    > Maybe this all will become clearer when the reworked CFS Bandwidth
    > support is ready ;-) I see this patch as the first part of it.

    That is one of the reasons I have not been "fixing"
    CFS bandwidth related code in the current patch series.

    Having all of those changes in one location seems like
    it would be best.

    --
    All Rights Reversed.
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2019-08-29 20:06    [W:3.747 / U:0.132 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site