On 28 April 2015 at 11:53, Shilpasri G Bhat
<shilpa.b...@linux.vnet.ibm.com> wrote:
> Changes from v1:
> - Add macros to define OCC_RESET, OCC_LOAD and OCC_THROTTLE
> - Define a structure to store chip id, chip mask which has bits set
> for cpus present in the chip, throttled state and a work_struct.
> - Modify powernv_cpufreq_throttle_check() to be called via smp_call()
Why ? I might have missed it but there should be some reasoning behind
what you are changing.
> - On Pmax throttling/unthrottling update 'chip.throttled' and not the
> global 'throttled' as Pmax capping is local to the chip.
> - Remove the condition which checks if local pstate is less than Pmin
> while checking for Psafe frequency. When OCC becomes active after
> reset we update 'thottled' to false and when the cpufreq governor
> initiates a pstate change, the local pstate will be in Psafe and we
> will be reporting a false positive when we are not throttled.
> - Schedule a kworker on receiving throttling/unthrottling OCC message
> for that chip and schedule on all chips after receiving active.
> - After an OCC reset all the cpus will be in Psafe frequency. So call
> target() and restore the frequency to policy->cur after OCC_ACTIVE
> and Pmax unthrottling
> - Taken care of Viresh and Preeti's comments.
That's a lot. I am not an expert here and so really can't comment on
the internals of ppc. But, is it patch solving a single problem ? I don't
know, I somehow got the impression that it can be split into multiple
(smaller & review-able) patches. Only if it makes sense. Your call.
> diff --git a/drivers/cpufreq/powernv-cpufreq.c
> b/drivers/cpufreq/powernv-cpufreq.c
> +void powernv_cpufreq_work_fn(struct work_struct *work)
> +{
> + struct chip *c = container_of(work, struct chip, throttle);
> + unsigned int cpu;
> +
> + smp_call_function_any(&c->mask,
> + powernv_cpufreq_throttle_check, NULL, 0);
> +
> + for_each_cpu(cpu, &c->mask) {
for_each_online_cpu ?
> + int index;
> + struct cpufreq_frequency_table *freq_table;
> + struct cpufreq_policy cpu_policy;
Name it policy.
> +
> + if (!cpu_online(cpu))
> + continue;
And you can kill this..
> + cpufreq_get_policy(&cpu_policy, cpu);
> + freq_table = cpufreq_frequency_get_table(cpu_policy.cpu);
Just do, policy->freq_table.
> +static int powernv_cpufreq_occ_msg(struct notifier_block *nb,
> + unsigned long msg_type, void *msg)
> +{
> + if (reason && reason <= 5)
> + pr_info("OCC: Chip %d Pmax reduced due to %s\n",
> + (int)chip_id, throttle_reason[reason]);
> + else
> + pr_info("OCC: Chip %d %s\n", (int)chip_id,
> + throttle_reason[reason]);
Blank line here. They are better for readability after blocks and loops.
> + for (i = 0; i < nr_chips; i++)
> + if (chips[i].id == (int)chip_id)
Why isn't .id 64 bit ?
> + schedule_work(&chips[i].throttle);
> + }
> + return 0;
> +}
> +
> +static struct notifier_block powernv_cpufreq_opal_nb = {
> + .notifier_call = powernv_cpufreq_occ_msg,
> + .next = NULL,
> + .priority = 0,
> +};
> +
> static void powernv_cpufreq_stop_cpu(struct cpufreq_policy *policy)
> {
> struct powernv_smp_call_data freq_data;
> @@ -414,6 +530,35 @@ static struct cpufreq_driver powernv_cpufreq_driver = {
> .attr = powernv_cpu_freq_attr,
> };
>
> +static int init_chip_info(void)
> +{
> + int chip[256], i = 0, cpu;
> + int prev_chip_id = INT_MAX;
> +
> + for_each_possible_cpu(cpu) {
> + int c = cpu_to_chip_id(cpu);
Does 'c' refer to id here ? Name it so then.
> +
> + if (prev_chip_id != c) {
> + prev_chip_id = c;
> + chip[nr_chips++] = c;
> + }
> + }
> +
> + chips = kmalloc_array(nr_chips, sizeof(struct chip), GFP_KERNEL);
> +
A blank line isn't preferred much here :). Sorry about these blank lines.
> + if (!chips)
> + return -ENOMEM;
> +
> + for (i = 0; i < nr_chips; i++) {
> + chips[i].id = chip[i];
> + cpumask_copy(&chips[i].mask, cpumask_of_node(chip[i]));
> + chips[i].throttled = false;
> + INIT_WORK(&chips[i].throttle, powernv_cpufreq_work_fn);
> + }
> +
> + return 0;
> +}
> +
> static int __init powernv_cpufreq_init(void)
> {
> int rc = 0;
> @@ -429,7 +574,13 @@ static int __init powernv_cpufreq_init(void)
> return rc;
> }
>
> + /* Populate chip info */
> + rc = init_chip_info();
> + if (rc)
> + return rc;
> +
> register_reboot_notifier(&powernv_cpufreq_reboot_nb);
> + opal_message_notifier_register(OPAL_MSG_OCC,
> &powernv_cpufreq_opal_nb);
> return cpufreq_register_driver(&powernv_cpufreq_driver);
> }
> module_init(powernv_cpufreq_init);
> --
> 1.9.3
>
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
