2022-05-05 17:01:02

by Huang Rui

[permalink] [raw]
Subject: Re: [PATCH V5 0/3] Add unit test module for AMD P-State driver

On Thu, May 05, 2022 at 10:29:10AM +0800, Meng, Li (Jassmine) wrote:
> Hi all:
>
> AMD P-State unit test(amd-pstate-ut) is a kernel module for testing
> the functions of amd-pstate driver.
> It could import as a module to launch some test tasks.
> 1) It can help all users to verify their processor support (SBIOS/
> Firmware or Hardware).
> 2) Kernel can have a basic function test to avoid the kernel regression
> during the update.
> 3) We can introduce more functional or performance tests to align the
> together, it will benefit power and performance scale optimization.
>
> We upstream out AMD P-state driver into Linux kernel and use this unit
> test module to verify the required conditions and basic functions of
> amd-pstate before integration test.
>
> We use test module in the kselftest frameworks to implement it.
> We create amd-pstate-ut module and tie it into kselftest.
>
> For example: The test case aput_acpi_cpc is used to check whether the
> _CPC object is exist in SBIOS.
> The amd-pstate initialization will fail if the _CPC in ACPI SBIOS is not
> existed at the detected processor, so it is a necessary condition.
>
> At present, it only implements the basic framework and some simple test
> cases.
>
> TODO : 1) we will add more test cases to improve the depth and coverage of
> the test.
>
> Please check the documentation amd-pstate.rst for details of the test steps.
>
> See patch series in below git repo:
> V1: https://lore.kernel.org/linux-pm/[email protected]/
> V2: https://lore.kernel.org/lkml/[email protected]/
> V3: https://lore.kernel.org/lkml/[email protected]/
> V4: https://lore.kernel.org/lkml/[email protected]/
>
> Changes from V1 -> V2:
> - cpufreq: amd-pstate:
> - - add a trailing of amd-pstate.h to MAINTAINER AMD PSTATE DRIVER.
> - selftests: cpufreq:
> - - add a wrapper shell script for the amd_pstate_testmod module.
> - selftests: cpufreq:
> - - remove amd_pstate_testmod kernel module to
> .../cpufreq/amd_pstate_testmod.
> - Documentation: amd-pstate:
> - - amd_pstate_testmod rst document is not provided at present.
>
> Changes from V2 -> V3:
> - cpufreq: amd-pstate:
> - - adjust the order of add amd-pstate.h in MAINTAINERS.
> - selftests: cpufreq:
> - - remove the call of amd_pstate_testmod.sh from cpufreq Makefile to
> main.sh.
> - selftests: cpufreq:
> - - add explain the goal or intention of the AMD P-State Unit Test
> module.
> - - modify comments.
> - - use the checkpatch.pl to check my patches.
> - - add conditions judgment before formal test.
> - - delete some unnecessary test cases.
> - - modify test cases about perf and performance etc.
>
> Changes from V3 -> V4:
> - selftests: amd-pstate:
> - - remove script and test module to tools/testing/selftests/amd-pstate/
> - - uniformly named amd-pstate-ut.
> - - check current architectures and cpufreq driver in amd-pstate-ut.sh
> - - delete codes about conditions in amd-pstate-ut.c
> - Documentation: amd-pstate:
> - - add introduce document about amd-pstate unit test.
>
> Changes from V4 -> V5:
> - selftests: amd-pstate:
> - - add print the current scaling_driver.
> - - add amd-pstate-ut.ko into TEST_GEN_FILES.
> - - move "insmod/rmmod amd-pstate-ut.ko" stuff into script amd_pstate_ut.sh
> - - add a check of read back from X86_FEATURE_CPPC in get_shared_mem().
> - Documentation: amd-pstate:
> - - delete the test step about insmod/rmmod amd-pstate-ut.ko
>
> Thanks,
> Jasmine
>
> Meng Li (3):
> cpufreq: amd-pstate: Expose struct amd_cpudata
> selftests: amd-pstate: Add test module for amd-pstate driver
> Documentation: amd-pstate: Add unit test introduction

Series are Acked-by: Huang Rui <[email protected]>

Thanks Jassmine! Let's wait for Khan's review comments.

Best Regards,
Ray

>
> Documentation/admin-guide/pm/amd-pstate.rst | 85 ++++++
> MAINTAINERS | 1 +
> drivers/cpufreq/amd-pstate.c | 60 +---
> include/linux/amd-pstate.h | 77 +++++
> tools/testing/selftests/Makefile | 1 +
> tools/testing/selftests/amd-pstate/Makefile | 15 +
> .../selftests/amd-pstate/amd-pstate-ut.sh | 45 +++
> .../amd-pstate/amd-pstate-ut/Makefile | 20 ++
> .../amd-pstate/amd-pstate-ut/amd-pstate-ut.c | 278 ++++++++++++++++++
> tools/testing/selftests/amd-pstate/config | 1 +
> 10 files changed, 524 insertions(+), 59 deletions(-)
> create mode 100644 include/linux/amd-pstate.h
> create mode 100644 tools/testing/selftests/amd-pstate/Makefile
> create mode 100755 tools/testing/selftests/amd-pstate/amd-pstate-ut.sh
> create mode 100644 tools/testing/selftests/amd-pstate/amd-pstate-ut/Makefile
> create mode 100644 tools/testing/selftests/amd-pstate/amd-pstate-ut/amd-pstate-ut.c
> create mode 100644 tools/testing/selftests/amd-pstate/config
>
> --
> 2.25.1
>


2022-05-17 19:51:35

by Shuah Khan

[permalink] [raw]
Subject: Re: [PATCH V5 0/3] Add unit test module for AMD P-State driver

On 5/5/22 12:41 AM, Huang Rui wrote:
> On Thu, May 05, 2022 at 10:29:10AM +0800, Meng, Li (Jassmine) wrote:
>> Hi all:
>>
>> AMD P-State unit test(amd-pstate-ut) is a kernel module for testing
>> the functions of amd-pstate driver.
>> It could import as a module to launch some test tasks.
>> 1) It can help all users to verify their processor support (SBIOS/
>> Firmware or Hardware).
>> 2) Kernel can have a basic function test to avoid the kernel regression
>> during the update.
>> 3) We can introduce more functional or performance tests to align the
>> together, it will benefit power and performance scale optimization.
>>
>> We upstream out AMD P-state driver into Linux kernel and use this unit
>> test module to verify the required conditions and basic functions of
>> amd-pstate before integration test.
>>
>> We use test module in the kselftest frameworks to implement it.
>> We create amd-pstate-ut module and tie it into kselftest.
>>
>> For example: The test case aput_acpi_cpc is used to check whether the
>> _CPC object is exist in SBIOS.
>> The amd-pstate initialization will fail if the _CPC in ACPI SBIOS is not
>> existed at the detected processor, so it is a necessary condition.
>>
>> At present, it only implements the basic framework and some simple test
>> cases.
>>
>> TODO : 1) we will add more test cases to improve the depth and coverage of
>> the test.
>>
>> Please check the documentation amd-pstate.rst for details of the test steps.
>>
>> See patch series in below git repo:
>> V1: https://lore.kernel.org/linux-pm/[email protected]/
>> V2: https://lore.kernel.org/lkml/[email protected]/
>> V3: https://lore.kernel.org/lkml/[email protected]/
>> V4: https://lore.kernel.org/lkml/[email protected]/
>>

>> Changes from V4 -> V5:
>> - selftests: amd-pstate:
>> - - add print the current scaling_driver.
>> - - add amd-pstate-ut.ko into TEST_GEN_FILES.
>> - - move "insmod/rmmod amd-pstate-ut.ko" stuff into script amd_pstate_ut.sh
>> - - add a check of read back from X86_FEATURE_CPPC in get_shared_mem().
>> - Documentation: amd-pstate:
>> - - delete the test step about insmod/rmmod amd-pstate-ut.ko
>>
>> Thanks,
>> Jasmine
>>

Sorry for the delay on this. I will review the series in the next couple
of days.

Did you consider using KUnit for this? I think asked that question when
reviewing the previous version.

thanks,
-- Shuah

2022-05-18 04:00:50

by Shuah Khan

[permalink] [raw]
Subject: Re: [PATCH V5 0/3] Add unit test module for AMD P-State driver

On 5/17/22 8:45 AM, Shuah Khan wrote:
> On 5/5/22 12:41 AM, Huang Rui wrote:
>> On Thu, May 05, 2022 at 10:29:10AM +0800, Meng, Li (Jassmine) wrote:
>>> Hi all:
>>>
>>> AMD P-State unit test(amd-pstate-ut) is a kernel module for testing
>>> the functions of amd-pstate driver.
>>> It could import as a module to launch some test tasks.
>>> 1) It can help all users to verify their processor support (SBIOS/
>>> Firmware or Hardware).
>>> 2) Kernel can have a basic function test to avoid the kernel regression
>>> during the update.
>>> 3) We can introduce more functional or performance tests to align the
>>> together, it will benefit power and performance scale optimization.
>>>
>>> We upstream out AMD P-state driver into Linux kernel and use this unit
>>> test module to verify the required conditions and basic functions of
>>> amd-pstate before integration test.
>>>
>>> We use test module in the kselftest frameworks to implement it.
>>> We create amd-pstate-ut module and tie it into kselftest.
>>>
>>> For example: The test case aput_acpi_cpc is used to check whether the
>>> _CPC object is exist in SBIOS.
>>> The amd-pstate initialization will fail if the _CPC in ACPI SBIOS is not
>>> existed at the detected processor, so it is a necessary condition.
>>>
>>> At present, it only implements the basic framework and some simple test
>>> cases.
>>>
>>> TODO : 1) we will add more test cases to improve the depth and coverage of
>>> the test.
>>>
>>> Please check the documentation amd-pstate.rst for details of the test steps.
>>>
>>> See patch series in below git repo:
>>> V1: https://lore.kernel.org/linux-pm/[email protected]/
>>> V2: https://lore.kernel.org/lkml/[email protected]/
>>> V3: https://lore.kernel.org/lkml/[email protected]/
>>> V4: https://lore.kernel.org/lkml/[email protected]/
>>>
>
>>> Changes from V4 -> V5:
>>> - selftests: amd-pstate:
>>> - - add print the current scaling_driver.
>>> - - add amd-pstate-ut.ko into TEST_GEN_FILES.
>>> - - move "insmod/rmmod amd-pstate-ut.ko" stuff into script amd_pstate_ut.sh
>>> - - add a check of read back from X86_FEATURE_CPPC in get_shared_mem().
>>> - Documentation: amd-pstate:
>>> - - delete the test step about insmod/rmmod amd-pstate-ut.ko
>>>
>>> Thanks,
>>> Jasmine
>>>
>
> Sorry for the delay on this. I will review the series in the next couple
> of days.
>
> Did you consider using KUnit for this? I think asked that question when
> reviewing the previous version.
>

I reviewed the patches and the test driver amd-pstate-ut doesn't belong under
selftests. I would recommend the following approach:

- add this test driver under drivers/cpufreq

- KUnit is a better fit for this unit test driver unless you want
to be able to run this without KUnit configured

- add the test script under selftests - the script then can load the
test driver and run tests and unload the driver.

thanks,
-- Shuah


2022-05-18 05:40:22

by Huang Rui

[permalink] [raw]
Subject: Re: [PATCH V5 0/3] Add unit test module for AMD P-State driver

On Wed, May 18, 2022 at 04:03:05AM +0800, Shuah Khan wrote:
> On 5/17/22 8:45 AM, Shuah Khan wrote:
> > On 5/5/22 12:41 AM, Huang Rui wrote:
> >> On Thu, May 05, 2022 at 10:29:10AM +0800, Meng, Li (Jassmine) wrote:
> >>> Hi all:
> >>>
> >>> AMD P-State unit test(amd-pstate-ut) is a kernel module for testing
> >>> the functions of amd-pstate driver.
> >>> It could import as a module to launch some test tasks.
> >>> 1) It can help all users to verify their processor support (SBIOS/
> >>> Firmware or Hardware).
> >>> 2) Kernel can have a basic function test to avoid the kernel regression
> >>> during the update.
> >>> 3) We can introduce more functional or performance tests to align the
> >>> together, it will benefit power and performance scale optimization.
> >>>
> >>> We upstream out AMD P-state driver into Linux kernel and use this unit
> >>> test module to verify the required conditions and basic functions of
> >>> amd-pstate before integration test.
> >>>
> >>> We use test module in the kselftest frameworks to implement it.
> >>> We create amd-pstate-ut module and tie it into kselftest.
> >>>
> >>> For example: The test case aput_acpi_cpc is used to check whether the
> >>> _CPC object is exist in SBIOS.
> >>> The amd-pstate initialization will fail if the _CPC in ACPI SBIOS is not
> >>> existed at the detected processor, so it is a necessary condition.
> >>>
> >>> At present, it only implements the basic framework and some simple test
> >>> cases.
> >>>
> >>> TODO : 1) we will add more test cases to improve the depth and coverage of
> >>> the test.
> >>>
> >>> Please check the documentation amd-pstate.rst for details of the test steps.
> >>>
> >>> See patch series in below git repo:
> >>> V1: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-pm%2F20220323071502.2674156-1-li.meng%40amd.com%2F&amp;data=05%7C01%7Cray.huang%40amd.com%7C267a994f51e34371021c08da384042f1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637884145911708064%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=m9BlIFlf8IEeSGjycWdppnIMoZTbfmi3tIsuRJZL6cw%3D&amp;reserved=0
> >>> V2: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20220413090510.4039589-1-li.meng%40amd.com%2F&amp;data=05%7C01%7Cray.huang%40amd.com%7C267a994f51e34371021c08da384042f1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637884145911708064%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=h1o69SJ5iaVChb1h8a3FZURAeKAj6vlB%2FmR%2BtfmpoE4%3D&amp;reserved=0
> >>> V3: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20220421074152.599419-1-li.meng%40amd.com%2F&amp;data=05%7C01%7Cray.huang%40amd.com%7C267a994f51e34371021c08da384042f1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637884145911708064%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=WcXo7q2snNWM8nUSF08HjPDB7UuwFMDTTwe6zLcXHyY%3D&amp;reserved=0
> >>> V4: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20220427135315.3447550-1-li.meng%40amd.com%2F&amp;data=05%7C01%7Cray.huang%40amd.com%7C267a994f51e34371021c08da384042f1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637884145911708064%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=Rnn9bLhc1sDkNyBlmwmb8Zy4IJUKwtIq0M0qhcUbdM8%3D&amp;reserved=0
> >>>
> >
> >>> Changes from V4 -> V5:
> >>> - selftests: amd-pstate:
> >>> - - add print the current scaling_driver.
> >>> - - add amd-pstate-ut.ko into TEST_GEN_FILES.
> >>> - - move "insmod/rmmod amd-pstate-ut.ko" stuff into script amd_pstate_ut.sh
> >>> - - add a check of read back from X86_FEATURE_CPPC in get_shared_mem().
> >>> - Documentation: amd-pstate:
> >>> - - delete the test step about insmod/rmmod amd-pstate-ut.ko
> >>>
> >>> Thanks,
> >>> Jasmine
> >>>
> >
> > Sorry for the delay on this. I will review the series in the next couple
> > of days.
> >
> > Did you consider using KUnit for this? I think asked that question when
> > reviewing the previous version.
> >
>
> I reviewed the patches and the test driver amd-pstate-ut doesn't belong under
> selftests. I would recommend the following approach:
>
> - add this test driver under drivers/cpufreq
>
> - KUnit is a better fit for this unit test driver unless you want
> to be able to run this without KUnit configured
>
> - add the test script under selftests - the script then can load the
> test driver and run tests and unload the driver.
>

Thanks Shuah. We would like to introduce more CPU benchmark testing based
the amd-pstate-ut next step, it may not be all the stuff in the kernel
module. E.X. use the script to trigger the tbench, gitsource, kernbench,
netperf, speedometer, and etc. testing and monitor the cpu frequency and
performance goals change, power consumption at runtime. Can the KUnit +
Kselftests work more straight forward in user scenario?

Thanks,
Ray

2022-05-18 20:04:28

by Shuah Khan

[permalink] [raw]
Subject: Re: [PATCH V5 0/3] Add unit test module for AMD P-State driver

On 5/17/22 11:19 PM, Huang Rui wrote:
> On Wed, May 18, 2022 at 04:03:05AM +0800, Shuah Khan wrote:

>> I reviewed the patches and the test driver amd-pstate-ut doesn't belong under
>> selftests. I would recommend the following approach:
>>
>> - add this test driver under drivers/cpufreq
>>
>> - KUnit is a better fit for this unit test driver unless you want
>> to be able to run this without KUnit configured
>>
>> - add the test script under selftests - the script then can load the
>> test driver and run tests and unload the driver.
>>
>
> Thanks Shuah. We would like to introduce more CPU benchmark testing based
> the amd-pstate-ut next step, it may not be all the stuff in the kernel
> module. E.X. use the script to trigger the tbench, gitsource, kernbench,
> netperf, speedometer, and etc. testing and monitor the cpu frequency and
> performance goals change, power consumption at runtime. Can the KUnit +
> Kselftests work more straight forward in user scenario?
>

I am not sure if I understand your question. Also I am not sure if understand
the scope of the testing you are intending to do using amd-pstate-ut.

tbench, gitsource etc. are external tests (meaning not in kernel repo). Do these
tests depend on amd-pstate-ut?

It would be helpful to understand the scope first. What exactly will amd-pstate-ut
used for? Based on the patch series it looked like:

-- amd-pstate-ut has unit test code which will be triggered from kselftest

This is perfectly fine. The driver can reside under drivers/cpufreq and a test.sh
can reside under selftests. It can just use kselftest framework.

Based on what you are saying. kselftest might be a good choice.

thanks,
-- Shuah