Re: [RFCv6 PATCH 03/10] sched: scheduler-driven cpu frequency selection

[Steve Muckle]

Hi Ricky,

On 02/01/2016 09:10 AM, Ricky Liang wrote:
>> +static int cpufreq_sched_policy_init(struct cpufreq_policy *policy)
>> > +{
>> > +       struct gov_data *gd;
>> > +       int cpu;
>> > +
>> > +       for_each_cpu(cpu, policy->cpus)
>> > +               memset(&per_cpu(cpu_sched_capacity_reqs, cpu), 0,
>> > +                      sizeof(struct sched_capacity_reqs));
>> > +
>> > +       gd = kzalloc(sizeof(*gd), GFP_KERNEL);
>> > +       if (!gd)
>> > +               return -ENOMEM;
>> > +
>> > +       gd->throttle_nsec = policy->cpuinfo.transition_latency ?
>> > +                           policy->cpuinfo.transition_latency :
>> > +                           THROTTLE_NSEC;
>> > +       pr_debug("%s: throttle threshold = %u [ns]\n",
>> > +                 __func__, gd->throttle_nsec);
>> > +
>> > +       if (cpufreq_driver_is_slow()) {
>> > +               cpufreq_driver_slow = true;
>> > +               gd->task = kthread_create(cpufreq_sched_thread, policy,
>> > +                                         "kschedfreq:%d",
>> > +                                         
>> > cpumask_first(policy->related_cpus));
>> > +               if (IS_ERR_OR_NULL(gd->task)) {
>> > +                       pr_err("%s: failed to create kschedfreq thread\n",
>> > +                              __func__);
>> > +                       goto err;
>> > +               }
>> > +               get_task_struct(gd->task);
>> > +               kthread_bind_mask(gd->task, policy->related_cpus);
>> > +               wake_up_process(gd->task);
>> > +               init_irq_work(&gd->irq_work, cpufreq_sched_irq_work);
>> > +       }
>> > +
>> > +       policy->governor_data = gd;
>
> This should be moved before if(cpufreq_driver_is_slow()) {...}. I've
> seen NULL pointer deference at boot in cpufreq_sched_thread() when it
> tried to run sched_setscheduler_nocheck(gd->task, SCHED_FIFO, &param).

Agreed, this has been addressed during various cleanups and
reorganization since the last posting.

> 
>> > +       set_sched_freq();
>> > +
>> > +       return 0;
>> > +
>> > +err:
> And probably also set policy->governor_data to NULL here.

Changed. Thanks for the comments.

thanks,
Steve