From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30449 invoked by alias); 14 Mar 2006 14:00:29 -0000 Received: (qmail 30119 invoked by uid 22791); 14 Mar 2006 14:00:23 -0000 X-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from tomts13.bellnexxia.net (HELO tomts13-srv.bellnexxia.net) (209.226.175.34) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 14 Mar 2006 14:00:16 +0000 Received: from krystal.dyndns.org ([67.68.250.154]) by tomts13-srv.bellnexxia.net (InterMail vM.5.01.06.13 201-253-122-130-113-20050324) with ESMTP id <20060314140012.OVSJ29052.tomts13-srv.bellnexxia.net@krystal.dyndns.org> for ; Tue, 14 Mar 2006 09:00:12 -0500 Received: from localhost (localhost [127.0.0.1]) (uid 1000) by krystal.dyndns.org with local; Tue, 14 Mar 2006 09:00:11 -0500 id 0022A833.4416CC6B.000038C9 Date: Tue, 14 Mar 2006 14:00:00 -0000 From: Mathieu Desnoyers To: Ananth N Mavinakayanahalli Cc: SystemTAP Subject: Re: [RFC/PATCH] replace preempt_* calls with rcu_read_* variants Message-ID: <20060314140011.GB55@Krystal> References: <20060314134415.GA16136@in.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline In-Reply-To: <20060314134415.GA16136@in.ibm.com> X-Editor: vi X-Info: http://krystal.dyndns.org:8080 X-Operating-System: Linux/2.4.31-grsec (i686) X-Uptime: 08:58:10 up 1 day, 21:15, 3 users, load average: 0.98, 0.93, 0.80 User-Agent: Mutt/1.5.11+cvs20060126 X-IsSubscribed: yes Mailing-List: contact systemtap-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Post: List-Help: , Sender: systemtap-owner@sourceware.org X-SW-Source: 2006-q1/txt/msg00783.txt.bz2 Just beware that preempt_enable() should not be called from the scheduler, as it can trigger a wakeup and cause a deadlock. Mathieu * Ananth N Mavinakayanahalli (ananth@in.ibm.com) wrote: > Hi, > > Can somebody please remind me why we are not using rcu_read_unlock/lock() > to RCU protect the kprobe handlers, instead, are using > preempt_disable/enable()? > > We do use preempt_enable_no_resched() in the code. Is there a fallout if > we check need_resched()? > > Realistically, I'd like us to adhere to the RCU locking framework. The > RT changes are trickling in to the mainline and it includes changes to > the RCU infrastructure. So, read_lock_rcu() which, as of now is just a > preempt_disable(), may change to something more than that in the future. > > Basically, a plain preempt_disable/enable() may not protect us in the > RCU read side critical section for much longer. > > The following patch is against 2.6.16-rc6 - tested on IA32. > > Comments? Thoughts? > > Ananth > --- > > Signed-off-by: Ananth N Mavinakayanahalli > > > arch/i386/kernel/kprobes.c | 27 +++++++++++---------------- > arch/ia64/kernel/kprobes.c | 27 +++++++++++---------------- > arch/powerpc/kernel/kprobes.c | 27 +++++++++++---------------- > arch/sparc64/kernel/kprobes.c | 19 ++++++++----------- > arch/x86_64/kernel/kprobes.c | 27 +++++++++++---------------- > 5 files changed, 52 insertions(+), 75 deletions(-) > > Index: linux-2.6.16-rc6/arch/i386/kernel/kprobes.c > =================================================================== > --- linux-2.6.16-rc6.orig/arch/i386/kernel/kprobes.c > +++ linux-2.6.16-rc6/arch/i386/kernel/kprobes.c > @@ -31,7 +31,6 @@ > #include > #include > #include > -#include > #include > #include > #include > @@ -159,11 +158,8 @@ static int __kprobes kprobe_handler(stru > unsigned long *lp; > struct kprobe_ctlblk *kcb; > > - /* > - * We don't want to be preempted for the entire > - * duration of kprobe processing > - */ > - preempt_disable(); > + /* Make entire duration of kprobe processing RCU safe */ > + rcu_read_lock(); > kcb = get_kprobe_ctlblk(); > > /* Check if the application is using LDT entry for its code segment and > @@ -258,7 +254,7 @@ ss_probe: > return 1; > > no_kprobe: > - preempt_enable_no_resched(); > + rcu_read_unlock(); > return ret; > } > > @@ -326,12 +322,11 @@ int __kprobes trampoline_probe_handler(s > > reset_current_kprobe(); > spin_unlock_irqrestore(&kretprobe_lock, flags); > - preempt_enable_no_resched(); > + rcu_read_unlock(); > > /* > - * By returning a non-zero value, we are telling > - * kprobe_handler() that we don't want the post_handler > - * to run (and have re-enabled preemption) > + * By returning a non-zero value, we are telling kprobe_handler() > + * that we don't want the post_handler to run > */ > return 1; > } > @@ -435,7 +430,7 @@ static inline int post_kprobe_handler(st > } > reset_current_kprobe(); > out: > - preempt_enable_no_resched(); > + rcu_read_unlock(); > > /* > * if somebody else is singlestepping across a probe point, eflags > @@ -461,7 +456,7 @@ static inline int kprobe_fault_handler(s > regs->eflags |= kcb->kprobe_old_eflags; > > reset_current_kprobe(); > - preempt_enable_no_resched(); > + rcu_read_unlock(); > } > return 0; > } > @@ -487,11 +482,11 @@ int __kprobes kprobe_exceptions_notify(s > case DIE_GPF: > case DIE_PAGE_FAULT: > /* kprobe_running() needs smp_processor_id() */ > - preempt_disable(); > + rcu_read_lock(); > if (kprobe_running() && > kprobe_fault_handler(args->regs, args->trapnr)) > ret = NOTIFY_STOP; > - preempt_enable(); > + rcu_read_unlock(); > break; > default: > break; > @@ -558,7 +553,7 @@ int __kprobes longjmp_break_handler(stru > *regs = kcb->jprobe_saved_regs; > memcpy((kprobe_opcode_t *) stack_addr, kcb->jprobes_stack, > MIN_STACK_SIZE(stack_addr)); > - preempt_enable_no_resched(); > + rcu_read_unlock(); > return 1; > } > return 0; > Index: linux-2.6.16-rc6/arch/ia64/kernel/kprobes.c > =================================================================== > --- linux-2.6.16-rc6.orig/arch/ia64/kernel/kprobes.c > +++ linux-2.6.16-rc6/arch/ia64/kernel/kprobes.c > @@ -28,7 +28,6 @@ > #include > #include > #include > -#include > #include > > #include > @@ -387,12 +386,11 @@ int __kprobes trampoline_probe_handler(s > > reset_current_kprobe(); > spin_unlock_irqrestore(&kretprobe_lock, flags); > - preempt_enable_no_resched(); > + rcu_read_unlock(); > > /* > - * By returning a non-zero value, we are telling > - * kprobe_handler() that we don't want the post_handler > - * to run (and have re-enabled preemption) > + * By returning a non-zero value, we are telling kprobe_handler() > + * that we don't want the post_handler to run > */ > return 1; > } > @@ -602,11 +600,8 @@ static int __kprobes pre_kprobes_handler > kprobe_opcode_t *addr = (kprobe_opcode_t *)instruction_pointer(regs); > struct kprobe_ctlblk *kcb; > > - /* > - * We don't want to be preempted for the entire > - * duration of kprobe processing > - */ > - preempt_disable(); > + /* Make the entire duration of kprobe processing RCU safe */ > + rcu_read_lock(); > kcb = get_kprobe_ctlblk(); > > /* Handle recursion cases */ > @@ -686,7 +681,7 @@ ss_probe: > return 1; > > no_kprobe: > - preempt_enable_no_resched(); > + rcu_read_unlock(); > return ret; > } > > @@ -713,7 +708,7 @@ static int __kprobes post_kprobes_handle > reset_current_kprobe(); > > out: > - preempt_enable_no_resched(); > + rcu_read_unlock(); > return 1; > } > > @@ -728,7 +723,7 @@ static int __kprobes kprobes_fault_handl > if (kcb->kprobe_status & KPROBE_HIT_SS) { > resume_execution(cur, regs); > reset_current_kprobe(); > - preempt_enable_no_resched(); > + rcu_read_unlock(); > } > > return 0; > @@ -755,11 +750,11 @@ int __kprobes kprobe_exceptions_notify(s > break; > case DIE_PAGE_FAULT: > /* kprobe_running() needs smp_processor_id() */ > - preempt_disable(); > + rcu_read_lock(); > if (kprobe_running() && > kprobes_fault_handler(args->regs, args->trapnr)) > ret = NOTIFY_STOP; > - preempt_enable(); > + rcu_read_unlock(); > default: > break; > } > @@ -851,7 +846,7 @@ int __kprobes longjmp_break_handler(stru > bytes ); > invalidate_stacked_regs(); > > - preempt_enable_no_resched(); > + rcu_read_unlock(); > return 1; > } > > Index: linux-2.6.16-rc6/arch/powerpc/kernel/kprobes.c > =================================================================== > --- linux-2.6.16-rc6.orig/arch/powerpc/kernel/kprobes.c > +++ linux-2.6.16-rc6/arch/powerpc/kernel/kprobes.c > @@ -30,7 +30,6 @@ > #include > #include > #include > -#include > #include > #include > #include > @@ -147,11 +146,8 @@ static inline int kprobe_handler(struct > unsigned int *addr = (unsigned int *)regs->nip; > struct kprobe_ctlblk *kcb; > > - /* > - * We don't want to be preempted for the entire > - * duration of kprobe processing > - */ > - preempt_disable(); > + /* Make the entire duration of kprobe processing RCU safe */ > + rcu_read_lock(); > kcb = get_kprobe_ctlblk(); > > /* Check we're not actually recursing */ > @@ -235,7 +231,7 @@ ss_probe: > return 1; > > no_kprobe: > - preempt_enable_no_resched(); > + rcu_read_unlock(); > return ret; > } > > @@ -304,12 +300,11 @@ int __kprobes trampoline_probe_handler(s > > reset_current_kprobe(); > spin_unlock_irqrestore(&kretprobe_lock, flags); > - preempt_enable_no_resched(); > + rcu_read_unlock(); > > /* > - * By returning a non-zero value, we are telling > - * kprobe_handler() that we don't want the post_handler > - * to run (and have re-enabled preemption) > + * By returning a non-zero value, we are telling kprobe_handler() > + * that we don't want the post_handler to run > */ > return 1; > } > @@ -356,7 +351,7 @@ static inline int post_kprobe_handler(st > } > reset_current_kprobe(); > out: > - preempt_enable_no_resched(); > + rcu_read_unlock(); > > /* > * if somebody else is singlestepping across a probe point, msr > @@ -383,7 +378,7 @@ static inline int kprobe_fault_handler(s > regs->msr |= kcb->kprobe_saved_msr; > > reset_current_kprobe(); > - preempt_enable_no_resched(); > + rcu_read_unlock(); > } > return 0; > } > @@ -408,11 +403,11 @@ int __kprobes kprobe_exceptions_notify(s > break; > case DIE_PAGE_FAULT: > /* kprobe_running() needs smp_processor_id() */ > - preempt_disable(); > + rcu_read_lock(); > if (kprobe_running() && > kprobe_fault_handler(args->regs, args->trapnr)) > ret = NOTIFY_STOP; > - preempt_enable(); > + rcu_read_unlock(); > break; > default: > break; > @@ -453,7 +448,7 @@ int __kprobes longjmp_break_handler(stru > * saved regs... > */ > memcpy(regs, &kcb->jprobe_saved_regs, sizeof(struct pt_regs)); > - preempt_enable_no_resched(); > + rcu_read_unlock(); > return 1; > } > > Index: linux-2.6.16-rc6/arch/sparc64/kernel/kprobes.c > =================================================================== > --- linux-2.6.16-rc6.orig/arch/sparc64/kernel/kprobes.c > +++ linux-2.6.16-rc6/arch/sparc64/kernel/kprobes.c > @@ -107,11 +107,8 @@ static int __kprobes kprobe_handler(stru > int ret = 0; > struct kprobe_ctlblk *kcb; > > - /* > - * We don't want to be preempted for the entire > - * duration of kprobe processing > - */ > - preempt_disable(); > + /* Make the entire duration of kprobe processing RCU safe */ > + rcu_read_lock(); > kcb = get_kprobe_ctlblk(); > > if (kprobe_running()) { > @@ -177,7 +174,7 @@ ss_probe: > return 1; > > no_kprobe: > - preempt_enable_no_resched(); > + rcu_read_unlock(); > return ret; > } > > @@ -293,7 +290,7 @@ static inline int post_kprobe_handler(st > } > reset_current_kprobe(); > out: > - preempt_enable_no_resched(); > + rcu_read_unlock(); > > return 1; > } > @@ -310,7 +307,7 @@ static inline int kprobe_fault_handler(s > resume_execution(cur, regs, kcb); > > reset_current_kprobe(); > - preempt_enable_no_resched(); > + rcu_read_unlock(); > } > return 0; > } > @@ -336,11 +333,11 @@ int __kprobes kprobe_exceptions_notify(s > case DIE_GPF: > case DIE_PAGE_FAULT: > /* kprobe_running() needs smp_processor_id() */ > - preempt_disable(); > + rcu_read_lock(); > if (kprobe_running() && > kprobe_fault_handler(args->regs, args->trapnr)) > ret = NOTIFY_STOP; > - preempt_enable(); > + rcu_read_unlock(); > break; > default: > break; > @@ -430,7 +427,7 @@ int __kprobes longjmp_break_handler(stru > &(kcb->jprobe_saved_stack), > sizeof(kcb->jprobe_saved_stack)); > > - preempt_enable_no_resched(); > + rcu_read_unlock(); > return 1; > } > return 0; > Index: linux-2.6.16-rc6/arch/x86_64/kernel/kprobes.c > =================================================================== > --- linux-2.6.16-rc6.orig/arch/x86_64/kernel/kprobes.c > +++ linux-2.6.16-rc6/arch/x86_64/kernel/kprobes.c > @@ -36,7 +36,6 @@ > #include > #include > #include > -#include > > #include > #include > @@ -292,11 +291,8 @@ int __kprobes kprobe_handler(struct pt_r > kprobe_opcode_t *addr = (kprobe_opcode_t *)(regs->rip - sizeof(kprobe_opcode_t)); > struct kprobe_ctlblk *kcb; > > - /* > - * We don't want to be preempted for the entire > - * duration of kprobe processing > - */ > - preempt_disable(); > + /* Make the entire duration of kprobe processing RCU safe */ > + rcu_read_lock(); > kcb = get_kprobe_ctlblk(); > > /* Check we're not actually recursing */ > @@ -383,7 +379,7 @@ ss_probe: > return 1; > > no_kprobe: > - preempt_enable_no_resched(); > + rcu_read_unlock(); > return ret; > } > > @@ -451,12 +447,11 @@ int __kprobes trampoline_probe_handler(s > > reset_current_kprobe(); > spin_unlock_irqrestore(&kretprobe_lock, flags); > - preempt_enable_no_resched(); > + rcu_read_unlock(); > > /* > - * By returning a non-zero value, we are telling > - * kprobe_handler() that we don't want the post_handler > - * to run (and have re-enabled preemption) > + * By returning a non-zero value, we are telling kprobe_handler() > + * that we don't want the post_handler to run > */ > return 1; > } > @@ -561,7 +556,7 @@ int __kprobes post_kprobe_handler(struct > } > reset_current_kprobe(); > out: > - preempt_enable_no_resched(); > + rcu_read_unlock(); > > /* > * if somebody else is singlestepping across a probe point, eflags > @@ -587,7 +582,7 @@ int __kprobes kprobe_fault_handler(struc > regs->eflags |= kcb->kprobe_old_rflags; > > reset_current_kprobe(); > - preempt_enable_no_resched(); > + rcu_read_unlock(); > } > return 0; > } > @@ -613,11 +608,11 @@ int __kprobes kprobe_exceptions_notify(s > case DIE_GPF: > case DIE_PAGE_FAULT: > /* kprobe_running() needs smp_processor_id() */ > - preempt_disable(); > + rcu_read_lock(); > if (kprobe_running() && > kprobe_fault_handler(args->regs, args->trapnr)) > ret = NOTIFY_STOP; > - preempt_enable(); > + rcu_read_unlock(); > break; > default: > break; > @@ -683,7 +678,7 @@ int __kprobes longjmp_break_handler(stru > *regs = kcb->jprobe_saved_regs; > memcpy((kprobe_opcode_t *) stack_addr, kcb->jprobes_stack, > MIN_STACK_SIZE(stack_addr)); > - preempt_enable_no_resched(); > + rcu_read_unlock(); > return 1; > } > return 0; > OpenPGP public key: http://krystal.dyndns.org:8080/key/compudj.gpg Key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68