From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21531 invoked by alias); 14 Mar 2006 13:44:29 -0000 Received: (qmail 21524 invoked by uid 22791); 14 Mar 2006 13:44:27 -0000 X-Spam-Status: No, hits=-0.8 required=5.0 tests=AWL,BAYES_00,DNS_FROM_RFC_ABUSE,SPF_SOFTFAIL X-Spam-Check-By: sourceware.org Received: from e32.co.us.ibm.com (HELO e32.co.us.ibm.com) (32.97.110.150) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 14 Mar 2006 13:44:26 +0000 Received: from westrelay02.boulder.ibm.com (westrelay02.boulder.ibm.com [9.17.195.11]) by e32.co.us.ibm.com (8.12.11/8.12.11) with ESMTP id k2EDiOix007987 for ; Tue, 14 Mar 2006 08:44:24 -0500 Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by westrelay02.boulder.ibm.com (8.12.10/NCO/VER6.8) with ESMTP id k2EDfXLl255852 for ; Tue, 14 Mar 2006 06:41:33 -0700 Received: from d03av01.boulder.ibm.com (loopback [127.0.0.1]) by d03av01.boulder.ibm.com (8.12.11/8.13.3) with ESMTP id k2EDiN59021428 for ; Tue, 14 Mar 2006 06:44:24 -0700 Received: from thinktux.in.ibm.com ([9.124.35.22]) by d03av01.boulder.ibm.com (8.12.11/8.12.11) with ESMTP id k2EDiLba021380 for ; Tue, 14 Mar 2006 06:44:22 -0700 Received: from thinktux.in.ibm.com (unknown [127.0.0.1]) by thinktux.in.ibm.com (Postfix) with ESMTP id 41E4712B6E0 for ; Tue, 14 Mar 2006 19:14:16 +0530 (IST) Received: (from ananth@localhost) by thinktux.in.ibm.com (8.12.8/8.12.8/Submit) id k2EDiFp7016602 for systemtap@sources.redhat.com; Tue, 14 Mar 2006 19:14:15 +0530 Date: Tue, 14 Mar 2006 13:44:00 -0000 From: Ananth N Mavinakayanahalli To: SystemTAP Subject: [RFC/PATCH] replace preempt_* calls with rcu_read_* variants Message-ID: <20060314134415.GA16136@in.ibm.com> Reply-To: ananth@in.ibm.com Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.4.1i 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/msg00782.txt.bz2 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;