diff mbox

[2/2] CPUFreq: Add new sysfs attribute freqdomain_cpus for acpi-freq driver

Message ID CAKohpokUmvkkGwCDDa1d8OCz8YcsE2J0GQkA-dJ=EE+6REm7Mg@mail.gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Viresh Kumar June 25, 2013, 3:56 a.m. UTC
On 25 June 2013 07:36, Lan Tianyu <tianyu.lan@intel.com> wrote:
> diff --git a/Documentation/cpu-freq/user-guide.txt b/Documentation/cpu-freq/user-guide.txt
> index ff2f283..0cc72f7 100644
> --- a/Documentation/cpu-freq/user-guide.txt
> +++ b/Documentation/cpu-freq/user-guide.txt
> @@ -196,6 +196,10 @@ affected_cpus :                    List of Online CPUs that require software
>  related_cpus :                 List of Online + Offline CPUs that need software
>                                 coordination of frequency.
>
> +freqdomain_cpus :              List of Online + Offline CPUs in same CPU dependency
> +                               domain. (This is only available for acpi-cpufreq
> +                               driver)
> +

This is generic file, don't add this information here. Add this in
acpi-cpufreq file.

>  scaling_driver :               Hardware driver for cpufreq.
>
>  scaling_cur_freq :             Current frequency of the CPU as determined by
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 17e3496..b859997 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -176,6 +176,16 @@ static struct global_attr global_boost = __ATTR(boost, 0644,
>                                                 show_global_boost,
>                                                 store_global_boost);
>
> +static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
> +{
> +       struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy->cpu);
> +       struct acpi_processor_performance *perf = data->acpi_data;
> +
> +       return cpufreq_show_cpus(perf->shared_cpu_map, buf);
> +}

I am not sure if this is enough. Check this commit:

aa77a52764a92216b61a6c8079b5c01937c046cd

It had these changes:

        dmi_check_system(sw_any_bug_dmi_table);
@@ -742,7 +741,6 @@ static int acpi_cpufreq_cpu_init(struct
cpufreq_policy *policy)
        if (check_amd_hwpstate_cpu(cpu) && !acpi_pstate_strict) {
                cpumask_clear(policy->cpus);
                cpumask_set_cpu(cpu, policy->cpus);
-               cpumask_copy(policy->related_cpus, cpu_sibling_mask(cpu));
                policy->shared_type = CPUFREQ_SHARED_TYPE_HW;
                pr_info_once(PFX "overriding BIOS provided _PSD data\n");
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

lan,Tianyu June 25, 2013, 6:54 a.m. UTC | #1
Hi Viresh
	Thanks for your review.

On 2013?06?25? 11:56, Viresh Kumar wrote:
> On 25 June 2013 07:36, Lan Tianyu <tianyu.lan@intel.com> wrote:
>> diff --git a/Documentation/cpu-freq/user-guide.txt b/Documentation/cpu-freq/user-guide.txt
>> index ff2f283..0cc72f7 100644
>> --- a/Documentation/cpu-freq/user-guide.txt
>> +++ b/Documentation/cpu-freq/user-guide.txt
>> @@ -196,6 +196,10 @@ affected_cpus :                    List of Online CPUs that require software
>>  related_cpus :                 List of Online + Offline CPUs that need software
>>                                 coordination of frequency.
>>
>> +freqdomain_cpus :              List of Online + Offline CPUs in same CPU dependency
>> +                               domain. (This is only available for acpi-cpufreq
>> +                               driver)
>> +
> 
> This is generic file, don't add this information here. Add this in
> acpi-cpufreq file.
I don't find acpi-cpufreq.txt under
Documentation/cpu-freq/acpi-cpufreq.txt. So I should create it?

> 
>>  scaling_driver :               Hardware driver for cpufreq.
>>
>>  scaling_cur_freq :             Current frequency of the CPU as determined by
>> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
>> index 17e3496..b859997 100644
>> --- a/drivers/cpufreq/acpi-cpufreq.c
>> +++ b/drivers/cpufreq/acpi-cpufreq.c
>> @@ -176,6 +176,16 @@ static struct global_attr global_boost = __ATTR(boost, 0644,
>>                                                 show_global_boost,
>>                                                 store_global_boost);
>>
>> +static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
>> +{
>> +       struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy->cpu);
>> +       struct acpi_processor_performance *perf = data->acpi_data;
>> +
>> +       return cpufreq_show_cpus(perf->shared_cpu_map, buf);
>> +}
> 
> I am not sure if this is enough. Check this commit:
> 
> aa77a52764a92216b61a6c8079b5c01937c046cd
> 
> It had these changes:

Please see the acpi_processor_preregister_performance() in the
drivers/acpi/processor_perlib.c. All cpus in the same dependency domain
are stored in the perf->shared_cpu_map(including the reference cpu the
perf belongs to). Original code will copy shared_cpu_map to
policy->related_cpus, do cpumask_or between policy->related_cpus and
policy->cpus in the cpufreq_add_dev() and store the result into
policy->related_cpus. Expose the data via related_cpus.

For acpi-cpufreq driver, the shared_cpu_map is the biggest subset since
it regardless the coordination type.

After the commit aa77a5, only the cpus in the software or
software&hardware coordination type dependency domain will copy to
policy->cpus and finally store into policy->related_cpus in the
cpufreq_add_dev() by cpumask_or. And then we miss the cpus in the
hardware coordination type domain. But these info is still stored in the
policy->shared_cpu_map. Does this make sense?

> 
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 937bc28..57a8774 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -730,7 +730,6 @@ static int acpi_cpufreq_cpu_init(struct
> cpufreq_policy *policy)
>             policy->shared_type == CPUFREQ_SHARED_TYPE_ANY) {
>                 cpumask_copy(policy->cpus, perf->shared_cpu_map);
>         }
> -       cpumask_copy(policy->related_cpus, perf->shared_cpu_map);
> 
>  #ifdef CONFIG_SMP
>         dmi_check_system(sw_any_bug_dmi_table);
> @@ -742,7 +741,6 @@ static int acpi_cpufreq_cpu_init(struct
> cpufreq_policy *policy)
>         if (check_amd_hwpstate_cpu(cpu) && !acpi_pstate_strict) {
>                 cpumask_clear(policy->cpus);
>                 cpumask_set_cpu(cpu, policy->cpus);
> -               cpumask_copy(policy->related_cpus, cpu_sibling_mask(cpu));
>                 policy->shared_type = CPUFREQ_SHARED_TYPE_HW;
>                 pr_info_once(PFX "overriding BIOS provided _PSD data\n");
>
Viresh Kumar June 25, 2013, 7:48 a.m. UTC | #2
On 25 June 2013 12:24, Lan Tianyu <tianyu.lan@intel.com> wrote:
> On 2013?06?25? 11:56, Viresh Kumar wrote:

>> This is generic file, don't add this information here. Add this in
>> acpi-cpufreq file.
> I don't find acpi-cpufreq.txt under
> Documentation/cpu-freq/acpi-cpufreq.txt. So I should create it?

I meant acpi-cpufreq.c file

> Please see the acpi_processor_preregister_performance() in the
> drivers/acpi/processor_perlib.c. All cpus in the same dependency domain
> are stored in the perf->shared_cpu_map(including the reference cpu the
> perf belongs to). Original code will copy shared_cpu_map to
> policy->related_cpus, do cpumask_or between policy->related_cpus and
> policy->cpus in the cpufreq_add_dev() and store the result into
> policy->related_cpus. Expose the data via related_cpus.
>
> For acpi-cpufreq driver, the shared_cpu_map is the biggest subset since
> it regardless the coordination type.
>
> After the commit aa77a5, only the cpus in the software or
> software&hardware coordination type dependency domain will copy to
> policy->cpus and finally store into policy->related_cpus in the
> cpufreq_add_dev() by cpumask_or. And then we miss the cpus in the
> hardware coordination type domain. But these info is still stored in the
> policy->shared_cpu_map. Does this make sense?

I was concerned about this code that was present earlier. That changes
related_cpus based on some conditions.

        if (check_amd_hwpstate_cpu(cpu) && !acpi_pstate_strict) {
                cpumask_clear(policy->cpus);
                cpumask_set_cpu(cpu, policy->cpus);
                cpumask_copy(policy->related_cpus, cpu_sibling_mask(cpu));
                policy->shared_type = CPUFREQ_SHARED_TYPE_HW;
                pr_info_once(PFX "overriding BIOS provided _PSD data\n");
        }
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
lan,Tianyu June 25, 2013, 8:19 a.m. UTC | #3
On 2013?06?25? 15:48, Viresh Kumar wrote:
> On 25 June 2013 12:24, Lan Tianyu <tianyu.lan@intel.com> wrote:
>> On 2013?06?25? 11:56, Viresh Kumar wrote:
> 
>>> This is generic file, don't add this information here. Add this in
>>> acpi-cpufreq file.
>> I don't find acpi-cpufreq.txt under
>> Documentation/cpu-freq/acpi-cpufreq.txt. So I should create it?
> 
> I meant acpi-cpufreq.c file
Ok. From my opinion, the new attribute is an ABI and it's better to add
descriptor under Document directory. The user can be easy to find how to
use it.

> 
>> Please see the acpi_processor_preregister_performance() in the
>> drivers/acpi/processor_perlib.c. All cpus in the same dependency domain
>> are stored in the perf->shared_cpu_map(including the reference cpu the
>> perf belongs to). Original code will copy shared_cpu_map to
>> policy->related_cpus, do cpumask_or between policy->related_cpus and
>> policy->cpus in the cpufreq_add_dev() and store the result into
>> policy->related_cpus. Expose the data via related_cpus.
>>
>> For acpi-cpufreq driver, the shared_cpu_map is the biggest subset since
>> it regardless the coordination type.
>>
>> After the commit aa77a5, only the cpus in the software or
>> software&hardware coordination type dependency domain will copy to
>> policy->cpus and finally store into policy->related_cpus in the
>> cpufreq_add_dev() by cpumask_or. And then we miss the cpus in the
>> hardware coordination type domain. But these info is still stored in the
>> policy->shared_cpu_map. Does this make sense?
> 
> I was concerned about this code that was present earlier. That changes
> related_cpus based on some conditions.

Please see the commit which add the code. Maybe, we should overwrite
shared_cpu_map by sibling_cpus for this case?

commit acd316248205d553594296f1895ba5196b89ffcc
Author: Andre Przywara <andre.przywara@amd.com>
Date:   Tue Sep 4 08:28:03 2012 +0000

    acpi-cpufreq: Add quirk to disable _PSD usage on all AMD CPUs

    To workaround some Windows specific behavior, the ACPI _PSD table
    on AMD desktop boards advertises all cores as dependent, meaning
    that they all can only use the same P-state. acpi-cpufreq strictly
    obeys this description, instantiating one CPU only and symlinking
    the others. But the hardware can have distinct frequencies for each
    core and powernow-k8 did it that way.
    So, in order to use the hardware to its full potential and keep the
    original powernow-k8 behavior, lets override the _PSD table setting
    on AMD hardware.
    We use the siblings table, as it matches the current hardware
    behavior.

    Signed-off-by: Andre Przywara <andre.przywara@amd.com>
    Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>


> 
>         if (check_amd_hwpstate_cpu(cpu) && !acpi_pstate_strict) {
>                 cpumask_clear(policy->cpus);
>                 cpumask_set_cpu(cpu, policy->cpus);
>                 cpumask_copy(policy->related_cpus, cpu_sibling_mask(cpu));
>                 policy->shared_type = CPUFREQ_SHARED_TYPE_HW;
>                 pr_info_once(PFX "overriding BIOS provided _PSD data\n");
>         }
>
Viresh Kumar June 25, 2013, 3:31 p.m. UTC | #4
On 25 June 2013 13:49, Lan Tianyu <tianyu.lan@intel.com> wrote:
> Ok. From my opinion, the new attribute is an ABI and it's better to add
> descriptor under Document directory. The user can be easy to find how to
> use it.

Hmm.. So maybe acpi-cpufreq file would be a good starting point. Then
it can have more details about the driver in future.

> Please see the commit which add the code. Maybe, we should overwrite
> shared_cpu_map by sibling_cpus for this case?

Not sure if changing shared_cpu_map has any other implications or not.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki June 25, 2013, 11:02 p.m. UTC | #5
On Tuesday, June 25, 2013 04:19:14 PM Lan Tianyu wrote:
> On 2013?06?25? 15:48, Viresh Kumar wrote:
> > On 25 June 2013 12:24, Lan Tianyu <tianyu.lan@intel.com> wrote:
> >> On 2013?06?25? 11:56, Viresh Kumar wrote:
> > 
> >>> This is generic file, don't add this information here. Add this in
> >>> acpi-cpufreq file.
> >> I don't find acpi-cpufreq.txt under
> >> Documentation/cpu-freq/acpi-cpufreq.txt. So I should create it?
> > 
> > I meant acpi-cpufreq.c file
> Ok. From my opinion, the new attribute is an ABI and it's better to add
> descriptor under Document directory. The user can be easy to find how to
> use it.

Yes, this should be documented under Documentation/ABI/testing/.

Thanks,
Rafael
Rafael Wysocki June 25, 2013, 11:03 p.m. UTC | #6
On Tuesday, June 25, 2013 09:01:03 PM Viresh Kumar wrote:
> On 25 June 2013 13:49, Lan Tianyu <tianyu.lan@intel.com> wrote:
> > Ok. From my opinion, the new attribute is an ABI and it's better to add
> > descriptor under Document directory. The user can be easy to find how to
> > use it.
> 
> Hmm.. So maybe acpi-cpufreq file would be a good starting point. Then
> it can have more details about the driver in future.
> 
> > Please see the commit which add the code. Maybe, we should overwrite
> > shared_cpu_map by sibling_cpus for this case?
> 
> Not sure if changing shared_cpu_map has any other implications or not.

Well, I wouldn't change it, then.

Thanks,
Rafael
lan,Tianyu June 26, 2013, 2:17 a.m. UTC | #7
On 2013?06?26? 07:02, Rafael J. Wysocki wrote:
> On Tuesday, June 25, 2013 04:19:14 PM Lan Tianyu wrote:
>> On 2013?06?25? 15:48, Viresh Kumar wrote:
>>> On 25 June 2013 12:24, Lan Tianyu <tianyu.lan@intel.com> wrote:
>>>> On 2013?06?25? 11:56, Viresh Kumar wrote:
>>>
>>>>> This is generic file, don't add this information here. Add this in
>>>>> acpi-cpufreq file.
>>>> I don't find acpi-cpufreq.txt under
>>>> Documentation/cpu-freq/acpi-cpufreq.txt. So I should create it?
>>>
>>> I meant acpi-cpufreq.c file
>> Ok. From my opinion, the new attribute is an ABI and it's better to add
>> descriptor under Document directory. The user can be easy to find how to
>> use it.
> 
> Yes, this should be documented under Documentation/ABI/testing/.

I find the following descriptor in the
Documentation/ABI/testing/sysfs-devices-system-cpu. So I originally put
the new attribute descriptor in the user-guide.txt. Now, creating new
acpi-cpufreq file under Documentation/cpu-freq/ and adding the
descriptor in the new file maybe a good choice?


What:           /sys/devices/system/cpu/cpu#/cpufreq/*
Date:           pre-git history
Contact:        cpufreq@vger.kernel.org
Description:    Discover and change clock speed of CPUs

                Clock scaling allows you to change the clock speed of the
                CPUs on the fly. This is a nice method to save battery
                power, because the lower the clock speed, the less power
                the CPU consumes.

                There are many knobs to tweak in this directory.

                See files in Documentation/cpu-freq/ for more information.

                In particular, read Documentation/cpu-freq/user-guide.txt
                to learn how to control the knobs.


> 
> Thanks,
> Rafael
> 
>
lan,Tianyu June 26, 2013, 2:41 a.m. UTC | #8
On 2013?06?26? 07:03, Rafael J. Wysocki wrote:
> On Tuesday, June 25, 2013 09:01:03 PM Viresh Kumar wrote:
>> On 25 June 2013 13:49, Lan Tianyu <tianyu.lan@intel.com> wrote:
>>> Ok. From my opinion, the new attribute is an ABI and it's better to add
>>> descriptor under Document directory. The user can be easy to find how to
>>> use it.
>>
>> Hmm.. So maybe acpi-cpufreq file would be a good starting point. Then
>> it can have more details about the driver in future.
>>
>>> Please see the commit which add the code. Maybe, we should overwrite
>>> shared_cpu_map by sibling_cpus for this case?
>>
>> Not sure if changing shared_cpu_map has any other implications or not.
> 
> Well, I wouldn't change it, then.
Ok. How about add new field "cpufreqdomain_cpus" in the  struct
acpi_cpufreq_data and expose its value for new attribute. For normal
case, copy the shared_cpu_map to it for normal case. For AMD case, copy
sibling_cpus to it.
   Another choice, just keeping what my patch has done. Wait for the new
request since current patch can satisfy reporter and it's not clear
whether the patch is enough for AMD platform.(At last from my view).

> 
> Thanks,
> Rafael
> 
>
Viresh Kumar June 26, 2013, 6:54 a.m. UTC | #9
On 26 June 2013 08:11, Lan Tianyu <tianyu.lan@intel.com> wrote:
> On 2013?06?26? 07:03, Rafael J. Wysocki wrote:
>> On Tuesday, June 25, 2013 09:01:03 PM Viresh Kumar wrote:
>>> On 25 June 2013 13:49, Lan Tianyu <tianyu.lan@intel.com> wrote:
>>>> Ok. From my opinion, the new attribute is an ABI and it's better to add
>>>> descriptor under Document directory. The user can be easy to find how to
>>>> use it.
>>>
>>> Hmm.. So maybe acpi-cpufreq file would be a good starting point. Then
>>> it can have more details about the driver in future.
>>>
>>>> Please see the commit which add the code. Maybe, we should overwrite
>>>> shared_cpu_map by sibling_cpus for this case?
>>>
>>> Not sure if changing shared_cpu_map has any other implications or not.
>>
>> Well, I wouldn't change it, then.

Please add a blank line before and after your reply. It makes it more
readable.

> Ok. How about add new field "cpufreqdomain_cpus" in the  struct
> acpi_cpufreq_data and expose its value for new attribute. For normal
> case, copy the shared_cpu_map to it for normal case. For AMD case, copy
> sibling_cpus to it.

This should work I guess.

>    Another choice, just keeping what my patch has done. Wait for the new
> request since current patch can satisfy reporter and it's not clear
> whether the patch is enough for AMD platform.(At last from my view).

NAK. We must keep it as it was before my patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
lan,Tianyu June 26, 2013, 6:57 a.m. UTC | #10
On 2013?06?26? 14:54, Viresh Kumar wrote:
> On 26 June 2013 08:11, Lan Tianyu <tianyu.lan@intel.com> wrote:
>> On 2013?06?26? 07:03, Rafael J. Wysocki wrote:
>>> On Tuesday, June 25, 2013 09:01:03 PM Viresh Kumar wrote:
>>>> On 25 June 2013 13:49, Lan Tianyu <tianyu.lan@intel.com> wrote:
>>>>> Ok. From my opinion, the new attribute is an ABI and it's better to add
>>>>> descriptor under Document directory. The user can be easy to find how to
>>>>> use it.
>>>>
>>>> Hmm.. So maybe acpi-cpufreq file would be a good starting point. Then
>>>> it can have more details about the driver in future.
>>>>
>>>>> Please see the commit which add the code. Maybe, we should overwrite
>>>>> shared_cpu_map by sibling_cpus for this case?
>>>>
>>>> Not sure if changing shared_cpu_map has any other implications or not.
>>>
>>> Well, I wouldn't change it, then.
> 
> Please add a blank line before and after your reply. It makes it more
> readable.

Thanks for reminder. I will notice this.

> 
>> Ok. How about add new field "cpufreqdomain_cpus" in the  struct
>> acpi_cpufreq_data and expose its value for new attribute. For normal
>> case, copy the shared_cpu_map to it for normal case. For AMD case, copy
>> sibling_cpus to it.
> 
> This should work I guess.

Ok. I will follow this one.

> 
>>    Another choice, just keeping what my patch has done. Wait for the new
>> request since current patch can satisfy reporter and it's not clear
>> whether the patch is enough for AMD platform.(At last from my view).
> 
> NAK. We must keep it as it was before my patch.
> 

OK. Please ignore this one.
diff mbox

Patch

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 937bc28..57a8774 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -730,7 +730,6 @@  static int acpi_cpufreq_cpu_init(struct
cpufreq_policy *policy)
            policy->shared_type == CPUFREQ_SHARED_TYPE_ANY) {
                cpumask_copy(policy->cpus, perf->shared_cpu_map);
        }
-       cpumask_copy(policy->related_cpus, perf->shared_cpu_map);

 #ifdef CONFIG_SMP