MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Message-ID: <19186.45014.502448.698606@cargo.ozlabs.ibm.com>
Date: Thu, 5 Nov 2009 21:58:30 +1100
From: Paul Mackerras <paulus@samba.org>
To: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>, LKML <linux-kernel@vger.kernel.org>,
       Prasad <prasad@linux.vnet.ibm.com>,
       Alan Stern <stern@rowland.harvard.edu>,
       Peter Zijlstra <peterz@infradead.org>,
       Arnaldo Carvalho de Melo <acme@redhat.com>,
       Steven Rostedt <rostedt@goodmis.org>, Jan Kiszka <jan.kiszka@web.de>,
       Jiri Slaby <jirislaby@gmail.com>, Li Zefan <lizf@cn.fujitsu.com>,
       Avi Kivity <avi@redhat.com>, Mike Galbraith <efault@gmx.de>,
       Masami Hiramatsu <mhiramat@redhat.com>,
       Paul Mundt <lethal@linux-sh.org>
Subject: Re: [PATCH 5/6] hw-breakpoints: Arbitrate access to pmu following registers constraints
In-Reply-To: <1257275474-5285-6-git-send-email-fweisbec@gmail.com>
References: <1257275474-5285-1-git-send-email-fweisbec@gmail.com>
	<1257275474-5285-6-git-send-email-fweisbec@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1487
Lines: 48

Frederic Weisbecker writes:

> Allow or refuse to build a counter using the breakpoints pmu following
> given constraints.

As far as I can see, you assume each CPU has HBP_NUM breakpoint
registers which are all interchangeable and can all be used either for
data breakpoints or instruction breakpoints.  Is that accurate?

If so, we'll need to extend it a bit for Power since we have some CPUs
that have one data breakpoint register and one instruction breakpoint
register.  In general on powerpc the instruction and data breakpoint
facilities are separate, i.e. we have no registers that can be used
for either.

> +static void toggle_bp_slot(struct perf_event *bp, bool enable)
> +{
> +	int cpu = bp->cpu;
> +	unsigned int *nr;
> +	struct task_struct *tsk = bp->ctx->task;
> +
> +	/* Flexible */
> +	if (!bp->attr.pinned) {
> +		if (cpu >= 0) {
> +			nr = &per_cpu(nr_bp_flexible, cpu);
> +			goto toggle;
> +		}
> +
> +		for_each_online_cpu(cpu) {
> +			nr = &per_cpu(nr_bp_flexible, cpu);
> +			goto toggle;

...

> +toggle:
> +	*nr = enable ? *nr + 1 : *nr - 1;
> +}

This won't do what I think you want.  In the case where
!bp->attr.pinned and cpu == -1, it will only update the count for the
first online cpu, not all of them.

Paul.
--
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/