This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [RFC/PATCH] replace preempt_* calls with rcu_read_* variants
- From: Mathieu Desnoyers <compudj at krystal dot dyndns dot org>
- To: Ananth N Mavinakayanahalli <ananth at in dot ibm dot com>
- Cc: SystemTAP <systemtap at sources dot redhat dot com>
- Date: Tue, 14 Mar 2006 09:00:11 -0500
- Subject: Re: [RFC/PATCH] replace preempt_* calls with rcu_read_* variants
- References: <20060314134415.GA16136@in.ibm.com>
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 <ananth@in.ibm.com>
>
>
> 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 <linux/config.h>
> #include <linux/kprobes.h>
> #include <linux/ptrace.h>
> -#include <linux/preempt.h>
> #include <asm/cacheflush.h>
> #include <asm/kdebug.h>
> #include <asm/desc.h>
> @@ -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 <linux/ptrace.h>
> #include <linux/string.h>
> #include <linux/slab.h>
> -#include <linux/preempt.h>
> #include <linux/moduleloader.h>
>
> #include <asm/pgtable.h>
> @@ -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 <linux/config.h>
> #include <linux/kprobes.h>
> #include <linux/ptrace.h>
> -#include <linux/preempt.h>
> #include <asm/cacheflush.h>
> #include <asm/kdebug.h>
> #include <asm/sstep.h>
> @@ -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 <linux/ptrace.h>
> #include <linux/string.h>
> #include <linux/slab.h>
> -#include <linux/preempt.h>
>
> #include <asm/cacheflush.h>
> #include <asm/pgtable.h>
> @@ -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