From: Vincent Guittot <vincent.guittot@linaro.org> To: Odin Ugedal <odin@uged.al> Cc: Ingo Molnar <mingo@redhat.com>, Peter Zijlstra <peterz@infradead.org>, Juri Lelli <juri.lelli@redhat.com>, Dietmar Eggemann <dietmar.eggemann@arm.com>, Steven Rostedt <rostedt@goodmis.org>, Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>, Daniel Bristot de Oliveira <bristot@redhat.com>, "open list:CONTROL GROUP (CGROUP)" <cgroups@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 1/3] sched/fair: Add tg_load_contrib cfs_rq decay checking Date: Thu, 27 May 2021 14:37:16 +0200 [thread overview] Message-ID: <CAKfTPtA5NxDpG194zAF9CjchJ5hm-7fOBOPGJZG4+z4-+32qGw@mail.gmail.com> (raw) In-Reply-To: <CAFpoUr0PTYs+CSiWt3WOXnxq=wN3uEyC=h+_3kDc9wLoqaRC_Q@mail.gmail.com> On Thu, 27 May 2021 at 13:04, Odin Ugedal <odin@uged.al> wrote: > > > 1st : ensure that cfs_rq->load_sum is not null if cfs_rq-> load_isn't too > > 2nd : call update_tg_load_avg() during child update so we will be sure > > to update tg_load_avg_contrib before removing the cfs from the list > > Ahh, yeah, with "1st" that would work. Yeah, that was my initial > implementation of the change, but I thought that it was better to keep > the logic away from the "hot path". We can verify this in > update_tg_cfs_load(), and then force update_tg_load_avg() inside For 1st problem, the way we were updating load_avg and load_sum, we were losing the sync between both value > __update_blocked_fair() when avg.load_avg is 0. (Given that this is > the only place where we can end up in this situation. I can update > this patch to do that instead. In fact, the update was already there but not always called (see the patchset i just sent) > > Another solution is to update avg.load_avg > inside__update_blocked_fair() when load_sum is 0, and then propagate > that with update_tg_load_avg(). This removes the logic from the hot > path all together. > > Not sure what the preferred way is. I have not found any other places > where this situation _should_ occur, but who knows.. > > Odin
WARNING: multiple messages have this Message-ID (diff)
From: Vincent Guittot <vincent.guittot-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> To: Odin Ugedal <odin-RObV4cXtwVA@public.gmane.org> Cc: Ingo Molnar <mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>, Juri Lelli <juri.lelli-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, Dietmar Eggemann <dietmar.eggemann-5wv7dgnIgG8@public.gmane.org>, Steven Rostedt <rostedt-nx8X9YLhiw1AfugRpC6u6w@public.gmane.org>, Ben Segall <bsegall-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>, Mel Gorman <mgorman-l3A5Bk7waGM@public.gmane.org>, Daniel Bristot de Oliveira <bristot-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "open list:CONTROL GROUP (CGROUP)" <cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, linux-kernel <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> Subject: Re: [PATCH 1/3] sched/fair: Add tg_load_contrib cfs_rq decay checking Date: Thu, 27 May 2021 14:37:16 +0200 [thread overview] Message-ID: <CAKfTPtA5NxDpG194zAF9CjchJ5hm-7fOBOPGJZG4+z4-+32qGw@mail.gmail.com> (raw) In-Reply-To: <CAFpoUr0PTYs+CSiWt3WOXnxq=wN3uEyC=h+_3kDc9wLoqaRC_Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> On Thu, 27 May 2021 at 13:04, Odin Ugedal <odin-RObV4cXtwVA@public.gmane.org> wrote: > > > 1st : ensure that cfs_rq->load_sum is not null if cfs_rq-> load_isn't too > > 2nd : call update_tg_load_avg() during child update so we will be sure > > to update tg_load_avg_contrib before removing the cfs from the list > > Ahh, yeah, with "1st" that would work. Yeah, that was my initial > implementation of the change, but I thought that it was better to keep > the logic away from the "hot path". We can verify this in > update_tg_cfs_load(), and then force update_tg_load_avg() inside For 1st problem, the way we were updating load_avg and load_sum, we were losing the sync between both value > __update_blocked_fair() when avg.load_avg is 0. (Given that this is > the only place where we can end up in this situation. I can update > this patch to do that instead. In fact, the update was already there but not always called (see the patchset i just sent) > > Another solution is to update avg.load_avg > inside__update_blocked_fair() when load_sum is 0, and then propagate > that with update_tg_load_avg(). This removes the logic from the hot > path all together. > > Not sure what the preferred way is. I have not found any other places > where this situation _should_ occur, but who knows.. > > Odin
next prev parent reply other threads:[~2021-05-27 12:37 UTC|newest] Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-18 12:51 [PATCH 0/3] sched/fair: Fix load decay issues related to throttling Odin Ugedal 2021-05-18 12:51 ` Odin Ugedal 2021-05-18 12:52 ` [PATCH 1/3] sched/fair: Add tg_load_contrib cfs_rq decay checking Odin Ugedal 2021-05-18 12:52 ` Odin Ugedal 2021-05-25 9:58 ` Vincent Guittot 2021-05-25 10:33 ` Odin Ugedal 2021-05-25 10:33 ` Odin Ugedal 2021-05-25 14:30 ` Vincent Guittot 2021-05-25 14:30 ` Vincent Guittot 2021-05-26 10:50 ` Vincent Guittot 2021-05-26 10:50 ` Vincent Guittot 2021-05-27 7:50 ` Odin Ugedal 2021-05-27 7:50 ` Odin Ugedal 2021-05-27 9:35 ` Vincent Guittot 2021-05-27 9:45 ` Odin Ugedal 2021-05-27 10:49 ` Vincent Guittot 2021-05-27 10:49 ` Vincent Guittot 2021-05-27 11:04 ` Odin Ugedal 2021-05-27 11:04 ` Odin Ugedal 2021-05-27 12:37 ` Vincent Guittot [this message] 2021-05-27 12:37 ` Vincent Guittot 2021-05-27 12:37 ` Odin Ugedal 2021-05-27 12:37 ` Odin Ugedal 2021-05-27 12:39 ` Odin Ugedal 2021-05-27 12:39 ` Odin Ugedal 2021-05-27 12:49 ` Vincent Guittot 2021-05-18 12:52 ` [PATCH 2/3] sched/fair: Correctly insert cfs_rq's to list on unthrottle Odin Ugedal 2021-05-18 12:52 ` Odin Ugedal 2021-05-28 14:24 ` Vincent Guittot 2021-05-28 15:06 ` Odin Ugedal 2021-05-28 15:27 ` Vincent Guittot 2021-05-28 15:27 ` Vincent Guittot 2021-05-29 9:33 ` Odin Ugedal 2021-05-31 12:14 ` Vincent Guittot 2021-05-31 12:14 ` Vincent Guittot 2021-05-18 12:52 ` [PATCH 3/3] sched/fair: Fix ascii art by relpacing tabs Odin Ugedal 2021-05-27 13:27 ` Vincent Guittot 2021-05-27 13:27 ` Vincent Guittot 2021-06-01 14:04 ` [tip: sched/core] " tip-bot2 for Odin Ugedal
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=CAKfTPtA5NxDpG194zAF9CjchJ5hm-7fOBOPGJZG4+z4-+32qGw@mail.gmail.com \ --to=vincent.guittot@linaro.org \ --cc=bristot@redhat.com \ --cc=bsegall@google.com \ --cc=cgroups@vger.kernel.org \ --cc=dietmar.eggemann@arm.com \ --cc=juri.lelli@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=mgorman@suse.de \ --cc=mingo@redhat.com \ --cc=odin@uged.al \ --cc=peterz@infradead.org \ --cc=rostedt@goodmis.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.