Re: [PATCH v2 1/5] examples/l3fwd-power: fix uncore deinit for non-legacy

["Macnamara, Chris" <chris.macnamara@intel.com>]

> Uncore resources were not being deinitialized in non-legacy modes (such
> as pmd-mgmt), causing the uncore frequency not to return to its original
> value after the application exited.
>
> The root cause is that uncore initialization can be performed for all
> modes, whereas the deinitialization logic is incorrectly restricted
> to legacy mode only. So do the deinitialization of uncore on all app
> modes.
>
> Fixes: 10db2a5b8724 ("examples/l3fwd-power: add options for uncore frequency")
> Cc: stable@dpdk.org
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>   examples/l3fwd-power/main.c | 66 ++++++++++++++++++++-----------------
>   1 file changed, 35 insertions(+), 31 deletions(-)
>
> diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
> index 02ec17d799..1122aeb930 100644
> --- a/examples/l3fwd-power/main.c
> +++ b/examples/l3fwd-power/main.c
> @@ -2271,28 +2271,31 @@ init_power_library(void)
>   	unsigned int lcore_id;
>   	int ret = 0;
>   
> -	RTE_LCORE_FOREACH(lcore_id) {
> -		/* init power management library */
> -		ret = rte_power_init(lcore_id);
> -		if (ret) {
> -			RTE_LOG(ERR, L3FWD_POWER,
> -				"Library initialization failed on core %u\n",
> -				lcore_id);
> -			return ret;
> -		}
> -		/* we're not supporting the VM channel mode */
> -		env = rte_power_get_env();
> -		if (env != PM_ENV_ACPI_CPUFREQ &&
> -				env != PM_ENV_PSTATE_CPUFREQ &&
> -				env != PM_ENV_AMD_PSTATE_CPUFREQ &&
> -				env != PM_ENV_CPPC_CPUFREQ) {
> -			RTE_LOG(ERR, L3FWD_POWER,
> -				"Only ACPI and PSTATE mode are supported\n");
> -			return -1;
> +	/* only legacy mode relies on the initialization of cpufreq library */
> +	if (app_mode == APP_MODE_LEGACY) {
> +		RTE_LCORE_FOREACH(lcore_id) {
> +			/* init power management library */
> +			ret = rte_power_init(lcore_id);
> +			if (ret) {
> +				RTE_LOG(ERR, L3FWD_POWER,
> +					"Library initialization failed on core %u\n",
> +					lcore_id);
> +				return ret;
> +			}
> +			/* we're not supporting the VM channel mode */
> +			env = rte_power_get_env();
> +			if (env != PM_ENV_ACPI_CPUFREQ &&
> +					env != PM_ENV_PSTATE_CPUFREQ &&
> +					env != PM_ENV_AMD_PSTATE_CPUFREQ &&
> +					env != PM_ENV_CPPC_CPUFREQ) {
> +				RTE_LOG(ERR, L3FWD_POWER,
> +					"Only ACPI and PSTATE mode are supported\n");
> +				return -1;
> +			}
>   		}
>   	}
>   
> -	if (cpu_resume_latency != -1) {
> +	if (app_mode == APP_MODE_LEGACY && cpu_resume_latency != -1) {
>   		RTE_LCORE_FOREACH(lcore_id) {
>   			/* Back old CPU resume latency. */
>   			ret = rte_power_qos_get_cpu_resume_latency(lcore_id);
> @@ -2329,14 +2332,16 @@ deinit_power_library(void)
>   	unsigned int lcore_id, max_pkg, max_die, die, pkg;
>   	int ret = 0;
>   
> -	RTE_LCORE_FOREACH(lcore_id) {
> -		/* deinit power management library */
> -		ret = rte_power_exit(lcore_id);
> -		if (ret) {
> -			RTE_LOG(ERR, L3FWD_POWER,
> -				"Library deinitialization failed on core %u\n",
> -				lcore_id);
> -			return ret;
> +	if (app_mode == APP_MODE_LEGACY) {
> +		RTE_LCORE_FOREACH(lcore_id) {
> +			/* deinit power management library */
> +			ret = rte_power_exit(lcore_id);
> +			if (ret) {
> +				RTE_LOG(ERR, L3FWD_POWER,
> +					"Library deinitialization failed on core %u\n",
> +					lcore_id);
> +				return ret;
> +			}
>   		}
>   	}
>   
> @@ -2360,7 +2365,7 @@ deinit_power_library(void)
>   		}
>   	}
>   
> -	if (cpu_resume_latency != -1) {
> +	if (app_mode == APP_MODE_LEGACY && cpu_resume_latency != -1) {
>   		RTE_LCORE_FOREACH(lcore_id) {
>   			/* Restore the original value. */
>   			rte_power_qos_set_cpu_resume_latency(lcore_id,

Agree there is an issue with uncore deinit.

cpu_resume_latency & init_power_library: After this patch, 
cpu_resume_latency (for C-state PM QoS control) is gated behind 
APP_MODE_LEGACY even though it's orthogonal to the cpufreq library, so 
it's silently ignored in pmd-mgmt/interrupt modes. It’s used for C state 
management which applies in other modes such as interrupt mode.

Also init_power_library() now has both its blocks legacy-guarded 
internally, the newly-unconditional call by removing APP_MODE_LEGACY to 
it is a no-op in non-legacy modes. This leaves the two functions 
inconsistent: init_power_library() does nothing outside legacy mode, 
whereas deinit_power_library() still has real work to do in every mode 
because of the uncore cleanup.

Consider, keep the APP_MODE_LEGACY guard at the init call site as the 
init is truly legacy only or split the uncore teardown into its own 
deinit_uncore() so that deinit_power_library() stays strictly about 
cpufreq.