* [Powertop] [RFC] [PATCH 09/11] Deduplication: properly merge cpu_package::calculate_freq() into abstract_cpu::calculate_freq()
@ 2012-11-05 11:12 Ivan Shapovalov
0 siblings, 0 replies; 3+ messages in thread
From: Ivan Shapovalov @ 2012-11-05 11:12 UTC (permalink / raw)
To: powertop
[-- Attachment #1: Type: text/plain, Size: 3899 bytes --]
The overridden method "cpu_package::calculate_freq()" differs from a generic implementation in
that it propagates the package's frequency to all in-package cores by
calling change_effective_frequency().
Instead of duplicating the code, introduce a small virtual function
freq_updated() which normally propagates frequency changes upwards and
override it for cpu_package, retaining the correct behavior. Then remove the override of
calculate_freq().
Signed-off-by: Ivan Shapovalov <intelfx100(a)gmail.com>
---
src/cpu/abstract_cpu.cpp | 11 ++++++++---
src/cpu/cpu.h | 5 +++--
src/cpu/cpu_package.cpp | 38 +++++++++++---------------------------
3 files changed, 22 insertions(+), 32 deletions(-)
diff --git a/src/cpu/abstract_cpu.cpp b/src/cpu/abstract_cpu.cpp
index 083a1e0..d64cb24 100644
--- a/src/cpu/abstract_cpu.cpp
+++ b/src/cpu/abstract_cpu.cpp
@@ -69,6 +69,13 @@ void abstract_cpu::account_freq(uint64_t freq, uint64_t duration)
}
+void abstract_cpu::freq_updated(uint64_t time)
+{
+ if(parent)
+ parent->calculate_freq(time);
+ old_idle = idle;
+}
+
void abstract_cpu::measurement_start(void)
{
unsigned int i;
@@ -385,9 +392,7 @@ void abstract_cpu::calculate_freq(uint64_t time)
current_frequency = freq;
idle = is_idle;
- if (parent)
- parent->calculate_freq(time);
- old_idle = idle;
+ freq_updated(time);
}
void abstract_cpu::change_effective_frequency(uint64_t time, uint64_t frequency)
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
index 1e9d771..498447b 100644
--- a/src/cpu/cpu.h
+++ b/src/cpu/cpu.h
@@ -85,6 +85,7 @@ protected:
uint64_t max_minus_one_frequency;
virtual void account_freq(uint64_t frequency, uint64_t duration);
+ virtual void freq_updated(uint64_t time);
public:
uint64_t last_stamp;
uint64_t total_stamp;
@@ -192,6 +193,8 @@ public:
class cpu_package: public abstract_cpu
{
+protected:
+ virtual void freq_updated(uint64_t time);
public:
virtual char * fill_cstate_line(int line_nr, char *buffer, const char *separator="");
virtual char * fill_cstate_name(int line_nr, char *buffer);
@@ -199,8 +202,6 @@ public:
virtual char * fill_pstate_line(int line_nr, char *buffer);
virtual char * fill_pstate_name(int line_nr, char *buffer);
virtual int can_collapse(void) { return childcount == 1;};
-
- virtual void calculate_freq(uint64_t time);
};
extern void enumerate_cpus(void);
diff --git a/src/cpu/cpu_package.cpp b/src/cpu/cpu_package.cpp
index 0ad5bdd..277b55f 100644
--- a/src/cpu/cpu_package.cpp
+++ b/src/cpu/cpu_package.cpp
@@ -27,6 +27,17 @@
#include "../lib.h"
#include "../parameters/parameters.h"
+void cpu_package::freq_updated(uint64_t time)
+{
+ if (parent)
+ parent->calculate_freq(time);
+ /*
+ * Make the frequency changes to propagate to all cores in a package.
+ */
+ change_effective_frequency(time, current_frequency);
+ old_idle = idle;
+}
+
char * cpu_package::fill_cstate_line(int line_nr, char *buffer, const char *separator)
{
unsigned int i;
@@ -90,30 +101,3 @@ char * cpu_package::fill_pstate_line(int line_nr, char *buffer)
sprintf(buffer," %5.1f%% ", percentage(1.0* (pstates[line_nr]->time_after) / total_stamp));
return buffer;
}
-
-void cpu_package::calculate_freq(uint64_t time)
-{
- uint64_t freq = 0;
- bool is_idle = true;
- unsigned int i;
-
- /* calculate the maximum frequency of all children */
- for (i = 0; i < children.size(); i++)
- if (children[i]) {
- uint64_t f = 0;
- if (!children[i]->idle) {
- f = children[i]->current_frequency;
- is_idle = false;
- }
- if (f > freq)
- freq = f;
- }
-
- current_frequency = freq;
- idle = is_idle;
- if (parent)
- parent->calculate_freq(time);
- change_effective_frequency(time, current_frequency);
- old_idle = idle;
-}
-
--
1.8.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Powertop] [RFC] [PATCH 09/11] Deduplication: properly merge cpu_package::calculate_freq() into abstract_cpu::calculate_freq()
@ 2012-11-06 20:30 Sergey Senozhatsky
0 siblings, 0 replies; 3+ messages in thread
From: Sergey Senozhatsky @ 2012-11-06 20:30 UTC (permalink / raw)
To: powertop
[-- Attachment #1: Type: text/plain, Size: 4430 bytes --]
On (11/05/12 15:12), Ivan Shapovalov wrote:
>
> The overridden method "cpu_package::calculate_freq()" differs from a generic implementation in
> that it propagates the package's frequency to all in-package cores by
> calling change_effective_frequency().
>
> Instead of duplicating the code, introduce a small virtual function
> freq_updated() which normally propagates frequency changes upwards and
> override it for cpu_package, retaining the correct behavior. Then remove the override of
> calculate_freq().
>
> Signed-off-by: Ivan Shapovalov <intelfx100(a)gmail.com>
> ---
> src/cpu/abstract_cpu.cpp | 11 ++++++++---
> src/cpu/cpu.h | 5 +++--
> src/cpu/cpu_package.cpp | 38 +++++++++++---------------------------
> 3 files changed, 22 insertions(+), 32 deletions(-)
>
> diff --git a/src/cpu/abstract_cpu.cpp b/src/cpu/abstract_cpu.cpp
> index 083a1e0..d64cb24 100644
> --- a/src/cpu/abstract_cpu.cpp
> +++ b/src/cpu/abstract_cpu.cpp
> @@ -69,6 +69,13 @@ void abstract_cpu::account_freq(uint64_t freq, uint64_t duration)
>
> }
>
> +void abstract_cpu::freq_updated(uint64_t time)
> +{
> + if(parent)
> + parent->calculate_freq(time);
> + old_idle = idle;
> +}
> +
why is this function called something_updatED?
-ss
> void abstract_cpu::measurement_start(void)
> {
> unsigned int i;
> @@ -385,9 +392,7 @@ void abstract_cpu::calculate_freq(uint64_t time)
>
> current_frequency = freq;
> idle = is_idle;
> - if (parent)
> - parent->calculate_freq(time);
> - old_idle = idle;
> + freq_updated(time);
> }
>
> void abstract_cpu::change_effective_frequency(uint64_t time, uint64_t frequency)
> diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
> index 1e9d771..498447b 100644
> --- a/src/cpu/cpu.h
> +++ b/src/cpu/cpu.h
> @@ -85,6 +85,7 @@ protected:
> uint64_t max_minus_one_frequency;
>
> virtual void account_freq(uint64_t frequency, uint64_t duration);
> + virtual void freq_updated(uint64_t time);
> public:
> uint64_t last_stamp;
> uint64_t total_stamp;
> @@ -192,6 +193,8 @@ public:
>
> class cpu_package: public abstract_cpu
> {
> +protected:
> + virtual void freq_updated(uint64_t time);
> public:
> virtual char * fill_cstate_line(int line_nr, char *buffer, const char *separator="");
> virtual char * fill_cstate_name(int line_nr, char *buffer);
> @@ -199,8 +202,6 @@ public:
> virtual char * fill_pstate_line(int line_nr, char *buffer);
> virtual char * fill_pstate_name(int line_nr, char *buffer);
> virtual int can_collapse(void) { return childcount == 1;};
> -
> - virtual void calculate_freq(uint64_t time);
> };
>
> extern void enumerate_cpus(void);
> diff --git a/src/cpu/cpu_package.cpp b/src/cpu/cpu_package.cpp
> index 0ad5bdd..277b55f 100644
> --- a/src/cpu/cpu_package.cpp
> +++ b/src/cpu/cpu_package.cpp
> @@ -27,6 +27,17 @@
> #include "../lib.h"
> #include "../parameters/parameters.h"
>
> +void cpu_package::freq_updated(uint64_t time)
> +{
> + if (parent)
> + parent->calculate_freq(time);
> + /*
> + * Make the frequency changes to propagate to all cores in a package.
> + */
> + change_effective_frequency(time, current_frequency);
> + old_idle = idle;
> +}
> +
> char * cpu_package::fill_cstate_line(int line_nr, char *buffer, const char *separator)
> {
> unsigned int i;
> @@ -90,30 +101,3 @@ char * cpu_package::fill_pstate_line(int line_nr, char *buffer)
> sprintf(buffer," %5.1f%% ", percentage(1.0* (pstates[line_nr]->time_after) / total_stamp));
> return buffer;
> }
> -
> -void cpu_package::calculate_freq(uint64_t time)
> -{
> - uint64_t freq = 0;
> - bool is_idle = true;
> - unsigned int i;
> -
> - /* calculate the maximum frequency of all children */
> - for (i = 0; i < children.size(); i++)
> - if (children[i]) {
> - uint64_t f = 0;
> - if (!children[i]->idle) {
> - f = children[i]->current_frequency;
> - is_idle = false;
> - }
> - if (f > freq)
> - freq = f;
> - }
> -
> - current_frequency = freq;
> - idle = is_idle;
> - if (parent)
> - parent->calculate_freq(time);
> - change_effective_frequency(time, current_frequency);
> - old_idle = idle;
> -}
> -
> --
> 1.8.0
>
> _______________________________________________
> PowerTop mailing list
> PowerTop(a)lists.01.org
> https://lists.01.org/mailman/listinfo/powertop
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Powertop] [RFC] [PATCH 09/11] Deduplication: properly merge cpu_package::calculate_freq() into abstract_cpu::calculate_freq()
@ 2012-11-07 5:27 Ivan Shapovalov
0 siblings, 0 replies; 3+ messages in thread
From: Ivan Shapovalov @ 2012-11-07 5:27 UTC (permalink / raw)
To: powertop
[-- Attachment #1: Type: text/plain, Size: 4920 bytes --]
On 06 November 2012 23:30:19 Sergey Senozhatsky wrote:
> On (11/05/12 15:12), Ivan Shapovalov wrote:
> > The overridden method "cpu_package::calculate_freq()" differs from a
> > generic implementation in that it propagates the package's frequency to
> > all in-package cores by calling change_effective_frequency().
> >
> > Instead of duplicating the code, introduce a small virtual function
> > freq_updated() which normally propagates frequency changes upwards and
> > override it for cpu_package, retaining the correct behavior. Then remove
> > the override of calculate_freq().
> >
> > Signed-off-by: Ivan Shapovalov <intelfx100(a)gmail.com>
> > ---
> >
> > src/cpu/abstract_cpu.cpp | 11 ++++++++---
> > src/cpu/cpu.h | 5 +++--
> > src/cpu/cpu_package.cpp | 38 +++++++++++---------------------------
> > 3 files changed, 22 insertions(+), 32 deletions(-)
> >
> > diff --git a/src/cpu/abstract_cpu.cpp b/src/cpu/abstract_cpu.cpp
> > index 083a1e0..d64cb24 100644
> > --- a/src/cpu/abstract_cpu.cpp
> > +++ b/src/cpu/abstract_cpu.cpp
> > @@ -69,6 +69,13 @@ void abstract_cpu::account_freq(uint64_t freq, uint64_t
> > duration)>
> > }
> >
> > +void abstract_cpu::freq_updated(uint64_t time)
> > +{
> > + if(parent)
> > + parent->calculate_freq(time);
> > + old_idle = idle;
> > +}
> > +
>
> why is this function called something_updatED?
Because it is called after field "current_frequency" and/or field "idle" are
updated by someone. The intent of the function is to propagate the local
changes to the cpu node hierarchy.
- Ivan
>
>
> -ss
>
> > void abstract_cpu::measurement_start(void)
> > {
> >
> > unsigned int i;
> >
> > @@ -385,9 +392,7 @@ void abstract_cpu::calculate_freq(uint64_t time)
> >
> > current_frequency = freq;
> > idle = is_idle;
> >
> > - if (parent)
> > - parent->calculate_freq(time);
> > - old_idle = idle;
> > + freq_updated(time);
> >
> > }
> >
> > void abstract_cpu::change_effective_frequency(uint64_t time, uint64_t
> > frequency)>
> > diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
> > index 1e9d771..498447b 100644
> > --- a/src/cpu/cpu.h
> > +++ b/src/cpu/cpu.h
> >
> > @@ -85,6 +85,7 @@ protected:
> > uint64_t max_minus_one_frequency;
> >
> > virtual void account_freq(uint64_t frequency, uint64_t duration);
> >
> > + virtual void freq_updated(uint64_t time);
> >
> > public:
> > uint64_t last_stamp;
> > uint64_t total_stamp;
> >
> > @@ -192,6 +193,8 @@ public:
> > class cpu_package: public abstract_cpu
> > {
> >
> > +protected:
> > + virtual void freq_updated(uint64_t time);
> >
> > public:
> > virtual char * fill_cstate_line(int line_nr, char *buffer, const char
> > *separator=""); virtual char * fill_cstate_name(int line_nr, char
> > *buffer);
> >
> > @@ -199,8 +202,6 @@ public:
> > virtual char * fill_pstate_line(int line_nr, char *buffer);
> > virtual char * fill_pstate_name(int line_nr, char *buffer);
> > virtual int can_collapse(void) { return childcount == 1;};
> >
> > -
> > - virtual void calculate_freq(uint64_t time);
> >
> > };
> >
> > extern void enumerate_cpus(void);
> >
> > diff --git a/src/cpu/cpu_package.cpp b/src/cpu/cpu_package.cpp
> > index 0ad5bdd..277b55f 100644
> > --- a/src/cpu/cpu_package.cpp
> > +++ b/src/cpu/cpu_package.cpp
> > @@ -27,6 +27,17 @@
> >
> > #include "../lib.h"
> > #include "../parameters/parameters.h"
> >
> > +void cpu_package::freq_updated(uint64_t time)
> > +{
> > + if (parent)
> > + parent->calculate_freq(time);
> > + /*
> > + * Make the frequency changes to propagate to all cores in a package.
> > + */
> > + change_effective_frequency(time, current_frequency);
> > + old_idle = idle;
> > +}
> > +
> >
> > char * cpu_package::fill_cstate_line(int line_nr, char *buffer, const
> > char *separator) {
> >
> > unsigned int i;
> >
> > @@ -90,30 +101,3 @@ char * cpu_package::fill_pstate_line(int line_nr, char
> > *buffer)>
> > sprintf(buffer," %5.1f%% ", percentage(1.0*
> > (pstates[line_nr]->time_after) / total_stamp)); return buffer;
> >
> > }
> >
> > -
> > -void cpu_package::calculate_freq(uint64_t time)
> > -{
> > - uint64_t freq = 0;
> > - bool is_idle = true;
> > - unsigned int i;
> > -
> > - /* calculate the maximum frequency of all children */
> > - for (i = 0; i < children.size(); i++)
> > - if (children[i]) {
> > - uint64_t f = 0;
> > - if (!children[i]->idle) {
> > - f = children[i]->current_frequency;
> > - is_idle = false;
> > - }
> > - if (f > freq)
> > - freq = f;
> > - }
> > -
> > - current_frequency = freq;
> > - idle = is_idle;
> > - if (parent)
> > - parent->calculate_freq(time);
> > - change_effective_frequency(time, current_frequency);
> > - old_idle = idle;
> > -}
> > -
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-11-07 5:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-06 20:30 [Powertop] [RFC] [PATCH 09/11] Deduplication: properly merge cpu_package::calculate_freq() into abstract_cpu::calculate_freq() Sergey Senozhatsky
-- strict thread matches above, loose matches on Subject: below --
2012-11-07 5:27 Ivan Shapovalov
2012-11-05 11:12 Ivan Shapovalov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.