Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1221837 > unrolled thread

Re: [PATCH] cpufreq: pass policy to ->get() driver callback

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] cpufreq: pass policy to ->get() driver callback "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-10 03:20 +0200
    Re: [PATCH] cpufreq: pass policy to ->get() driver callback Viresh Kumar <viresh.kumar@linaro.org> - 2015-09-10 03:30 +0200
      Re: [PATCH] cpufreq: pass policy to ->get() driver callback "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-10 23:10 +0200
        Re: [PATCH] cpufreq: pass policy to ->get() driver callback Viresh Kumar <viresh.kumar@linaro.org> - 2015-09-11 18:20 +0200
          Re: [PATCH] cpufreq: pass policy to ->get() driver callback Viresh Kumar <viresh.kumar@linaro.org> - 2015-09-15 09:40 +0200
            Re: [PATCH] cpufreq: pass policy to ->get() driver callback Viresh Kumar <viresh.kumar@linaro.org> - 2015-09-15 10:00 +0200
            Re: [PATCH] cpufreq: pass policy to ->get() driver callback "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-16 03:10 +0200
      Re: [PATCH] cpufreq: pass policy to ->get() driver callback "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-10 23:20 +0200
      Re: [PATCH] cpufreq: pass policy to ->get() driver callback "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-10 23:40 +0200
        Re: [PATCH] cpufreq: pass policy to ->get() driver callback Viresh Kumar <viresh.kumar@linaro.org> - 2015-09-15 10:00 +0200
          Re: [PATCH] cpufreq: pass policy to ->get() driver callback "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-16 03:20 +0200

#1221837 — Re: [PATCH] cpufreq: pass policy to ->get() driver callback

On Friday, July 31, 2015 04:14:33 PM Viresh Kumar wrote:
> CPUFreq drivers today support ->get(cpu) callback, which returns current
> clock rate of the CPU. The problem with ->get() is that it takes cpu
> number as parameter and this unnecessarily makes things complex.
> 
> Firstly the core gets the cpu number by doing operation 'policy->cpu' on
> the policy and then many drivers

I see one.  That unfortunately is the acpi-cpufreq driver, but it still is one.

Well, cpufreq_generic_get() does _get_raw(), so I suppose acpi-cpufreq may
do that too?

> need to get the policy back and so do
> cpufreq_cpu_get(cpu) on the cpu passed as argument to ->get().
> 
> It would be better if we pass them 'policy' directly and drivers can use
> policy->cpu if that's all they need.

Passing a pointer and dereferencing it is generally less efficient than passing
a number.  Before the patch the core has to do the dereference before calling
->get, so it likely doesn't matter here, but the code churn from this change
is quite substantial and the benefit from it is in the noise IMO.

Overall, we need to talk about the design aspect of cpufreq, because there
are much more significant issues in it than things like this one.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1221839

On 10-09-15, 03:41, Rafael J. Wysocki wrote:
> I see one.  That unfortunately is the acpi-cpufreq driver, but it still is one.

Hmm..

> Well, cpufreq_generic_get() does _get_raw(), so I suppose acpi-cpufreq may
> do that too?

Yeah, it can.

> > need to get the policy back and so do
> > cpufreq_cpu_get(cpu) on the cpu passed as argument to ->get().
> > 
> > It would be better if we pass them 'policy' directly and drivers can use
> > policy->cpu if that's all they need.
> 
> Passing a pointer and dereferencing it is generally less efficient than passing
> a number.  Before the patch the core has to do the dereference before calling
> ->get, so it likely doesn't matter here, but the code churn from this change
> is quite substantial and the benefit from it is in the noise IMO.

Hmm.. Actually almost every other callback (bios_limit() is another
one), passes the policy to the driver, and I thought always passing
the policy will make it more symmetrical. And the expectation that the
cpufreq drivers wouldn't need to use policy from the ->get() callback
would be wrong. Even if there are only few users today. One is the
acpi-cpufreq driver and others are the ones, that are using
cpufreq_generic_get() :)

> Overall, we need to talk about the design aspect of cpufreq, because there
> are much more significant issues in it than things like this one.

I agree.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222396

On Thursday, September 10, 2015 06:52:22 AM Viresh Kumar wrote:
> On 10-09-15, 03:41, Rafael J. Wysocki wrote:
> > I see one.  That unfortunately is the acpi-cpufreq driver, but it still is one.
> 
> Hmm..
> 
> > Well, cpufreq_generic_get() does _get_raw(), so I suppose acpi-cpufreq may
> > do that too?
> 
> Yeah, it can.

OK, so let's make that change.

BTW, I wonder how much of the stuff in cpufreq.h can be moved to a local header
under drivers/cpufreq/.  It looks like the majority of it is not used by
anybody else.

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: cpufreq: acpi-cpufreq: Use cpufreq_cpu_get_raw() in ->get()

cpufreq_cpu_get() called by get_cur_freq_on_cpu() is overkill,
because the ->get() callback is always invoked in a context in
which all of the conditions checked by cpufreq_cpu_get() are
guaranteed to be satisfied.

Use cpufreq_cpu_get_raw() instead of it and drop the
corresponding cpufreq_cpu_put() from get_cur_freq_on_cpu().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/acpi-cpufreq.c |    3 +--
 drivers/cpufreq/cpufreq.c      |    2 +-
 include/linux/cpufreq.h        |    5 +++++
 3 files changed, 7 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/acpi-cpufreq.c
+++ linux-pm/drivers/cpufreq/acpi-cpufreq.c
@@ -375,12 +375,11 @@ static unsigned int get_cur_freq_on_cpu(
 
 	pr_debug("get_cur_freq_on_cpu (%d)\n", cpu);
 
-	policy = cpufreq_cpu_get(cpu);
+	policy = cpufreq_cpu_get_raw(cpu);
 	if (unlikely(!policy))
 		return 0;
 
 	data = policy->driver_data;
-	cpufreq_cpu_put(policy);
 	if (unlikely(!data || !data->freq_table))
 		return 0;
 
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -238,13 +238,13 @@ int cpufreq_generic_init(struct cpufreq_
 }
 EXPORT_SYMBOL_GPL(cpufreq_generic_init);
 
-/* Only for cpufreq core internal use */
 struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu)
 {
 	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
 
 	return policy && cpumask_test_cpu(cpu, policy->cpus) ? policy : NULL;
 }
+EXPORT_SYMBOL_GPL(cpufreq_cpu_get_raw);
 
 unsigned int cpufreq_generic_get(unsigned int cpu)
 {
Index: linux-pm/include/linux/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/cpufreq.h
+++ linux-pm/include/linux/cpufreq.h
@@ -129,9 +129,14 @@ struct cpufreq_policy {
 #define CPUFREQ_SHARED_TYPE_ANY	 (3) /* Freq can be set from any dependent CPU*/
 
 #ifdef CONFIG_CPU_FREQ
+struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu);
 struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu);
 void cpufreq_cpu_put(struct cpufreq_policy *policy);
 #else
+static inline struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu)
+{
+	return NULL;
+}
 static inline struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
 {
 	return NULL;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222942

On 10-09-15, 23:36, Rafael J. Wysocki wrote:
> BTW, I wonder how much of the stuff in cpufreq.h can be moved to a local header
> under drivers/cpufreq/.  It looks like the majority of it is not used by
> anybody else.

Okay, will check that out and do some cleanup.

> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: cpufreq: acpi-cpufreq: Use cpufreq_cpu_get_raw() in ->get()
> 
> cpufreq_cpu_get() called by get_cur_freq_on_cpu() is overkill,
> because the ->get() callback is always invoked in a context in
> which all of the conditions checked by cpufreq_cpu_get() are
> guaranteed to be satisfied.
> 
> Use cpufreq_cpu_get_raw() instead of it and drop the
> corresponding cpufreq_cpu_put() from get_cur_freq_on_cpu().
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/acpi-cpufreq.c |    3 +--
>  drivers/cpufreq/cpufreq.c      |    2 +-
>  include/linux/cpufreq.h        |    5 +++++
>  3 files changed, 7 insertions(+), 3 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/acpi-cpufreq.c
> +++ linux-pm/drivers/cpufreq/acpi-cpufreq.c
> @@ -375,12 +375,11 @@ static unsigned int get_cur_freq_on_cpu(
>  
>  	pr_debug("get_cur_freq_on_cpu (%d)\n", cpu);
>  
> -	policy = cpufreq_cpu_get(cpu);
> +	policy = cpufreq_cpu_get_raw(cpu);
>  	if (unlikely(!policy))
>  		return 0;
>  
>  	data = policy->driver_data;
> -	cpufreq_cpu_put(policy);
>  	if (unlikely(!data || !data->freq_table))
>  		return 0;
>  
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -238,13 +238,13 @@ int cpufreq_generic_init(struct cpufreq_
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_generic_init);
>  
> -/* Only for cpufreq core internal use */
>  struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu)
>  {
>  	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
>  
>  	return policy && cpumask_test_cpu(cpu, policy->cpus) ? policy : NULL;
>  }
> +EXPORT_SYMBOL_GPL(cpufreq_cpu_get_raw);
>  
>  unsigned int cpufreq_generic_get(unsigned int cpu)
>  {
> Index: linux-pm/include/linux/cpufreq.h
> ===================================================================
> --- linux-pm.orig/include/linux/cpufreq.h
> +++ linux-pm/include/linux/cpufreq.h
> @@ -129,9 +129,14 @@ struct cpufreq_policy {
>  #define CPUFREQ_SHARED_TYPE_ANY	 (3) /* Freq can be set from any dependent CPU*/
>  
>  #ifdef CONFIG_CPU_FREQ
> +struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu);
>  struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu);
>  void cpufreq_cpu_put(struct cpufreq_policy *policy);
>  #else
> +static inline struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu)
> +{
> +	return NULL;
> +}
>  static inline struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
>  {
>  	return NULL;

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1224719

On 11-09-15, 21:48, Viresh Kumar wrote:
> > -/* Only for cpufreq core internal use */
> >  struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu)
> >  {
> >  	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
> >  
> >  	return policy && cpumask_test_cpu(cpu, policy->cpus) ? policy : NULL;
> >  }
> > +EXPORT_SYMBOL_GPL(cpufreq_cpu_get_raw);

This routine was recently marked as static and now with your patch it
results in compilation error. You just need to remove the 'static'
part from the routine.

I would have sent a patch for this, but I thought you would like to
edit the original patch itself to not break git bisect.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1224732

On 15-09-15, 13:09, Viresh Kumar wrote:
> I would have sent a patch for this, but I thought you would like to
> edit the original patch itself to not break git bisect.

But in case you are applying it separately, here is the fix.

-------------------------8<-------------------------

From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 15 Sep 2015 13:09:54 +0530
Subject: [PATCH] cpufreq: cpufreq_cpu_get_raw() isn't a static function
 anymore

Its used outside of the core file and is exported as well. Drop the
'static' keyword from its declaration and fix below build error:

linux/drivers/cpufreq/cpufreq.c:241:31: error: static declaration of
'cpufreq_cpu_get_raw' follows non-static declaration

Fixes: 503655f70d9b ("cpufreq: acpi-cpufreq: Use cpufreq_cpu_get_raw() in ->get()")
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index a8252bb0f80c..ef5ed9470de9 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -238,7 +238,7 @@ int cpufreq_generic_init(struct cpufreq_policy *policy,
 }
 EXPORT_SYMBOL_GPL(cpufreq_generic_init);
 
-static struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu)
+struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu)
 {
 	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225646

On Tuesday, September 15, 2015 01:09:21 PM Viresh Kumar wrote:
> On 11-09-15, 21:48, Viresh Kumar wrote:
> > > -/* Only for cpufreq core internal use */
> > >  struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu)
> > >  {
> > >  	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
> > >  
> > >  	return policy && cpumask_test_cpu(cpu, policy->cpus) ? policy : NULL;
> > >  }
> > > +EXPORT_SYMBOL_GPL(cpufreq_cpu_get_raw);
> 
> This routine was recently marked as static and now with your patch it
> results in compilation error. You just need to remove the 'static'
> part from the routine.
> 
> I would have sent a patch for this, but I thought you would like to
> edit the original patch itself to not break git bisect.

I even tried to do that previously, but I made a mistake then.

It should be fixed in my bleeding-edge branch now.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222397

On Thursday, September 10, 2015 06:52:22 AM Viresh Kumar wrote:
> On 10-09-15, 03:41, Rafael J. Wysocki wrote:

[cut]

> > Passing a pointer and dereferencing it is generally less efficient than passing
> > a number.  Before the patch the core has to do the dereference before calling
> > ->get, so it likely doesn't matter here, but the code churn from this change
> > is quite substantial and the benefit from it is in the noise IMO.
> 
> Hmm.. Actually almost every other callback (bios_limit() is another
> one), passes the policy to the driver, and I thought always passing
> the policy will make it more symmetrical. And the expectation that the
> cpufreq drivers wouldn't need to use policy from the ->get() callback
> would be wrong. Even if there are only few users today. One is the
> acpi-cpufreq driver and others are the ones, that are using
> cpufreq_generic_get() :)

So the whole question is whether or not this is worth the whole code churn
related to the exchange of callbacks.

At this point I really don't know.  It depends on the design discussion I'd
like to start.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222399

On Thursday, September 10, 2015 06:52:22 AM Viresh Kumar wrote:
> On 10-09-15, 03:41, Rafael J. Wysocki wrote:

[cut]

> 
> > Overall, we need to talk about the design aspect of cpufreq, because there
> > are much more significant issues in it than things like this one.
> 
> I agree.

OK

Adding Mark and Srinivas who may be interested in this to CC.

Why don't we start with listing all of the cpufreq's shortcomings we'd like
to address, then try to sort out conflicting items and come up with a list
of tasks to complete?

To me, the most painful thing ATM is that cpufreq cannot use timer functions
to carry out state transitions even if the underlying driver could request
state to be changed from interrupt context.  IMO, we should utilize the
capabilities of the hardware where possible and only add overhead where it
is necessary.

Speaking of which I'm concerned that we're adding overhead for systems that
don't need software synchronization of state transitions (the "one CPU per
policy" case).  I'm not sure how much of that is really happening, but it
would be review the code from that angle and streamline things where
policy objects are not shared.

The locking is overdesigned and overkill (and you know that already), but
if we do the above, it'll be more strarightforward to simplify it IMO.

Finally, the initialization is questionable and in particular the fact that we
need to call the driver's ->init at least once to get policy->cpus populated.
This shouldn't be necessary, as that information reflects the topology of
the system and shouldn't depend on which driver is in use really.

Please let me know what your pain points are. :-)

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1224737

On 10-09-15, 23:59, Rafael J. Wysocki wrote:
> Adding Mark and Srinivas who may be interested in this to CC.

         Mike ? :)

> Why don't we start with listing all of the cpufreq's shortcomings we'd like
> to address, then try to sort out conflicting items and come up with a list
> of tasks to complete?

Yeah, sounds like a good plan.

> To me, the most painful thing ATM is that cpufreq cannot use timer functions
> to carry out state transitions even if the underlying driver could request
> state to be changed from interrupt context.  IMO, we should utilize the
> capabilities of the hardware where possible and only add overhead where it
> is necessary.

Absolutely right.

> Speaking of which I'm concerned that we're adding overhead for systems that
> don't need software synchronization of state transitions (the "one CPU per
> policy" case).  I'm not sure how much of that is really happening, but it
> would be review the code from that angle and streamline things where
> policy objects are not shared.

Maybe, maybe not.. Probably we need to look at exact code paths for
that and then decide.. But we should get this simplified if we can.

> The locking is overdesigned and overkill (and you know that already), but
> if we do the above, it'll be more strarightforward to simplify it IMO.

Locking is really bad today.. I wrote lots of patches for that, but
never got them upstreamed. I should try doing that after pushing the
remaining govenor patches ..

> Finally, the initialization is questionable and in particular the fact that we
> need to call the driver's ->init at least once to get policy->cpus populated.
> This shouldn't be necessary, as that information reflects the topology of
> the system and shouldn't depend on which driver is in use really.

I am not sure how feasible it will be get the topology information in
a generic way, but if we can, we should.

> Please let me know what your pain points are. :-)

There were few minor things as well, I already have some code for
them:
- Dividing the really large cpufreq.[h|c] files into better, more
  readable blocks. For example, one file for sysfs stuff alone..
- Some sort of test suite for cpufreq. I wrote one, which contains
  most of the cases, which helped us fixing governor races:

  https://git.linaro.org/people/viresh.kumar/cpufreq-tests.git

  But, these are plain bash scripts. Not really tied to any kernel
  internal test suite. Which suite should I adapt them for ?

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225653

On Tuesday, September 15, 2015 01:24:31 PM Viresh Kumar wrote:
> On 10-09-15, 23:59, Rafael J. Wysocki wrote:
> > Adding Mark and Srinivas who may be interested in this to CC.
> 
>          Mike ? :)
> 
> > Why don't we start with listing all of the cpufreq's shortcomings we'd like
> > to address, then try to sort out conflicting items and come up with a list
> > of tasks to complete?
> 
> Yeah, sounds like a good plan.
> 
> > To me, the most painful thing ATM is that cpufreq cannot use timer functions
> > to carry out state transitions even if the underlying driver could request
> > state to be changed from interrupt context.  IMO, we should utilize the
> > capabilities of the hardware where possible and only add overhead where it
> > is necessary.
> 
> Absolutely right.
> 
> > Speaking of which I'm concerned that we're adding overhead for systems that
> > don't need software synchronization of state transitions (the "one CPU per
> > policy" case).  I'm not sure how much of that is really happening, but it
> > would be review the code from that angle and streamline things where
> > policy objects are not shared.
> 
> Maybe, maybe not.. Probably we need to look at exact code paths for
> that and then decide.. But we should get this simplified if we can.
> 
> > The locking is overdesigned and overkill (and you know that already), but
> > if we do the above, it'll be more strarightforward to simplify it IMO.
> 
> Locking is really bad today.. I wrote lots of patches for that, but
> never got them upstreamed. I should try doing that after pushing the
> remaining govenor patches ..
> 
> > Finally, the initialization is questionable and in particular the fact that we
> > need to call the driver's ->init at least once to get policy->cpus populated.
> > This shouldn't be necessary, as that information reflects the topology of
> > the system and shouldn't depend on which driver is in use really.
> 
> I am not sure how feasible it will be get the topology information in
> a generic way, but if we can, we should.

That depends on what you mean by "generic".  I don't think there's one that
will work for all platforms, but I don't see a reason why it should depend
on the cpufreq driver.

cpufreq drivers can use DT or ACPI or OPPs provided by the platform for
that and all of those things can be accessed by the cpufreq core itself
just as well.

> > Please let me know what your pain points are. :-)
> 
> There were few minor things as well, I already have some code for
> them:
> - Dividing the really large cpufreq.[h|c] files into better, more
>   readable blocks. For example, one file for sysfs stuff alone..

Yeah, that's worth doing, but maybe in the process of re-working things
to avoid making significant changes depend on cleanups.

Speaking of sysfs, that's one more item for my list.  The structure of
the sysfs interface used by cpufreq today is confusing to put it lightly.

For example, we need to make the information that multiple CPUs share the
same cpufreq settings more explicit and I really don't think we should
fail sysfs accesses for offline CPUs when it is not necessary.  At least,
we should fail it for all of them or for none of them, but we don't do
that.

> - Some sort of test suite for cpufreq. I wrote one, which contains
>   most of the cases, which helped us fixing governor races:
> 
>   https://git.linaro.org/people/viresh.kumar/cpufreq-tests.git
> 
>   But, these are plain bash scripts. Not really tied to any kernel
>   internal test suite. Which suite should I adapt them for ?

kselftest seems to be a reasonable target to me.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web