From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755458Ab2ITOQX (ORCPT ); Thu, 20 Sep 2012 10:16:23 -0400 Received: from e39.co.us.ibm.com ([32.97.110.160]:53080 "EHLO e39.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755434Ab2ITOQV (ORCPT ); Thu, 20 Sep 2012 10:16:21 -0400 Date: Thu, 20 Sep 2012 19:36:09 +0530 From: Srikar Dronamraju To: Oleg Nesterov Cc: Ingo Molnar , Peter Zijlstra , Ananth N Mavinakayanahalli , Anton Arapov , Sebastian Andrzej Siewior , linux-kernel@vger.kernel.org Subject: Re: [PATCH 4/5] uprobes: Kill UTASK_BP_HIT state Message-ID: <20120920140609.GD27880@linux.vnet.ibm.com> Reply-To: Srikar Dronamraju References: <20120914171513.GA29599@redhat.com> <20120914171600.GA29649@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20120914171600.GA29649@redhat.com> User-Agent: Mutt/1.5.20 (2009-06-14) X-Content-Scanned: Fidelis XPS MAILER x-cbid: 12092014-4242-0000-0000-000002F6B32D Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Oleg Nesterov [2012-09-14 19:16:00]: > Kill UTASK_BP_HIT state, it buys nothing but complicates the code. > It is only used in uprobe_notify_resume() to decide who should be > called, we can check utask->active_uprobe != NULL instead. And this > allows us to simplify handle_swbp(), no need to clear utask->state. > > Likewise we could kill UTASK_SSTEP, but UTASK_BP_HIT is worse and > imho should die. The problem is, it creates the special case when > task->utask is NULL, we can't distinguish RUNNING and BP_HIT. With > his patch utask == NULL always means RUNNING. > > Signed-off-by: Oleg Nesterov Acked-by: Srikar Dronamraju > --- > include/linux/uprobes.h | 1 - > kernel/events/uprobes.c | 29 +++++++++-------------------- > 2 files changed, 9 insertions(+), 21 deletions(-) > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h > index e6f0331..18d839d 100644 > --- a/include/linux/uprobes.h > +++ b/include/linux/uprobes.h > @@ -59,7 +59,6 @@ struct uprobe_consumer { > #ifdef CONFIG_UPROBES > enum uprobe_task_state { > UTASK_RUNNING, > - UTASK_BP_HIT, > UTASK_SSTEP, > UTASK_SSTEP_ACK, > UTASK_SSTEP_TRAPPED, > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index 403d2e0..4ea0f0b 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -1469,10 +1469,6 @@ static void handle_swbp(struct pt_regs *regs) > bp_vaddr = uprobe_get_swbp_addr(regs); > uprobe = find_active_uprobe(bp_vaddr, &is_swbp); > > - utask = current->utask; > - if (utask) > - utask->state = UTASK_RUNNING; > - > if (!uprobe) { > if (is_swbp > 0) { > /* No matching uprobe; signal SIGTRAP. */ > @@ -1491,6 +1487,7 @@ static void handle_swbp(struct pt_regs *regs) > return; > } > > + utask = current->utask; > if (!utask) { > utask = add_utask(); > /* Cannot allocate; re-execute the instruction. */ > @@ -1547,13 +1544,12 @@ static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs) > } > > /* > - * On breakpoint hit, breakpoint notifier sets the TIF_UPROBE flag. (and on > - * subsequent probe hits on the thread sets the state to UTASK_BP_HIT) and > - * allows the thread to return from interrupt. > + * On breakpoint hit, breakpoint notifier sets the TIF_UPROBE flag and > + * allows the thread to return from interrupt. After that handle_swbp() > + * sets utask->active_uprobe. > * > - * On singlestep exception, singlestep notifier sets the TIF_UPROBE flag and > - * also sets the state to UTASK_SSTEP_ACK and allows the thread to return from > - * interrupt. > + * On singlestep exception, singlestep notifier sets the TIF_UPROBE flag > + * and allows the thread to return from interrupt. > * > * While returning to userspace, thread notices the TIF_UPROBE flag and calls > * uprobe_notify_resume(). > @@ -1563,10 +1559,10 @@ void uprobe_notify_resume(struct pt_regs *regs) > struct uprobe_task *utask; > > utask = current->utask; > - if (!utask || utask->state == UTASK_BP_HIT) > - handle_swbp(regs); > - else > + if (utask && utask->active_uprobe) > handle_singlestep(utask, regs); > + else > + handle_swbp(regs); > } > > /* > @@ -1575,17 +1571,10 @@ void uprobe_notify_resume(struct pt_regs *regs) > */ > int uprobe_pre_sstep_notifier(struct pt_regs *regs) > { > - struct uprobe_task *utask; > - > if (!current->mm || !test_bit(MMF_HAS_UPROBES, ¤t->mm->flags)) > return 0; > > - utask = current->utask; > - if (utask) > - utask->state = UTASK_BP_HIT; > - > set_thread_flag(TIF_UPROBE); > - > return 1; > } > > -- > 1.5.5.1 >