From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id F3FCA1B8AA for ; Tue, 10 Apr 2018 16:22:22 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Apr 2018 07:18:22 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,432,1517904000"; d="scan'208";a="31000950" Received: from dhunt5-mobl2.ger.corp.intel.com (HELO [10.237.221.55]) ([10.237.221.55]) by fmsmga007.fm.intel.com with ESMTP; 10 Apr 2018 07:18:22 -0700 To: Reshma Pattan , dev@dpdk.org Cc: jananeex.m.parthasarathy@intel.com References: <1523022706-12231-1-git-send-email-reshma.pattan@intel.com> From: "Hunt, David" Message-ID: <8daadb27-8c2a-465e-19f5-cbe5a7451b70@intel.com> Date: Tue, 10 Apr 2018 15:19:04 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <1523022706-12231-1-git-send-email-reshma.pattan@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH] app/test: enhance power manager unit tests X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Apr 2018 14:22:23 -0000 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 > --- > 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