On 04/09/15 14:27, Juri Lelli wrote: > On 15/08/15 13:35, Peter Zijlstra wrote: >> On Tue, Jul 07, 2015 at 07:24:21PM +0100, Morten Rasmussen wrote: >>> diff --git a/kernel/sched/cpufreq_sched.c b/kernel/sched/cpufreq_sched.c >>> new file mode 100644 >>> index 0000000..5020f24 >>> --- /dev/null >>> +++ b/kernel/sched/cpufreq_sched.c >>> @@ -0,0 +1,308 @@ >>> +/* >>> + * Copyright (C) 2015 Michael Turquette <[email protected]> >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2 as >>> + * published by the Free Software Foundation. >>> + */ >>> + >>> +#include <linux/cpufreq.h> >>> +#include <linux/module.h> >>> +#include <linux/kthread.h> >>> +#include <linux/percpu.h> >>> +#include <linux/irq_work.h> >>> + >>> +#include "sched.h" >>> + >>> +#define THROTTLE_NSEC 50000000 /* 50ms default */ >>> + >>> +static DEFINE_PER_CPU(unsigned long, pcpu_capacity); >>> +static DEFINE_PER_CPU(struct cpufreq_policy *, pcpu_policy); >>> + >>> +/** >>> + * gov_data - per-policy data internal to the governor >>> + * @throttle: next throttling period expiry. Derived from throttle_nsec >>> + * @throttle_nsec: throttle period length in nanoseconds >>> + * @task: worker thread for dvfs transition that may block/sleep >>> + * @irq_work: callback used to wake up worker thread >>> + * @freq: new frequency stored in *_sched_update_cpu and used in >>> *_sched_thread >>> + * >>> + * struct gov_data is the per-policy cpufreq_sched-specific data >>> structure. A >>> + * per-policy instance of it is created when the cpufreq_sched governor >>> receives >>> + * the CPUFREQ_GOV_START condition and a pointer to it exists in the >>> gov_data >>> + * member of struct cpufreq_policy. >>> + * >>> + * Readers of this data must call down_read(policy->rwsem). Writers must >>> + * call down_write(policy->rwsem). >>> + */ >>> +struct gov_data { >>> + ktime_t throttle; >>> + unsigned int throttle_nsec; >>> + struct task_struct *task; >>> + struct irq_work irq_work; >>> + struct cpufreq_policy *policy; >>> + unsigned int freq; >>> +}; >>> + >>> +static void cpufreq_sched_try_driver_target(struct cpufreq_policy *policy, >>> unsigned int freq) >>> +{ >>> + struct gov_data *gd = policy->governor_data; >>> + >>> + /* avoid race with cpufreq_sched_stop */ >>> + if (!down_write_trylock(&policy->rwsem)) >>> + return; >>> + >>> + __cpufreq_driver_target(policy, freq, CPUFREQ_RELATION_L); >>> + >>> + gd->throttle = ktime_add_ns(ktime_get(), gd->throttle_nsec); >>> + up_write(&policy->rwsem); >>> +} >> >> That locking truly is disgusting.. why can't we change that? >> >>> +static int cpufreq_sched_thread(void *data) >>> +{ >> >>> + >>> + ret = set_cpus_allowed_ptr(gd->task, policy->related_cpus); >> >> That's not sufficient, you really want to have called kthread_bind() on >> these threads, otherwise userspace can change affinity on you. >> >>> + >>> + do_exit(0); >> >> I thought kthreads only needed to return... >> >>> +} >> >>> +void cpufreq_sched_set_cap(int cpu, unsigned long capacity) >>> +{ >>> + unsigned int freq_new, cpu_tmp; >>> + struct cpufreq_policy *policy; >>> + struct gov_data *gd; >>> + unsigned long capacity_max = 0; >>> + >>> + /* update per-cpu capacity request */ >>> + __this_cpu_write(pcpu_capacity, capacity); >>> + >>> + policy = cpufreq_cpu_get(cpu); >> >> So this does a down_read_trylock(&cpufreq_rwsem) and a >> read_lock_irqsave(&cpufreq_driver_lock), all while holding scheduler >> locks. >> >>> + if (cpufreq_driver_might_sleep()) >>> + irq_work_queue_on(&gd->irq_work, cpu); >>> + else >>> + cpufreq_sched_try_driver_target(policy, freq_new); >> >> This will then do a down_write_trylock(&policy->rwsem) >> >>> + >>> +out: >>> + cpufreq_cpu_put(policy); >> >>> + return; >>> +} >> >> That is just insane... surely we can replace all that with a wee bit of >> RCU logic. >> >> So something like: >> >> DEFINE_MUTEX(cpufreq_mutex); >> struct cpufreq_driver *cpufreq_driver; >> >> struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu) >> { >> struct cpufreq_driver *driver; >> struct cpufreq_policy *policy; >> >> rcu_read_lock(); >> driver = rcu_dereference(cpufreq_driver); >> if (!driver) >> goto err; >> >> policy = per_cpu_ptr(driver->policy, cpu); >> if (!policy) >> goto err; >> >> return policy; >> >> err: >> rcu_read_unlock(); >> return NULL; >> } >> >> >> void cpufreq_cpu_put(struct cpufreq_policy *policy) >> { >> rcu_read_unlock(); >> } >> >> >> >> void cpufreq_set_driver(struct cpufreq_driver *driver) >> { >> mutex_lock(&cpufreq_mutex); >> >> rcu_assign_pointer(cpufreq_driver, NULL); >> >> /* >> * Wait for everyone to observe the lack of driver; iow. until >> * its unused. >> */ >> synchronize_rcu(); >> >> /* >> * Now that ye olde driver be gone, install a new one. >> */ >> if (driver) >> rcu_assign_pointer(cpufreq_driver, driver); >> >> mutex_unlock(&cpufreq_mutex); >> } >> >> >> No need for cpufreq_rwsem or cpufreq_driver_lock.. >> >> >> Hmm? >> > > So, just to recall what we discussed at LPC (I have Mike's slides > at hand :-)). It seems that key points are: > > 1- we agreed that locking in cpufreq core has to change as we > have to access it from scheduler hot-paths; what Peter is > proposing above looks viable to me, what others (way more > confident then me with cpufreq inners) say? > > 2- the interface has to be extended as we have to let other > scheduling classes drive freq selection too; I guess that how > we do aggregation depends on the nature of sched classes, > but we didn't really reach any sort of agreement here; is > this anyway something we can focus on after fixing locking? > > 3- the interface should also support peripheral devices; this > seems a interesting feature to have, but how about we postpone > it after we've got previous points right? > > What did I miss of crucial? :-) >
Hi Mike, others, ping on above points. Any comments on how we can move forward? :-) Best, - Juri -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
