Re: [dpdk-dev] [PATCH] app/test: enhance power manager unit tests

["Hunt, David" <david.hunt@intel.com>]

Hi Reshma,


On 6/4/2018 2:51 PM, Reshma Pattan wrote:
> Unit Testcases are added for power_acpi_cpu_freq,
> power_kvm_vm_test to improve coverage
>
> Signed-off-by: Jananee Parthasarathy <jananeex.m.parthasarathy@intel.com>
> ---
>   test/test/test_power_acpi_cpufreq.c |  2 +-
>   test/test/test_power_kvm_vm.c       | 62 +++++++++++++++++++++++++++++++++----
>   2 files changed, 57 insertions(+), 7 deletions(-)
>
> diff --git a/test/test/test_power_acpi_cpufreq.c b/test/test/test_power_acpi_cpufreq.c
> index 3bfd033..8da2dcc 100644
> --- a/test/test/test_power_acpi_cpufreq.c
> +++ b/test/test/test_power_acpi_cpufreq.c
> @@ -27,7 +27,7 @@
>   #define TEST_POWER_FREQS_NUM_MAX ((unsigned)RTE_MAX_LCORE_FREQS)
>   
>   #define TEST_POWER_SYSFILE_CUR_FREQ \
> -	"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_cur_freq"
> +	"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_cur_freq"

This change is OK with me, from what I can see using cpuinfo_cur_freq 
instead of
scaling_cpu_freq gives us more compatibility on a wider selection of 
operating systems.

>   
>   static uint32_t total_freq_num;
>   static uint32_t freqs[TEST_POWER_FREQS_NUM_MAX];
> diff --git a/test/test/test_power_kvm_vm.c b/test/test/test_power_kvm_vm.c
> index 91b31c4..012ad82 100644
> --- a/test/test/test_power_kvm_vm.c
> +++ b/test/test/test_power_kvm_vm.c
> @@ -25,12 +25,19 @@
>   #define TEST_POWER_VM_LCORE_ID            0U
>   #define TEST_POWER_VM_LCORE_OUT_OF_BOUNDS (RTE_MAX_LCORE+1)
>   #define TEST_POWER_VM_LCORE_INVALID       1U
> +#define TEMP_POWER_MANAGER_FILE_PATH  "/tmp/testpm"
> +
> +int guest_channel_host_connect(const char *path, unsigned int lcore_id);
> +int power_kvm_vm_enable_turbo(unsigned int lcore_id);
> +int power_kvm_vm_disable_turbo(unsigned int lcore_id);

I see here you are calling guest_channel_host_connect to "emulate" a 
virtio-serial connection
to a host. While I am not a huge fan of faking functionality, I feel 
that having these unit tests
check ABI and API breakages is more beneficial, so I'm good with it for 
this reason.

However, there's no need to have the power_kvm_vm_enable/disable_turbo() 
prototypes, as you
can just use rte_power_freq_enable_turbo() and 
rte_power_freq_disable_turbo(), which in turn
call power_kvm_vm_enable_turbo() and power_kvm_vm_disable_turbo()

>   
>   static int
>   test_power_kvm_vm(void)
>   {
>   	int ret;
>   	enum power_management_env env;
> +	char fPath[PATH_MAX];
> +	FILE *fPtr = NULL;
>   
>   	ret = rte_power_set_env(PM_ENV_KVM_VM);
>   	if (ret != 0) {
> @@ -95,12 +102,31 @@
>   	/* Test initialisation of a valid lcore */
>   	ret = rte_power_init(TEST_POWER_VM_LCORE_ID);
>   	if (ret < 0) {
> -		printf("Cannot initialise power management for lcore %u, this "
> -				"may occur if environment is not configured "
> -				"correctly(KVM VM) or operating in another valid "
> -				"Power management environment\n", TEST_POWER_VM_LCORE_ID);
> -		rte_power_unset_env();
> -		return -1;
> +		printf("rte_power_init failed as expected in host\n");
> +		/* This test would be successful when run on VM,
> +		 * in order to run in Host itself, temporary file path
> +		 * is created and same is used for further communication
> +		 */
> +
> +		snprintf(fPath, PATH_MAX, "%s.%u",
> +			TEMP_POWER_MANAGER_FILE_PATH, TEST_POWER_VM_LCORE_ID);
> +		fPtr = fopen(fPath, "w");
> +		if (fPtr == NULL) {
> +			printf(" Unable to create file\n");
> +			rte_power_unset_env();
> +			return -1;
> +		}
> +		ret = guest_channel_host_connect(TEMP_POWER_MANAGER_FILE_PATH,
> +			TEST_POWER_VM_LCORE_ID);
> +		if (ret == 0)
> +			printf("guest_channel_host_connect successful\n");
> +		else {
> +			printf("guest_channel_host_connect failed\n");
> +			rte_power_unset_env();
> +			fclose(fPtr);
> +			remove(fPath);
> +			return -1;
> +		}
>   	}
>   
>   	/* Test initialisation of previously initialised lcore */
> @@ -175,6 +201,22 @@
>   		goto fail_all;
>   	}
>   
> +	/* Test KVM_VM Enable Turbo of valid core */
> +	ret = power_kvm_vm_enable_turbo(TEST_POWER_VM_LCORE_ID);

see comment above about using rte_power_freq_enable_turbo()

> +	if (ret == -1) {
> +		printf("power_kvm_vm_enable_turbo failed on valid lcore"
> +			"%u\n", TEST_POWER_VM_LCORE_ID);
> +		goto fail_all;
> +	}
> +
> +	/* Test KVM_VM Disable Turbo of valid core */
> +	ret = power_kvm_vm_disable_turbo(TEST_POWER_VM_LCORE_ID);

see comment above about using rte_power_freq_disable_turbo()

> +	if (ret == -1) {
> +		printf("power_kvm_vm_disable_turbo failed on valid lcore"
> +		"%u\n", TEST_POWER_VM_LCORE_ID);
> +		goto fail_all;
> +	}
> +
>   	/* Test frequency up of valid lcore */
>   	ret = rte_power_freq_up(TEST_POWER_VM_LCORE_ID);
>   	if (ret != 1) {
> @@ -274,10 +316,18 @@
>   		return -1;
>   	}
>   	rte_power_unset_env();
> +	if (fPtr != NULL) {
> +		fclose(fPtr);
> +		remove(fPath);
> +	}
>   	return 0;
>   fail_all:
>   	rte_power_exit(TEST_POWER_VM_LCORE_ID);
>   	rte_power_unset_env();
> +	if (fPtr != NULL) {
> +		fclose(fPtr);
> +		remove(fPath);
> +	}
>   	return -1;
>   }
>   #endif

With the changes described above:

Acked-by: David Hunt <david.hunt@intel.com>