| Message ID | 1457932932-28444-2-git-send-email-mturquette+renesas@baylibre.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Sun, Mar 13, 2016 at 10:22:05PM -0700, Michael Turquette wrote: > cpufreq_trigger_update() was introduced in "cpufreq: Rework the > scheduler hooks for triggering updates"[0]. Consensus is that this > helper is not needed and removing it will aid in experimenting with > deadline and rt capacity requests. > > Instead of reverting the above patch, which includes useful renaming of > data structures and related functions, simply remove the function, > update affected kerneldoc and change rt.c and deadline.c to use > cpufreq_update_util(). This fails to explain how the need for these hooks is dealt with. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 15, 2016 at 02:45:45PM -0700, Michael Turquette wrote: > Quoting Peter Zijlstra (2016-03-15 14:14:48) > > On Sun, Mar 13, 2016 at 10:22:05PM -0700, Michael Turquette wrote: > > > cpufreq_trigger_update() was introduced in "cpufreq: Rework the > > > scheduler hooks for triggering updates"[0]. Consensus is that this > > > helper is not needed and removing it will aid in experimenting with > > > deadline and rt capacity requests. > > > > > > Instead of reverting the above patch, which includes useful renaming of > > > data structures and related functions, simply remove the function, > > > update affected kerneldoc and change rt.c and deadline.c to use > > > cpufreq_update_util(). > > > > This fails to explain how the need for these hooks is dealt with. > > Sorry, I don't understand your point. The removed hook, > "cpufreq_trigger_update()" was only used in deadline.c and rt.c, and > this patch effectively reverts Rafael's patch that introduces that > function. > > It simply does not revert the other changes in Rafael's patch, such as > some renaming. > > deadline.c and rt.c are made to use cpufreq_update_util() and pass in > ULONG_MAX for capacity and 0 for time. This is exactly what they did > before patch "cpufreq: Rework the scheduler hooks for triggering > updates". Clearly I need to learn to read again.. You're right. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Mar 13, 2016 at 10:22:05PM -0700, Michael Turquette wrote: > cpufreq_trigger_update() was introduced in "cpufreq: Rework the > scheduler hooks for triggering updates"[0]. Consensus is that this > helper is not needed and removing it will aid in experimenting with > deadline and rt capacity requests. > > Instead of reverting the above patch, which includes useful renaming of > data structures and related functions, simply remove the function, > update affected kerneldoc and change rt.c and deadline.c to use > cpufreq_update_util(). > -/** > - * cpufreq_trigger_update - Trigger CPU performance state evaluation if needed. > - * @time: Current time. > - * > - * The way cpufreq is currently arranged requires it to evaluate the CPU > - * performance state (frequency/voltage) on a regular basis. To facilitate > - * that, cpufreq_update_util() is called by update_load_avg() in CFS when > - * executed for the current CPU's runqueue. > - * > - * However, this isn't sufficient to prevent the CPU from being stuck in a > - * completely inadequate performance level for too long, because the calls > - * from CFS will not be made if RT or deadline tasks are active all the time > - * (or there are RT and DL tasks only). > - * > - * As a workaround for that issue, this function is called by the RT and DL > - * sched classes to trigger extra cpufreq updates to prevent it from stalling, > - * but that really is a band-aid. Going forward it should be replaced with > - * solutions targeted more specifically at RT and DL tasks. > - */ > -void cpufreq_trigger_update(u64 time) > -{ > - cpufreq_update_util(time, ULONG_MAX, 0); > -} > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c > index 1a035fa..3fd5bc4 100644 > --- a/kernel/sched/deadline.c > +++ b/kernel/sched/deadline.c > @@ -728,7 +728,7 @@ static void update_curr_dl(struct rq *rq) > > /* Kick cpufreq (see the comment in drivers/cpufreq/cpufreq.c). */ > if (cpu_of(rq) == smp_processor_id()) > - cpufreq_trigger_update(rq_clock(rq)); > + cpufreq_update_util(rq_clock(rq), ULONG_MAX, 0); > > /* > * Consumed budget is computed considering the time as OK, so take two on this, now hopefully more coherent (yay for sleep!). So my problem is that this (update_curr_dl) is not the right location to set DL utilization (although it might be for avg dl, see the other email). The only reason it lives here, is that some cpufreq governors require 'timely' calls into this hook. The comment you destroyed tries to convey this. We should still remove this requirement from the governors. And for simple DL guarantees this hook is placed wrong. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/kernel/sched/cpufreq.c b/kernel/sched/cpufreq.c index eecaba4..bd012c2 100644 --- a/kernel/sched/cpufreq.c +++ b/kernel/sched/cpufreq.c @@ -20,8 +20,8 @@ static DEFINE_PER_CPU(struct freq_update_hook *, cpufreq_freq_update_hook); * * Set and publish the freq_update_hook pointer for the given CPU. That pointer * points to a struct freq_update_hook object containing a callback function - * to call from cpufreq_trigger_update(). That function will be called from - * an RCU read-side critical section, so it must not sleep. + * to call from cpufreq_update_util(). That function will be called from an + * RCU read-side critical section, so it must not sleep. * * Callers must use RCU-sched callbacks to free any memory that might be * accessed via the old update_util_data pointer or invoke synchronize_sched() @@ -87,27 +87,3 @@ void cpufreq_update_util(u64 time, unsigned long util, unsigned long max) if (hook) hook->func(hook, time, util, max); } - -/** - * cpufreq_trigger_update - Trigger CPU performance state evaluation if needed. - * @time: Current time. - * - * The way cpufreq is currently arranged requires it to evaluate the CPU - * performance state (frequency/voltage) on a regular basis. To facilitate - * that, cpufreq_update_util() is called by update_load_avg() in CFS when - * executed for the current CPU's runqueue. - * - * However, this isn't sufficient to prevent the CPU from being stuck in a - * completely inadequate performance level for too long, because the calls - * from CFS will not be made if RT or deadline tasks are active all the time - * (or there are RT and DL tasks only). - * - * As a workaround for that issue, this function is called by the RT and DL - * sched classes to trigger extra cpufreq updates to prevent it from stalling, - * but that really is a band-aid. Going forward it should be replaced with - * solutions targeted more specifically at RT and DL tasks. - */ -void cpufreq_trigger_update(u64 time) -{ - cpufreq_update_util(time, ULONG_MAX, 0); -} diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c index 1a035fa..3fd5bc4 100644 --- a/kernel/sched/deadline.c +++ b/kernel/sched/deadline.c @@ -728,7 +728,7 @@ static void update_curr_dl(struct rq *rq) /* Kick cpufreq (see the comment in drivers/cpufreq/cpufreq.c). */ if (cpu_of(rq) == smp_processor_id()) - cpufreq_trigger_update(rq_clock(rq)); + cpufreq_update_util(rq_clock(rq), ULONG_MAX, 0); /* * Consumed budget is computed considering the time as diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c index 9dd1c09..53ad077 100644 --- a/kernel/sched/rt.c +++ b/kernel/sched/rt.c @@ -947,7 +947,7 @@ static void update_curr_rt(struct rq *rq) /* Kick cpufreq (see the comment in drivers/cpufreq/cpufreq.c). */ if (cpu_of(rq) == smp_processor_id()) - cpufreq_trigger_update(rq_clock(rq)); + cpufreq_update_util(rq_clock(rq), ULONG_MAX, 0); delta_exec = rq_clock_task(rq) - curr->se.exec_start; if (unlikely((s64)delta_exec <= 0)) diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index 7ae012e..f06dfca 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -1742,9 +1742,7 @@ static inline u64 irq_time_read(int cpu) #ifdef CONFIG_CPU_FREQ void cpufreq_update_util(u64 time, unsigned long util, unsigned long max); -void cpufreq_trigger_update(u64 time); #else static inline void cpufreq_update_util(u64 time, unsigned long util, unsigned long max) {} -static inline void cpufreq_trigger_update(u64 time) {} #endif /* CONFIG_CPU_FREQ */
cpufreq_trigger_update() was introduced in "cpufreq: Rework the scheduler hooks for triggering updates"[0]. Consensus is that this helper is not needed and removing it will aid in experimenting with deadline and rt capacity requests. Instead of reverting the above patch, which includes useful renaming of data structures and related functions, simply remove the function, update affected kerneldoc and change rt.c and deadline.c to use cpufreq_update_util(). [0] lkml.kernel.org/r/7541372.ciUW4go8Ux@vostro.rjw.lan Signed-off-by: Michael Turquette <mturquette+renesas@baylibre.com> --- kernel/sched/cpufreq.c | 28 ++-------------------------- kernel/sched/deadline.c | 2 +- kernel/sched/rt.c | 2 +- kernel/sched/sched.h | 2 -- 4 files changed, 4 insertions(+), 30 deletions(-)