diff mbox series

[v11,3/9] cpufreq: introduce init_boost callback to initialize boost state for pstate drivers

Message ID 688d8e1ce875655c344ac3b29876da5dc0456739.1718262992.git.perry.yuan@amd.com (mailing list archive)
State Superseded, archived
Delegated to: Mario Limonciello
Headers show
Series AMD Pstate Driver Core Performance Boost | expand

Commit Message

Yuan, Perry June 13, 2024, 7:25 a.m. UTC
Introduce a new init_boost callback in cpufreq to initialize the boost
state for specific pstate drivers. This initialization is required before
calling the set_boost interface for each CPU.

The init_boost callback will set up and synchronize each CPU's current
boost state before invoking the set_boost function. Without this step,
the boost state may be inconsistent across CPUs.

Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
 drivers/cpufreq/cpufreq.c | 12 ++++++++++--
 include/linux/cpufreq.h   |  2 ++
 2 files changed, 12 insertions(+), 2 deletions(-)

Comments

Mario Limonciello June 13, 2024, 5:55 p.m. UTC | #1
On 6/13/2024 02:25, Perry Yuan wrote:
> Introduce a new init_boost callback in cpufreq to initialize the boost
> state for specific pstate drivers. This initialization is required before
> calling the set_boost interface for each CPU.
> 
> The init_boost callback will set up and synchronize each CPU's current
> boost state before invoking the set_boost function. Without this step,
> the boost state may be inconsistent across CPUs.
> 
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
>   drivers/cpufreq/cpufreq.c | 12 ++++++++++--
>   include/linux/cpufreq.h   |  2 ++
>   2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 1fdabb660231..e1a4730f4f8c 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1429,8 +1429,16 @@ static int cpufreq_online(unsigned int cpu)
>   			goto out_free_policy;
>   		}
>   
> -		/* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
> -		policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
> +		/* init boost state to prepare set_boost callback for each CPU */
> +		if (cpufreq_driver->init_boost) {
> +			ret = cpufreq_driver->init_boost(policy);
> +			if (ret)
> +				pr_debug("%s: %d: initialization boost failed\n", __func__,
> +					__LINE__);

The message should have the subject at the beginning.  IE:

"boost initialization failed\n"

Also, isn't this fatal if init failed?  IE shouldn't failing have a:

	goto out_free_policy;

> +		} else {
> +			/* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
> +			policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
> +		}
>   
>   		/*
>   		 * The initialization has succeeded and the policy is online.
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 20f7e98ee8af..0698c0292d8f 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -409,6 +409,8 @@ struct cpufreq_driver {
>   	bool		boost_enabled;
>   	int		(*set_boost)(struct cpufreq_policy *policy, int state);
>   
> +	/* initialize boost state to be consistent before calling set_boost */
> +	int		(*init_boost)(struct cpufreq_policy *policy);
>   	/*
>   	 * Set by drivers that want to register with the energy model after the
>   	 * policy is properly initialized, but before the governor is started.
Yuan, Perry June 14, 2024, 2:43 a.m. UTC | #2
[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Friday, June 14, 2024 1:55 AM
> To: Yuan, Perry <Perry.Yuan@amd.com>; Shenoy, Gautham Ranjal
> <gautham.shenoy@amd.com>; Petkov, Borislav <Borislav.Petkov@amd.com>
> Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v11 3/9] cpufreq: introduce init_boost callback to
> initialize boost state for pstate drivers
>
> On 6/13/2024 02:25, Perry Yuan wrote:
> > Introduce a new init_boost callback in cpufreq to initialize the boost
> > state for specific pstate drivers. This initialization is required
> > before calling the set_boost interface for each CPU.
> >
> > The init_boost callback will set up and synchronize each CPU's current
> > boost state before invoking the set_boost function. Without this step,
> > the boost state may be inconsistent across CPUs.
> >
> > Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> > ---
> >   drivers/cpufreq/cpufreq.c | 12 ++++++++++--
> >   include/linux/cpufreq.h   |  2 ++
> >   2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 1fdabb660231..e1a4730f4f8c 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -1429,8 +1429,16 @@ static int cpufreq_online(unsigned int cpu)
> >                     goto out_free_policy;
> >             }
> >
> > -           /* Let the per-policy boost flag mirror the cpufreq_driver
> boost during init */
> > -           policy->boost_enabled = cpufreq_boost_enabled() &&
> policy_has_boost_freq(policy);
> > +           /* init boost state to prepare set_boost callback for each CPU
> */
> > +           if (cpufreq_driver->init_boost) {
> > +                   ret = cpufreq_driver->init_boost(policy);
> > +                   if (ret)
> > +                           pr_debug("%s: %d: initialization boost
> failed\n", __func__,
> > +                                   __LINE__);
>
> The message should have the subject at the beginning.  IE:
>
> "boost initialization failed\n"
>
> Also, isn't this fatal if init failed?  IE shouldn't failing have a:


Firstly, I also add the " goto out_free_policy", and removed later,  because I think it is a little risky to fail the whole online process if driver init boost callback failed.
If driver init boost failed, it just let boost control function lost, but online initialize failed and go to free the policy,  it is a big problem.
I am ok to add the error handling if maintainer agree to see the potential online function aborting. 
Yuan, Perry June 19, 2024, 3:46 a.m. UTC | #3
[Public]

Hi Rafael

> -----Original Message-----
> From: Yuan, Perry <Perry.Yuan@amd.com>
> Sent: Friday, June 14, 2024 10:43 AM
> To: Limonciello, Mario <Mario.Limonciello@amd.com>; Shenoy, Gautham
> Ranjal <gautham.shenoy@amd.com>; Petkov, Borislav
> <Borislav.Petkov@amd.com>
> Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: RE: [PATCH v11 3/9] cpufreq: introduce init_boost callback to
> initialize boost state for pstate drivers
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> > -----Original Message-----
> > From: Limonciello, Mario <Mario.Limonciello@amd.com>
> > Sent: Friday, June 14, 2024 1:55 AM
> > To: Yuan, Perry <Perry.Yuan@amd.com>; Shenoy, Gautham Ranjal
> > <gautham.shenoy@amd.com>; Petkov, Borislav
> <Borislav.Petkov@amd.com>
> > Cc: rafael.j.wysocki@intel.com; viresh.kumar@linaro.org; Deucher,
> > Alexander <Alexander.Deucher@amd.com>; Huang, Shimmer
> > <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>;
> Meng, Li
> > (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH v11 3/9] cpufreq: introduce init_boost callback to
> > initialize boost state for pstate drivers
> >
> > On 6/13/2024 02:25, Perry Yuan wrote:
> > > Introduce a new init_boost callback in cpufreq to initialize the
> > > boost state for specific pstate drivers. This initialization is
> > > required before calling the set_boost interface for each CPU.
> > >
> > > The init_boost callback will set up and synchronize each CPU's
> > > current boost state before invoking the set_boost function. Without
> > > this step, the boost state may be inconsistent across CPUs.
> > >
> > > Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> > > ---
> > >   drivers/cpufreq/cpufreq.c | 12 ++++++++++--
> > >   include/linux/cpufreq.h   |  2 ++
> > >   2 files changed, 12 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index 1fdabb660231..e1a4730f4f8c 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -1429,8 +1429,16 @@ static int cpufreq_online(unsigned int cpu)
> > >                     goto out_free_policy;
> > >             }
> > >
> > > -           /* Let the per-policy boost flag mirror the cpufreq_driver
> > boost during init */
> > > -           policy->boost_enabled = cpufreq_boost_enabled() &&
> > policy_has_boost_freq(policy);
> > > +           /* init boost state to prepare set_boost callback for
> > > + each CPU
> > */
> > > +           if (cpufreq_driver->init_boost) {
> > > +                   ret = cpufreq_driver->init_boost(policy);
> > > +                   if (ret)
> > > +                           pr_debug("%s: %d: initialization boost
> > failed\n", __func__,
> > > +                                   __LINE__);
> >
> > The message should have the subject at the beginning.  IE:
> >
> > "boost initialization failed\n"
> >
> > Also, isn't this fatal if init failed?  IE shouldn't failing have a:
>
>
> Firstly, I also add the " goto out_free_policy", and removed later,  because I
> think it is a little risky to fail the whole online process if driver init boost
> callback failed.
> If driver init boost failed, it just let boost control function lost, but online
> initialize failed and go to free the policy,  it is a big problem.
> I am ok to add the error handling if maintainer agree to see the potential
> online function aborting. 
diff mbox series

Patch

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 1fdabb660231..e1a4730f4f8c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1429,8 +1429,16 @@  static int cpufreq_online(unsigned int cpu)
 			goto out_free_policy;
 		}
 
-		/* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
-		policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
+		/* init boost state to prepare set_boost callback for each CPU */
+		if (cpufreq_driver->init_boost) {
+			ret = cpufreq_driver->init_boost(policy);
+			if (ret)
+				pr_debug("%s: %d: initialization boost failed\n", __func__,
+					__LINE__);
+		} else {
+			/* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
+			policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
+		}
 
 		/*
 		 * The initialization has succeeded and the policy is online.
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 20f7e98ee8af..0698c0292d8f 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -409,6 +409,8 @@  struct cpufreq_driver {
 	bool		boost_enabled;
 	int		(*set_boost)(struct cpufreq_policy *policy, int state);
 
+	/* initialize boost state to be consistent before calling set_boost */
+	int		(*init_boost)(struct cpufreq_policy *policy);
 	/*
 	 * Set by drivers that want to register with the energy model after the
 	 * policy is properly initialized, but before the governor is started.