linux-kernel - Re: [PATCH v7 1/4] cpufreq: handle SW coordinated CPUs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 30 Jan 2013 20:32:38 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	Fabio Baltieri <fabio.baltieri@...aro.org>
Cc:	"Rafael J. Wysocki" <rjw@...k.pl>, cpufreq@...r.kernel.org,
	linux-pm@...r.kernel.org, Linus Walleij <linus.walleij@...aro.org>,
	swarren@...dotorg.org, linaro-dev@...ts.linaro.org,
	Nicolas Pitre <nicolas.pitre@...aro.org>,
	mathieu.poirier@...aro.org, Joseph Lo <josephl@...dia.com>,
	linux-kernel@...r.kernel.org,
	Rickard Andersson <rickard.andersson@...ricsson.com>
Subject: Re: [PATCH v7 1/4] cpufreq: handle SW coordinated CPUs

Hi Fabio,

I know Rafael has asked you to send only the incremental patch, but i
am interested in reviewing whole series again, as i haven't reviewed
earlier stuff :)

I believe you are required to send patches to patches@...aro.org too :)

On 30 January 2013 18:30, Fabio Baltieri <fabio.baltieri@...aro.org> wrote:
> From: Rickard Andersson <rickard.andersson@...ricsson.com>
>
> This patch fixes a bug that occurred when we had load on a secondary CPU
> and the primary CPU was sleeping. Only one sampling timer was spawned
> and it was spawned as a deferred timer on the primary CPU, so when a
> secondary CPU had a change in load this was not detected by the cpufreq
> governor (both ondemand and conservative).
>
> This patch make sure that deferred timers are run on all CPUs in the
> case of software controlled CPUs that run on the same frequency.

Its really a good point. I wondered earlier why does interactive governor
has per-cpu timer. BTW, you can check how interactive governor is handling
this requirement. That might improve these drivers too.

> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> +bool dbs_sw_coordinated_cpus(struct cpu_dbs_common_info *cdbs)
> +{
> +       struct cpufreq_policy *policy = cdbs->cur_policy;
> +
> +       return cpumask_weight(policy->cpus) > 1;
> +}
> +EXPORT_SYMBOL_GPL(dbs_sw_coordinated_cpus);

I believe this routine should be rather present in cpufreq core code,
as their might
be other users of it. Its really not related to dbs or governors.

My ideas about the name of routine then:
- policy_is_shared()
- or something better you have :)

> @@ -217,6 +227,10 @@ int cpufreq_governor_dbs(struct dbs_data *dbs_data,
>                         if (ignore_nice)
>                                 j_cdbs->prev_cpu_nice =
>                                         kcpustat_cpu(j).cpustat[CPUTIME_NICE];
> +
> +                       mutex_init(&j_cdbs->timer_mutex);
> +                       INIT_DEFERRABLE_WORK(&j_cdbs->work,
> +                                            dbs_data->gov_dbs_timer);

That doesn't look the right place for doing it. With this change we
will initialize
mutex and work for all cpus whenever cpufreq_governor_dbs(GOV_START) is
called. We really want to do it just before second_time: label, which will do
it only when this is called for the very first time or cpu.

That's what i thought initially :)

Then i looked at my own work and realized something else :).. Now, because
we stop/start governors on every cpu movement, this routine is called only once.

What you can do:
- Create a single for_each_cpu() in GOV_START path
- Get rid of dbs_data->enable routine as we don't need to store the
number of calls
  for GOV_START.

>                 }
>
>                 /*
> @@ -275,15 +289,16 @@ second_time:
>                 }
>                 mutex_unlock(&dbs_data->mutex);
>
> -               mutex_init(&cpu_cdbs->timer_mutex);
> -               dbs_timer_init(dbs_data, cpu_cdbs, *sampling_rate);
> +               for_each_cpu(j, policy->cpus)
> +                       dbs_timer_init(dbs_data, j, *sampling_rate);

Keep this too in the same for_each_cpu() loop and hence get to older
param types of dbs_timer_init(), i.e. don't pass cpu as we already have
j_cdbs in our loop.

>                 break;
>
>         case CPUFREQ_GOV_STOP:
>                 if (dbs_data->governor == GOV_CONSERVATIVE)
>                         cs_dbs_info->enable = 0;
>
> -               dbs_timer_exit(cpu_cdbs);
> +               for_each_cpu(j, policy->cpus)
> +                       dbs_timer_exit(dbs_data, j);
>
>                 mutex_lock(&dbs_data->mutex);
>                 mutex_destroy(&cpu_cdbs->timer_mutex);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/