Messages in this thread | | | Date | Tue, 27 Nov 2012 11:01:32 -0800 | From | Jacob Pan <> | Subject | Re: [PATCH v2 3/3] PM: Introduce Intel PowerClamp Driver |
| |
On Mon, 26 Nov 2012 15:27:25 -0800 Joe Perches <joe@perches.com> wrote:
> On Mon, 2012-11-26 at 06:37 -0800, Jacob Pan wrote: > > Intel PowerClamp driver performs synchronized idle injection across > > all online CPUs. The goal is to maintain a given package level > > C-state ratio. > > trivial notes: > > > diff --git a/drivers/thermal/intel_powerclamp.c > > b/drivers/thermal/intel_powerclamp.c > > You should still add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > before any include so that all messages are prefixed with powerclamp: > I missed that. will fix in v4. thanks. > > + > > +/* #define DEBUG */ > > + > > +#include <linux/module.h> > > +#include <linux/kernel.h> > [] > > +static void adjust_compensation(int target_ratio, unsigned int win) > > +{ > > + int delta; > > It'd be shorter code to use a temporary like > > struct powerclamp_calibration_data *d = > &cal_data[target_ratio]; > > > + > > + /* > > + * adjust compensations if confidence level has not been > > reached or > > + * there are too many wakeups during the last idle > > injection period, we > > + * cannot trust the data for compensation. > > + */ > > + if (cal_data[target_ratio].confidence >= CONFIDENCE_OK || > > + atomic_read(&idle_wakeup_counter) > > > + win * num_online_cpus()) > > + return; > > + > > + delta = set_target_ratio - current_ratio; > > + /* filter out bad data */ > > + if (delta >= 0 && delta <= (1+target_ratio/10)) { > > + if (cal_data[target_ratio].steady_comp) > > + cal_data[target_ratio].steady_comp = > > + roundup(delta+ > > + > > cal_data[target_ratio].steady_comp, > > + 2)/2; > > so that this fits on a single line and becomes: > > if (d->steady_comp) > d->steady_comp = roundup(delta + > d->steady_comp, 2) / 2; > etc. > looks much better, will fix. > What clamps target_ratio to the correct range? > I briefly scanned the code but didn't spot it. > target_ratio is a local variable for set_target_ratio, I did this to avoid locking in case user wants to change it during computation. set_target_ratio is clamped by: if (set_target_ratio > MAX_TARGET_RATIO) set_target_ratio = MAX_TARGET_RATIO;
I guess i could use clamp() macro.
> -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
[Jacob Pan]
-- Thanks,
Jacob
| |