Re: [PATCH 10/22] 2.6.22-rc3 perfmon2 : PMU context switch support

[Stephane Eranian <eranian@hpl.hp.com>]

David,

On Mon, Jun 04, 2007 at 07:50:27AM -0700, David Rientjes wrote:
> > + * 02111-1307 USA
> > + */
> > +#include <linux/kernel.h>
> > +#include <linux/perfmon.h>
> > +
> > +/*
> > + * used only in UP mode
> > + */
> > +void pfm_save_prev_context(struct pfm_context *ctxp)
> > +{
> 
> Is this comment still valid even though there's a call to this function in 
> the self-monitoring case in __pfm_load_context()?
> 
Yes. I have added the #ifndef CONFIG_SMP if __pfm_load_context(). The reason
for this is that in UP mode we use lazy save/restore on context switch. Thus
when you come in to load a context on the PMU, you may need to push the previous
context out.

> > +
> > +void pfm_save_pmds(struct pfm_context *ctx, struct pfm_event_set *set)
> > +{
> > +	u64 val, hw_val, *pmds, ovfl_mask;
> > +	u64 *used_mask, *cnt_mask;
> > +	u16 i, num;
> > +
> > +	ovfl_mask = pfm_pmu_conf->ovfl_mask;
> > +	num = set->nused_pmds;
> > +	cnt_mask = pfm_pmu_conf->cnt_pmds;
> > +	used_mask = set->used_pmds;
> > +	pmds = set->view->set_pmds;
> > +
> > +	for (i = 0; num; i++) {
> 
> This should probably be "i++, num--".
> 
No. Num represents the number of PMD registers we need to read. We only
read the PMD registers we use. Thus num needs to be decremented only when
i is the index of a *used* PM register.

> > +		if (test_bit(i, ulp(used_mask))) {
> > +			hw_val = val = pfm_read_pmd(ctx, i);
> > +			if (likely(test_bit(i, ulp(cnt_mask))))
> > +				val = (pmds[i] & ~ovfl_mask) |
> > +					(hw_val & ovfl_mask);
> > +			pmds[i] = val;
> > +			num--;
> > +		}
> > +	}
> > +}
> 
> You can elimiate the hw_val automatic by eliminating its assignment and 
> replacing its use with val.
> 
Done.

Thanks for your feedback.

-- 

-Stephane
-
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/