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

[David Rientjes <rientjes@google.com>]

On Tue, 29 May 2007, Stephane Eranian wrote:

> --- linux-2.6.22.base/perfmon/perfmon_ctxsw.c	1969-12-31 16:00:00.000000000 -0800
> +++ linux-2.6.22/perfmon/perfmon_ctxsw.c	2007-05-29 03:24:14.000000000 -0700
> @@ -0,0 +1,341 @@
> +/*
> + * perfmon_cxtsw.c: perfmon2 context switch code
> + *
> + * This file implements the perfmon2 interface which
> + * provides access to the hardware performance counters
> + * of the host processor.
> + *
> + * The initial version of perfmon.c was written by
> + * Ganesh Venkitachalam, IBM Corp.
> + *
> + * Then it was modified for perfmon-1.x by Stephane Eranian and
> + * David Mosberger, Hewlett Packard Co.
> + *
> + * Version Perfmon-2.x is a complete rewrite of perfmon-1.x
> + * by Stephane Eranian, Hewlett Packard Co.
> + *
> + * Copyright (c) 1999-2006 Hewlett-Packard Development Company, L.P.
> + * Contributed by Stephane Eranian <eranian@hpl.hp.com>
> + *                David Mosberger-Tang <davidm@hpl.hp.com>
> + *
> + * More information about perfmon available at:
> + * 	http://perfmon2.sf.net
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> + * 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()?

> +	struct pfm_event_set *set;
> +
> +	/*
> +	 * in UP per-thread, due to lazy save
> +	 * there could be a context from another
> +	 * task. We need to push it first before
> +	 * installing our new state
> +	 */
> +	set = ctxp->active_set;
> +	pfm_modview_begin(set);
> +	pfm_save_pmds(ctxp, set);
> +	set->view->set_status &= ~PFM_SETVFL_ACTIVE;
> +	pfm_modview_end(set);
> +	/*
> +	 * do not clear ownership because we rewrite
> +	 * right away
> +	 */
> +}
> +
> +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--".

> +		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.

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