platform/x86/intel-uncore-freq: fix inconsistent state on init failure

[PATCH] platform/x86/intel-uncore-freq: fix inconsistent state on init failure

Posted by shouyeliu 9 months, 4 weeks ago

When uncore_event_cpu_online() fails to initialize a control CPU (e.g.,
due to memory allocation failure or uncore_freq_add_entry() errors),
the code leaves stale entries in uncore_cpu_mask after that online CPU
will not try to call uncore_freq_add_entry, resulting in no sys interface.

Signed-off-by: shouyeliu <shouyeliu@gmail.com>
---
 .../x86/intel/uncore-frequency/uncore-frequency.c    | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
index 40bbf8e45fa4..1de0a4a9d6cd 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
@@ -146,15 +146,13 @@ static int uncore_event_cpu_online(unsigned int cpu)
 {
 	struct uncore_data *data;
 	int target;
+	int ret;
 
 	/* Check if there is an online cpu in the package for uncore MSR */
 	target = cpumask_any_and(&uncore_cpu_mask, topology_die_cpumask(cpu));
 	if (target < nr_cpu_ids)
 		return 0;
 
-	/* Use this CPU on this die as a control CPU */
-	cpumask_set_cpu(cpu, &uncore_cpu_mask);
-
 	data = uncore_get_instance(cpu);
 	if (!data)
 		return 0;
@@ -163,7 +161,13 @@ static int uncore_event_cpu_online(unsigned int cpu)
 	data->die_id = topology_die_id(cpu);
 	data->domain_id = UNCORE_DOMAIN_ID_INVALID;
 
-	return uncore_freq_add_entry(data, cpu);
+	ret = uncore_freq_add_entry(data, cpu);
+	if (!ret) {
+		/* Use this CPU on this die as a control CPU */
+		cpumask_set_cpu(cpu, &uncore_cpu_mask);
+	}
+
+	return ret;
 }
 
 static int uncore_event_cpu_offline(unsigned int cpu)
-- 
2.19.1

Re: [PATCH] platform/x86/intel-uncore-freq: fix inconsistent state on init failure

Posted by Ilpo Järvinen 9 months, 4 weeks ago

On Mon, 14 Apr 2025, shouyeliu wrote:

> When uncore_event_cpu_online() fails to initialize a control CPU (e.g.,
> due to memory allocation failure or uncore_freq_add_entry() errors),
> the code leaves stale entries in uncore_cpu_mask after that online CPU
> will not try to call uncore_freq_add_entry, resulting in no sys interface.

Please add () after any name that refers to a C function (you're not even 
being consistent here as you had it in some cases but not here).

Please try to split the very long sentence a bit and make it more obvious 
what causes what as the current wording is a bit vague, did you mean: 
uncore_event_cpu_online() will not call uncore_freq_add_entry() for
another CPU that is being onlined or something along those lines?

Will this change work/matter? Documentation/core-api/cpu_hotplug.rst says 
about cpuhp_setup_state():

"If a callback fails for CPU N then the teardown callback for CPU
 0 .. N-1 is invoked to rollback the operation. The state setup fails,
 the callbacks for the state are not installed and in case of dynamic
 allocation the allocated state is freed."

> 

Fixes tag?

> Signed-off-by: shouyeliu <shouyeliu@gmail.com>

The correct format for tags is documented in 
Documentation/process/5.Posting.rst:

tag: Full Name <email address>

> ---
>  .../x86/intel/uncore-frequency/uncore-frequency.c    | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
> index 40bbf8e45fa4..1de0a4a9d6cd 100644
> --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
> +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
> @@ -146,15 +146,13 @@ static int uncore_event_cpu_online(unsigned int cpu)
>  {
>  	struct uncore_data *data;
>  	int target;
> +	int ret;
>  
>  	/* Check if there is an online cpu in the package for uncore MSR */
>  	target = cpumask_any_and(&uncore_cpu_mask, topology_die_cpumask(cpu));
>  	if (target < nr_cpu_ids)
>  		return 0;
>  
> -	/* Use this CPU on this die as a control CPU */
> -	cpumask_set_cpu(cpu, &uncore_cpu_mask);
> -
>  	data = uncore_get_instance(cpu);
>  	if (!data)
>  		return 0;
> @@ -163,7 +161,13 @@ static int uncore_event_cpu_online(unsigned int cpu)
>  	data->die_id = topology_die_id(cpu);
>  	data->domain_id = UNCORE_DOMAIN_ID_INVALID;
>  
> -	return uncore_freq_add_entry(data, cpu);
> +	ret = uncore_freq_add_entry(data, cpu);
> +	if (!ret) {
> +		/* Use this CPU on this die as a control CPU */
> +		cpumask_set_cpu(cpu, &uncore_cpu_mask);
> +	}
> +
> +	return ret;

Please reverse to logic such that you return early on error, which is the 
usual error handling pattern.

>  }
>  
>  static int uncore_event_cpu_offline(unsigned int cpu)
> 

-- 
 i.

Re: [PATCH] platform/x86/intel-uncore-freq: fix inconsistent state on init failure

Posted by srinivas pandruvada 9 months, 4 weeks ago

On Mon, 2025-04-14 at 13:41 +0300, Ilpo Järvinen wrote:
> On Mon, 14 Apr 2025, shouyeliu wrote:
> 
> > When uncore_event_cpu_online() fails to initialize a control CPU
> > (e.g.,
> > due to memory allocation failure or uncore_freq_add_entry()
> > errors),
> > the code leaves stale entries in uncore_cpu_mask after that online
> > CPU
> > will not try to call uncore_freq_add_entry, resulting in no sys
> > interface.
> 
> Please add () after any name that refers to a C function (you're not
> even 
> being consistent here as you had it in some cases but not here).
> 
> Please try to split the very long sentence a bit and make it more
> obvious 
> what causes what as the current wording is a bit vague, did you mean:
> uncore_event_cpu_online() will not call uncore_freq_add_entry() for
> another CPU that is being onlined or something along those lines?
> 
> Will this change work/matter? Documentation/core-api/cpu_hotplug.rst
> says 
> about cpuhp_setup_state():
> 
> "If a callback fails for CPU N then the teardown callback for CPU
>  0 .. N-1 is invoked to rollback the operation. The state setup
> fails,
>  the callbacks for the state are not installed and in case of dynamic
>  allocation the allocated state is freed."
> 

Yes, cpuhp_setup_state() will fail and which will result in clean up.
So any fail of any fail uncore_event_cpu_online() will result in no sys
entries.

I think here the intention is to keep sys entries, which will not
happen with this patch.

For confirmation on 6.14 kernel, I forced failure on CPU 10:

[595799.696873] intel_uncore_init 
[595799.700102] uncore_event_cpu_online cpu:0
[595799.704240] uncore_event_cpu_online cpu:1
[595799.708360] uncore_event_cpu_online cpu:2
[595799.712505] uncore_event_cpu_online cpu:3
[595799.716633] uncore_event_cpu_online cpu:4
[595799.720755] uncore_event_cpu_online cpu:5
[595799.724953] uncore_event_cpu_online cpu:6
[595799.729158] uncore_event_cpu_online cpu:7
[595799.733409] uncore_event_cpu_online cpu:8
[595799.737674] uncore_event_cpu_online cpu:9
[595799.741954] uncore_event_cpu_online cpu:10
[595799.746134] Force CPU 10 to fail online
[595799.750182] uncore_event_cpu_offline cpu:0
[595799.754508] uncore_event_cpu_offline cpu:1
[595799.758834] uncore_event_cpu_offline cpu:2
[595799.763238] uncore_event_cpu_offline cpu:3
[595799.767558] uncore_event_cpu_offline cpu:4
[595799.771832] uncore_event_cpu_offline cpu:5
[595799.776178] uncore_event_cpu_offline cpu:6
[595799.780506] uncore_event_cpu_offline cpu:7
[595799.784862] uncore_event_cpu_offline cpu:8
[595799.789247] uncore_event_cpu_offline cpu:9
[595799.793540] intel_uncore_init cpuhp_setup_state failed
[595799.798776] intel_uncore_init failed


Thanks,
Srinivas



> > 
> 
> Fixes tag?
> 
> > Signed-off-by: shouyeliu <shouyeliu@gmail.com>
> 
> The correct format for tags is documented in 
> Documentation/process/5.Posting.rst:
> 
> tag: Full Name <email address>
> 
> > ---
> >  .../x86/intel/uncore-frequency/uncore-frequency.c    | 12
> > ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-
> > frequency.c b/drivers/platform/x86/intel/uncore-frequency/uncore-
> > frequency.c
> > index 40bbf8e45fa4..1de0a4a9d6cd 100644
> > --- a/drivers/platform/x86/intel/uncore-frequency/uncore-
> > frequency.c
> > +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-
> > frequency.c
> > @@ -146,15 +146,13 @@ static int uncore_event_cpu_online(unsigned
> > int cpu)
> >  {
> >  	struct uncore_data *data;
> >  	int target;
> > +	int ret;
> >  
> >  	/* Check if there is an online cpu in the package for
> > uncore MSR */
> >  	target = cpumask_any_and(&uncore_cpu_mask,
> > topology_die_cpumask(cpu));
> >  	if (target < nr_cpu_ids)
> >  		return 0;
> >  
> > -	/* Use this CPU on this die as a control CPU */
> > -	cpumask_set_cpu(cpu, &uncore_cpu_mask);
> > -
> >  	data = uncore_get_instance(cpu);
> >  	if (!data)
> >  		return 0;
> > @@ -163,7 +161,13 @@ static int uncore_event_cpu_online(unsigned
> > int cpu)
> >  	data->die_id = topology_die_id(cpu);
> >  	data->domain_id = UNCORE_DOMAIN_ID_INVALID;
> >  
> > -	return uncore_freq_add_entry(data, cpu);
> > +	ret = uncore_freq_add_entry(data, cpu);
> > +	if (!ret) {
> > +		/* Use this CPU on this die as a control CPU */
> > +		cpumask_set_cpu(cpu, &uncore_cpu_mask);
> > +	}
> > +
> > +	return ret;
> 
> Please reverse to logic such that you return early on error, which is
> the 
> usual error handling pattern.
> 
> >  }
> >  
> >  static int uncore_event_cpu_offline(unsigned int cpu)
> > 
>

Re: [PATCH] platform/x86/intel-uncore-freq: fix inconsistent state on init failure

Posted by liu shouye 9 months, 4 weeks ago

srinivas pandruvada <srinivas.pandruvada@linux.intel.com> 于2025年4月15日周二 00:08写道：
>
> On Mon, 2025-04-14 at 13:41 +0300, Ilpo Järvinen wrote:
> > On Mon, 14 Apr 2025, shouyeliu wrote:
> >
> > > When uncore_event_cpu_online() fails to initialize a control CPU
> > > (e.g.,
> > > due to memory allocation failure or uncore_freq_add_entry()
> > > errors),
> > > the code leaves stale entries in uncore_cpu_mask after that online
> > > CPU
> > > will not try to call uncore_freq_add_entry, resulting in no sys
> > > interface.
> >
> > Please add () after any name that refers to a C function (you're not
> > even
> > being consistent here as you had it in some cases but not here).
ok,I will modify it in the next version
> >
> > Please try to split the very long sentence a bit and make it more
> > obvious
> > what causes what as the current wording is a bit vague, did you mean:
> > uncore_event_cpu_online() will not call uncore_freq_add_entry() for
> > another CPU that is being onlined or something along those lines?
> >
> > Will this change work/matter? Documentation/core-api/cpu_hotplug.rst
> > says
> > about cpuhp_setup_state():
> >
> > "If a callback fails for CPU N then the teardown callback for CPU
> >  0 .. N-1 is invoked to rollback the operation. The state setup
> > fails,
> >  the callbacks for the state are not installed and in case of dynamic
> >  allocation the allocated state is freed."
> >
>
> Yes, cpuhp_setup_state() will fail and which will result in clean up.
> So any fail of any fail uncore_event_cpu_online() will result in no sys
> entries.
>
> I think here the intention is to keep sys entries, which will not
> happen with this patch.
>
> For confirmation on 6.14 kernel, I forced failure on CPU 10:
>
> [595799.696873] intel_uncore_init
> [595799.700102] uncore_event_cpu_online cpu:0
> [595799.704240] uncore_event_cpu_online cpu:1
> [595799.708360] uncore_event_cpu_online cpu:2
> [595799.712505] uncore_event_cpu_online cpu:3
> [595799.716633] uncore_event_cpu_online cpu:4
> [595799.720755] uncore_event_cpu_online cpu:5
> [595799.724953] uncore_event_cpu_online cpu:6
> [595799.729158] uncore_event_cpu_online cpu:7
> [595799.733409] uncore_event_cpu_online cpu:8
> [595799.737674] uncore_event_cpu_online cpu:9
> [595799.741954] uncore_event_cpu_online cpu:10
> [595799.746134] Force CPU 10 to fail online
> [595799.750182] uncore_event_cpu_offline cpu:0
> [595799.754508] uncore_event_cpu_offline cpu:1
> [595799.758834] uncore_event_cpu_offline cpu:2
> [595799.763238] uncore_event_cpu_offline cpu:3
> [595799.767558] uncore_event_cpu_offline cpu:4
> [595799.771832] uncore_event_cpu_offline cpu:5
> [595799.776178] uncore_event_cpu_offline cpu:6
> [595799.780506] uncore_event_cpu_offline cpu:7
> [595799.784862] uncore_event_cpu_offline cpu:8
> [595799.789247] uncore_event_cpu_offline cpu:9
> [595799.793540] intel_uncore_init cpuhp_setup_state failed
> [595799.798776] intel_uncore_init failed
>
>
> Thanks,
> Srinivas
Registering the CPU hot-plug callback function during booting can be
handled correctly. I think the problem occurs during runtime.
The original code may have problems when the CPU hot-plug modifies the
management CPU during runtime:
Assume that the CPUs of package 1 are 8-15, and the uncore driver has
been registered at boot time;
1. Offline all CPU No.8-15
2. Try online CPU No. 8, the code executes cpumask_set_cpu()
successfully, but fails in the uncore_freq_add_entry() process. At
this time, the mark of CPU No. 8 is added to uncore_cpu_mask, but no
sys interface is generated,cpu No.8 online fails;
3. Try online CPU No. 8 again, cpumask_any_and() judges success, and
the CPU No.8 online is successful at this time;
4. Assume that the attempt to online CPU No. 9-15 is successful at
this time, but there is no sys interface ————unexpected behavior 1.
5. Offline CPU No. 9-15, and offline No.8, will eventually call
uncore_freq_remove_die_entry()————unexpected behavior 2 is generated,
which may cause a crash.
>
>
>
> > >
> >
> > Fixes tag?
> >
> > > Signed-off-by: shouyeliu <shouyeliu@gmail.com>
> >
> > The correct format for tags is documented in
> > Documentation/process/5.Posting.rst:
> >
> > tag: Full Name <email address>
ok,I will modify it in the next version
> >
> > > ---
> > >  .../x86/intel/uncore-frequency/uncore-frequency.c    | 12
> > > ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-
> > > frequency.c b/drivers/platform/x86/intel/uncore-frequency/uncore-
> > > frequency.c
> > > index 40bbf8e45fa4..1de0a4a9d6cd 100644
> > > --- a/drivers/platform/x86/intel/uncore-frequency/uncore-
> > > frequency.c
> > > +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-
> > > frequency.c
> > > @@ -146,15 +146,13 @@ static int uncore_event_cpu_online(unsigned
> > > int cpu)
> > >  {
> > >     struct uncore_data *data;
> > >     int target;
> > > +   int ret;
> > >
> > >     /* Check if there is an online cpu in the package for
> > > uncore MSR */
> > >     target = cpumask_any_and(&uncore_cpu_mask,
> > > topology_die_cpumask(cpu));
> > >     if (target < nr_cpu_ids)
> > >             return 0;
> > >
> > > -   /* Use this CPU on this die as a control CPU */
> > > -   cpumask_set_cpu(cpu, &uncore_cpu_mask);
> > > -
> > >     data = uncore_get_instance(cpu);
> > >     if (!data)
> > >             return 0;
> > > @@ -163,7 +161,13 @@ static int uncore_event_cpu_online(unsigned
> > > int cpu)
> > >     data->die_id = topology_die_id(cpu);
> > >     data->domain_id = UNCORE_DOMAIN_ID_INVALID;
> > >
> > > -   return uncore_freq_add_entry(data, cpu);
> > > +   ret = uncore_freq_add_entry(data, cpu);
> > > +   if (!ret) {
> > > +           /* Use this CPU on this die as a control CPU */
> > > +           cpumask_set_cpu(cpu, &uncore_cpu_mask);
> > > +   }
> > > +
> > > +   return ret;
> >
> > Please reverse to logic such that you return early on error, which is
> > the
> > usual error handling pattern.
ok,I will modify it in the next version
> >
> > >  }
> > >
> > >  static int uncore_event_cpu_offline(unsigned int cpu)
> > >
> >
>