All of lore.kernel.org
 help / color / mirror / Atom feed
From: Oleg Nesterov <oleg@redhat.com>
To: Vince Weaver <vincent.weaver@maine.edu>
Cc: linux-kernel@vger.kernel.org,
	"Peter Zijlstra" <a.p.zijlstra@chello.nl>,
	"Paul Mackerras" <paulus@samba.org>,
	"Ingo Molnar" <mingo@redhat.com>,
	"Arnaldo Carvalho de Melo" <acme@ghostprotocols.net>,
	trinity@vger.kernel.org,
	"Frédéric Weisbecker" <fweisbec@gmail.com>,
	"Jiri Olsa" <jolsa@redhat.com>
Subject: [MAYBEPATCH] : WARN_ONCE in arch/x86/kernel/hw_breakpoint.c
Date: Wed, 29 May 2013 18:32:57 +0200	[thread overview]
Message-ID: <20130529163257.GA31720@redhat.com> (raw)
In-Reply-To: <20130528184702.GA6455@redhat.com>

OK, I seem to understand what toggle_bp_task_slot() does, and
it looks equally wrong.

I think we need something like the patch below... I'll try to
recheck/test tomorrow, but I would not mind if someone who knows
this code makes the authoritative fix.

Even if this patch is right, I think this all needs more cleanups,
at least. For example, every DEFINE_PER_CPU() looks bogus. This
is not pcpu memory.

Oleg.

--- x/kernel/events/hw_breakpoint.c
+++ x/kernel/events/hw_breakpoint.c
@@ -111,7 +111,7 @@ static unsigned int max_task_bp_pinned(int cpu, enum bp_type_idx type)
  * Count the number of breakpoints of the same type and same task.
  * The given event must be not on the list.
  */
-static int task_bp_pinned(int cpu, struct perf_event *bp, enum bp_type_idx type)
+static int task_bp_pinned(struct perf_event *bp, enum bp_type_idx type)
 {
 	struct task_struct *tsk = bp->hw.bp_target;
 	struct perf_event *iter;
@@ -120,7 +120,7 @@ static int task_bp_pinned(int cpu, struct perf_event *bp, enum bp_type_idx type)
 	list_for_each_entry(iter, &bp_task_head, hw.bp_list) {
 		if (iter->hw.bp_target == tsk &&
 		    find_slot_idx(iter) == type &&
-		    cpu == iter->cpu)
+		    bp->cpu == iter->cpu)
 			count += hw_breakpoint_weight(iter);
 	}
 
@@ -137,13 +137,17 @@ fetch_bp_busy_slots(struct bp_busy_slots *slots, struct perf_event *bp,
 {
 	int cpu = bp->cpu;
 	struct task_struct *tsk = bp->hw.bp_target;
+	int task_pinned;
+
+	if (tsk)
+		task_pinned = task_bp_pinned(bp, type);
 
 	if (cpu >= 0) {
 		slots->pinned = per_cpu(nr_cpu_bp_pinned[type], cpu);
 		if (!tsk)
 			slots->pinned += max_task_bp_pinned(cpu, type);
 		else
-			slots->pinned += task_bp_pinned(cpu, bp, type);
+			slots->pinned += task_pinned;
 		slots->flexible = per_cpu(nr_bp_flexible[type], cpu);
 
 		return;
@@ -156,7 +160,7 @@ fetch_bp_busy_slots(struct bp_busy_slots *slots, struct perf_event *bp,
 		if (!tsk)
 			nr += max_task_bp_pinned(cpu, type);
 		else
-			nr += task_bp_pinned(cpu, bp, type);
+			nr += task_pinned;
 
 		if (nr > slots->pinned)
 			slots->pinned = nr;
@@ -182,15 +186,13 @@ fetch_this_slot(struct bp_busy_slots *slots, int weight)
 /*
  * Add a pinned breakpoint for the given task in our constraint table
  */
-static void toggle_bp_task_slot(struct perf_event *bp, int cpu, bool enable,
+static void toggle_bp_task_slot(int old_count, int cpu, bool enable,
 				enum bp_type_idx type, int weight)
 {
 	unsigned int *tsk_pinned;
-	int old_count = 0;
 	int old_idx = 0;
 	int idx = 0;
 
-	old_count = task_bp_pinned(cpu, bp, type);
 	old_idx = old_count - 1;
 	idx = old_idx + weight;
 
@@ -216,6 +218,7 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type,
 {
 	int cpu = bp->cpu;
 	struct task_struct *tsk = bp->hw.bp_target;
+	int task_pinned;
 
 	/* Pinned counter cpu profiling */
 	if (!tsk) {
@@ -232,11 +235,13 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type,
 	if (!enable)
 		list_del(&bp->hw.bp_list);
 
+	task_pinned = task_bp_pinned(bp, type);
+
 	if (cpu >= 0) {
-		toggle_bp_task_slot(bp, cpu, enable, type, weight);
+		toggle_bp_task_slot(task_pinned, cpu, enable, type, weight);
 	} else {
 		for_each_online_cpu(cpu)
-			toggle_bp_task_slot(bp, cpu, enable, type, weight);
+			toggle_bp_task_slot(task_pinned, cpu, enable, type, weight);
 	}
 
 	if (enable)

  reply	other threads:[~2013-05-29 16:32 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-05-20 16:19 WARN_ONCE in arch/x86/kernel/hw_breakpoint.c Vince Weaver
2013-05-28 17:00 ` Oleg Nesterov
2013-05-28 17:28   ` Oleg Nesterov
2013-05-28 18:47     ` Oleg Nesterov
2013-05-29 16:32       ` Oleg Nesterov [this message]
2013-06-01 18:20 ` [PATCH 0/2]: " Oleg Nesterov
2013-06-01 18:21   ` [PATCH 1/2] hw_breakpoint: Fix cpu check in task_bp_pinned(cpu) Oleg Nesterov
2013-06-13 14:20     ` Frederic Weisbecker
2013-06-01 18:21   ` [PATCH 2/2] hw_breakpoint: Use cpu_possible_mask in {reserve,release}_bp_slot() Oleg Nesterov
2013-06-15 12:46     ` Frederic Weisbecker
2013-06-01 19:45 ` [PATCH 0/3] hw_breakpoint: cleanups Oleg Nesterov
2013-06-01 19:45   ` [PATCH 1/3] hw_breakpoint: Simplify list/idx mess in toggle_bp_slot() paths Oleg Nesterov
2013-06-15 12:59     ` Frederic Weisbecker
2013-06-01 19:46   ` [PATCH 2/3] hw_breakpoint: Simplify the "weight" usage " Oleg Nesterov
2013-06-15 13:14     ` Frederic Weisbecker
2013-06-01 19:46   ` [PATCH 3/3] hw_breakpoint: Introduce cpumask_of_bp() Oleg Nesterov
2013-06-15 13:29     ` Frederic Weisbecker
2013-06-13 14:01   ` [PATCH 0/3] hw_breakpoint: cleanups Frederic Weisbecker
2013-06-13 15:15     ` Oleg Nesterov
2013-06-13 15:24       ` Frederic Weisbecker
2013-06-02 19:49 ` [PATCH 0/2] hw_breakpoint: more cleanups Oleg Nesterov
2013-06-02 19:50   ` [PATCH 1/2] hw_breakpoint: Simplify *register_wide_hw_breakpoint() Oleg Nesterov
2013-06-18  0:12     ` Frederic Weisbecker
2013-06-02 19:50   ` [PATCH 2/2] hw_breakpoint: Introduce "struct bp_cpuinfo" Oleg Nesterov
2013-06-18 12:37     ` Frederic Weisbecker
2013-06-18 14:42       ` Oleg Nesterov
2013-06-18 17:01         ` Frederic Weisbecker
2013-06-19 15:54           ` Oleg Nesterov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130529163257.GA31720@redhat.com \
    --to=oleg@redhat.com \
    --cc=a.p.zijlstra@chello.nl \
    --cc=acme@ghostprotocols.net \
    --cc=fweisbec@gmail.com \
    --cc=jolsa@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=paulus@samba.org \
    --cc=trinity@vger.kernel.org \
    --cc=vincent.weaver@maine.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.