examples/l3fwd-power: fix to configure the uncore env

Message ID 20231026151959.1458112-1-sivaprasad.tummala@amd.com (mailing list archive)
State Accepted
Delegated to: Thomas Monjalon
Headers
Series examples/l3fwd-power: fix to configure the uncore env |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/github-robot: build success github build: passed
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-sample-apps-testing success Testing PASS

Commit Message

Sivaprasad Tummala Oct. 26, 2023, 3:19 p.m. UTC
Updated the l3fwd-power app to configure the uncore env before invoking
any uncore APIs. With auto-detection in 'rte_power_uncore_init()' it is
too late because other APIs already called.

Bugzilla ID: 1304
Fixes: ac1edcb6621a ("power: refactor uncore power management API")
Cc: karen.kelly@intel.com

Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
---
 examples/l3fwd-power/main.c  | 6 ++++++
 lib/power/rte_power_uncore.c | 7 +++++++
 lib/power/rte_power_uncore.h | 9 +++++----
 3 files changed, 18 insertions(+), 4 deletions(-)
  

Comments

Karen Kelly Oct. 27, 2023, 12:36 p.m. UTC | #1
On 26/10/2023 16:19, Sivaprasad Tummala wrote:
> Updated the l3fwd-power app to configure the uncore env before invoking
> any uncore APIs. With auto-detection in 'rte_power_uncore_init()' it is
> too late because other APIs already called.
>
> Bugzilla ID: 1304
> Fixes: ac1edcb6621a ("power: refactor uncore power management API")
> Cc:karen.kelly@intel.com
>
> Signed-off-by: Sivaprasad Tummala<sivaprasad.tummala@amd.com>
> ---
>   examples/l3fwd-power/main.c  | 6 ++++++
>   lib/power/rte_power_uncore.c | 7 +++++++
>   lib/power/rte_power_uncore.h | 9 +++++----
>   3 files changed, 18 insertions(+), 4 deletions(-)
Ran on my system, bug is resolved after applying this patch.

Tested-by: Karen Kelly <karen.kelly@intel.com>
  
Hunt, David Oct. 27, 2023, 12:37 p.m. UTC | #2
Hi Sivaprasad, Karen,

On 27/10/2023 13:36, Kelly, Karen wrote:
>
>
> On 26/10/2023 16:19, Sivaprasad Tummala wrote:
>> Updated the l3fwd-power app to configure the uncore env before invoking
>> any uncore APIs. With auto-detection in 'rte_power_uncore_init()' it is
>> too late because other APIs already called.
>>
>> Bugzilla ID: 1304
>> Fixes: ac1edcb6621a ("power: refactor uncore power management API")
>> Cc:karen.kelly@intel.com
>>
>> Signed-off-by: Sivaprasad Tummala<sivaprasad.tummala@amd.com>
>> ---
>>   examples/l3fwd-power/main.c  | 6 ++++++
>>   lib/power/rte_power_uncore.c | 7 +++++++
>>   lib/power/rte_power_uncore.h | 9 +++++----
>>   3 files changed, 18 insertions(+), 4 deletions(-)
> Ran on my system, bug is resolved after applying this patch.
>
> Tested-by: Karen Kelly <karen.kelly@intel.com>
>

Acked-by: David Hunt <david.hunt@intel.com>
  
Thomas Monjalon Nov. 23, 2023, 1:58 a.m. UTC | #3
26/10/2023 17:19, Sivaprasad Tummala:
> Updated the l3fwd-power app to configure the uncore env before invoking
> any uncore APIs. With auto-detection in 'rte_power_uncore_init()' it is
> too late because other APIs already called.

You are also updating the uncore API.

> +	if (env == RTE_UNCORE_PM_ENV_AUTO_DETECT)
> +		/* Currently only intel_uncore is supported. This will be
> +		 * extended with auto-detection support for multiple uncore
> +		 * implementations.
> +		 */
> +		env = RTE_UNCORE_PM_ENV_INTEL_UNCORE;

It looks like this patch does not make sense without AMD support.
  
Ferruh Yigit Nov. 23, 2023, 10:26 a.m. UTC | #4
On 11/23/2023 1:58 AM, Thomas Monjalon wrote:
> 26/10/2023 17:19, Sivaprasad Tummala:
>> Updated the l3fwd-power app to configure the uncore env before invoking
>> any uncore APIs. With auto-detection in 'rte_power_uncore_init()' it is
>> too late because other APIs already called.
> 
> You are also updating the uncore API.
> 
>> +	if (env == RTE_UNCORE_PM_ENV_AUTO_DETECT)
>> +		/* Currently only intel_uncore is supported. This will be
>> +		 * extended with auto-detection support for multiple uncore
>> +		 * implementations.
>> +		 */
>> +		env = RTE_UNCORE_PM_ENV_INTEL_UNCORE;
> 
> It looks like this patch does not make sense without AMD support.
> 

Yes, right now auto detect directly fallback to Intel, but there is an
intention to add AMD support too, that is why instead directly using
Intel preferred the auto detection abstraction.
  
Bruce Richardson Nov. 27, 2023, 4:54 p.m. UTC | #5
On Thu, Nov 23, 2023 at 02:58:58AM +0100, Thomas Monjalon wrote:
> 26/10/2023 17:19, Sivaprasad Tummala:
> > Updated the l3fwd-power app to configure the uncore env before invoking
> > any uncore APIs. With auto-detection in 'rte_power_uncore_init()' it is
> > too late because other APIs already called.
> 
> You are also updating the uncore API.
> 
> > +	if (env == RTE_UNCORE_PM_ENV_AUTO_DETECT)
> > +		/* Currently only intel_uncore is supported. This will be
> > +		 * extended with auto-detection support for multiple uncore
> > +		 * implementations.
> > +		 */
> > +		env = RTE_UNCORE_PM_ENV_INTEL_UNCORE;
> 
> It looks like this patch does not make sense without AMD support.
> 
This patch is fixing a regression introduced by an earlier patch in this
area (referenced in the fixes line). See bugzilla for more details on it[1]

This should go into 23.11 as, without it, what was working in earlier
releases no longer does so.

Thanks,
/Bruce

[1] https://bugs.dpdk.org/show_bug.cgi?id=1304
  
Thomas Monjalon Nov. 27, 2023, 9:13 p.m. UTC | #6
27/11/2023 17:54, Bruce Richardson:
> On Thu, Nov 23, 2023 at 02:58:58AM +0100, Thomas Monjalon wrote:
> > 26/10/2023 17:19, Sivaprasad Tummala:
> > > Updated the l3fwd-power app to configure the uncore env before invoking
> > > any uncore APIs. With auto-detection in 'rte_power_uncore_init()' it is
> > > too late because other APIs already called.
> > 
> > You are also updating the uncore API.
> > 
> > > +	if (env == RTE_UNCORE_PM_ENV_AUTO_DETECT)
> > > +		/* Currently only intel_uncore is supported. This will be
> > > +		 * extended with auto-detection support for multiple uncore
> > > +		 * implementations.
> > > +		 */
> > > +		env = RTE_UNCORE_PM_ENV_INTEL_UNCORE;
> > 
> > It looks like this patch does not make sense without AMD support.
> > 
> This patch is fixing a regression introduced by an earlier patch in this
> area (referenced in the fixes line). See bugzilla for more details on it[1]
> 
> This should go into 23.11 as, without it, what was working in earlier
> releases no longer does so.

Thank you for the heads-up, I've looked at it too much quickly.
  
Thomas Monjalon Nov. 27, 2023, 9:19 p.m. UTC | #7
27/10/2023 14:37, Hunt, David:
> On 27/10/2023 13:36, Kelly, Karen wrote:
> > On 26/10/2023 16:19, Sivaprasad Tummala wrote:
> >> Updated the l3fwd-power app to configure the uncore env before invoking
> >> any uncore APIs. With auto-detection in 'rte_power_uncore_init()' it is
> >> too late because other APIs already called.
> >>
> >> Bugzilla ID: 1304
> >> Fixes: ac1edcb6621a ("power: refactor uncore power management API")
> >> Cc:karen.kelly@intel.com
> >>
> >> Signed-off-by: Sivaprasad Tummala<sivaprasad.tummala@amd.com>
> >> ---
> >>   examples/l3fwd-power/main.c  | 6 ++++++
> >>   lib/power/rte_power_uncore.c | 7 +++++++
> >>   lib/power/rte_power_uncore.h | 9 +++++----
> >>   3 files changed, 18 insertions(+), 4 deletions(-)
> > Ran on my system, bug is resolved after applying this patch.
> >
> > Tested-by: Karen Kelly <karen.kelly@intel.com>
> >
> 
> Acked-by: David Hunt <david.hunt@intel.com>

So I merge it to fix a bug in the library, despite the patch title.
I am fixing a typo, and the commit description.
Applied
  

Patch

diff --git a/examples/l3fwd-power/main.c b/examples/l3fwd-power/main.c
index 5fbc16bb2a..9c0dcd343b 100644
--- a/examples/l3fwd-power/main.c
+++ b/examples/l3fwd-power/main.c
@@ -1561,6 +1561,12 @@  parse_uncore_options(enum uncore_choice choice, const char *argument)
 {
 	unsigned int die, pkg, max_pkg, max_die;
 	int ret = 0;
+	ret = rte_power_set_uncore_env(RTE_UNCORE_PM_ENV_AUTO_DETECT);
+	if (ret < 0) {
+		RTE_LOG(INFO, L3FWD_POWER, "Failed to set uncore env\n");
+		return ret;
+	}
+
 	max_pkg = rte_power_uncore_get_num_pkgs();
 	if (max_pkg == 0)
 		return -1;
diff --git a/lib/power/rte_power_uncore.c b/lib/power/rte_power_uncore.c
index 78a823d34c..d87328bedf 100644
--- a/lib/power/rte_power_uncore.c
+++ b/lib/power/rte_power_uncore.c
@@ -105,6 +105,13 @@  rte_power_set_uncore_env(enum rte_uncore_power_mgmt_env env)
 		return -1;
 	}
 
+	if (env == RTE_UNCORE_PM_ENV_AUTO_DETECT)
+		/* Currently only intel_uncore is supported. This will be
+		 * extended with auto-detection support for multiple uncore
+		 * implementations.
+		 */
+		env = RTE_UNCORE_PM_ENV_INTEL_UNCORE;
+
 	ret = 0;
 	if (env == RTE_UNCORE_PM_ENV_INTEL_UNCORE) {
 		rte_power_get_uncore_freq = power_get_intel_uncore_freq;
diff --git a/lib/power/rte_power_uncore.h b/lib/power/rte_power_uncore.h
index 295017b7f4..ede50a5f87 100644
--- a/lib/power/rte_power_uncore.h
+++ b/lib/power/rte_power_uncore.h
@@ -21,15 +21,16 @@  extern "C" {
 /* Uncore Power Management Environment */
 enum rte_uncore_power_mgmt_env {
 	RTE_UNCORE_PM_ENV_NOT_SET,
+	RTE_UNCORE_PM_ENV_AUTO_DETECT,
 	RTE_UNCORE_PM_ENV_INTEL_UNCORE,
 	RTE_UNCORE_PM_ENV_AMD_HSMP
 };
 
 /**
- * Set the default uncore power management implementation. If this is not called prior
- * to rte_power_uncore_init(), then auto-detect of the environment will take place.
- * It is thread safe. New env can be set only in uninitialized state
- * (thus rte_power_unset_uncore_env must be called if different env was already set).
+ * Set the default uncore power management implementation. This has to be called
+ * prior to calling any other rte_power_uncore_*() API.
+ * It is thread safe. New env can be set only in uninitialized state.
+ * rte_power_unset_uncore_env must be called if different env was already set).
  *
  * @param env
  *  env. The environment in which to initialise Uncore Power Management for.