Re: [PATCH] Kprobes: preempt_disable/enable() reordering redux

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Oct 24, 2005 at 11:57:19AM -0400, Ananth N Mavinakayanahalli wrote:
> From: Ananth N Mavinakayanahalli <[email protected]>
> 
> This patch (over 2.6.14-rc4-mm1) reorganizes the preempt_disable/enable
> calls to eliminate the extra preempt depth. Changes based on Paul
> McKenney's review suggestions for the kprobes RCU changeset.

Looks good!!!  (Am assuming that the synchronize_sched() remains in
the 8/9 patch.)

						Thanx, Paul

Acked-by: <[email protected]>

> Signed-off-by: Ananth N Mavinakayanahalli <[email protected]>
> Signed-off-by: Anil S Keshavamurthy <[email protected]>
> ---
> 
>  arch/i386/kernel/kprobes.c    |   25 +++++++++++++++----------
>  arch/ia64/kernel/kprobes.c    |   37 ++++++++++++++++++++-----------------
>  arch/ppc64/kernel/kprobes.c   |   25 +++++++++++++++----------
>  arch/sparc64/kernel/kprobes.c |   21 +++++++++++++--------
>  arch/x86_64/kernel/kprobes.c  |   29 +++++++++++++++--------------
>  include/linux/kprobes.h       |    2 +-
>  kernel/kprobes.c              |    2 +-
>  7 files changed, 80 insertions(+), 61 deletions(-)
> 
> Index: linux-2.6.14-rc4/arch/i386/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.14-rc4.orig/arch/i386/kernel/kprobes.c	2005-10-17 18:59:51.000000000 -0400
> +++ linux-2.6.14-rc4/arch/i386/kernel/kprobes.c	2005-10-21 11:02:52.000000000 -0400
> @@ -153,7 +153,14 @@ static int __kprobes kprobe_handler(stru
>  	int ret = 0;
>  	kprobe_opcode_t *addr = NULL;
>  	unsigned long *lp;
> -	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> +	struct kprobe_ctlblk *kcb;
> +
> +	/*
> +	 * We don't want to be preempted for the entire
> +	 * duration of kprobe processing
> +	 */
> +	preempt_disable();
> +	kcb = get_kprobe_ctlblk();
>  
>  	/* Check if the application is using LDT entry for its code segment and
>  	 * calculate the address by reading the base address from the LDT entry.
> @@ -221,11 +228,6 @@ static int __kprobes kprobe_handler(stru
>  		goto no_kprobe;
>  	}
>  
> -	/*
> -	 * This preempt_disable() matches the preempt_enable_no_resched()
> -	 * in post_kprobe_handler()
> -	 */
> -	preempt_disable();
>  	set_current_kprobe(p, regs, kcb);
>  	kcb->kprobe_status = KPROBE_HIT_ACTIVE;
>  
> @@ -239,6 +241,7 @@ ss_probe:
>  	return 1;
>  
>  no_kprobe:
> +	preempt_enable_no_resched();
>  	return ret;
>  }
>  
> @@ -310,8 +313,8 @@ int __kprobes trampoline_probe_handler(s
>  
>  	/*
>  	 * By returning a non-zero value, we are telling
> -	 * kprobe_handler() that we have handled unlocking
> -	 * and re-enabling preemption
> +	 * kprobe_handler() that we don't want the post_handler
> +	 * to run (and have re-enabled preemption)
>  	 */
>          return 1;
>  }
> @@ -455,7 +458,6 @@ int __kprobes kprobe_exceptions_notify(s
>  	struct die_args *args = (struct die_args *)data;
>  	int ret = NOTIFY_DONE;
>  
> -	rcu_read_lock();
>  	switch (val) {
>  	case DIE_INT3:
>  		if (kprobe_handler(args->regs))
> @@ -467,14 +469,16 @@ int __kprobes kprobe_exceptions_notify(s
>  		break;
>  	case DIE_GPF:
>  	case DIE_PAGE_FAULT:
> +		/* kprobe_running() needs smp_processor_id() */
> +		preempt_disable();
>  		if (kprobe_running() &&
>  		    kprobe_fault_handler(args->regs, args->trapnr))
>  			ret = NOTIFY_STOP;
> +		preempt_enable();
>  		break;
>  	default:
>  		break;
>  	}
> -	rcu_read_unlock();
>  	return ret;
>  }
>  
> @@ -537,6 +541,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();
>  		return 1;
>  	}
>  	return 0;
> Index: linux-2.6.14-rc4/include/linux/kprobes.h
> ===================================================================
> --- linux-2.6.14-rc4.orig/include/linux/kprobes.h	2005-10-17 19:00:01.000000000 -0400
> +++ linux-2.6.14-rc4/include/linux/kprobes.h	2005-10-19 10:56:35.000000000 -0400
> @@ -159,7 +159,7 @@ extern void show_registers(struct pt_reg
>  extern kprobe_opcode_t *get_insn_slot(void);
>  extern void free_insn_slot(kprobe_opcode_t *slot);
>  
> -/* Get the kprobe at this addr (if any) - called under a rcu_read_lock() */
> +/* Get the kprobe at this addr (if any) - called with preemption disabled */
>  struct kprobe *get_kprobe(void *addr);
>  struct hlist_head * kretprobe_inst_table_head(struct task_struct *tsk);
>  
> Index: linux-2.6.14-rc4/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.14-rc4.orig/kernel/kprobes.c	2005-10-17 19:00:02.000000000 -0400
> +++ linux-2.6.14-rc4/kernel/kprobes.c	2005-10-18 14:19:01.000000000 -0400
> @@ -167,7 +167,7 @@ static inline void reset_kprobe_instance
>   * This routine is called either:
>   * 	- under the kprobe_lock spinlock - during kprobe_[un]register()
>   * 				OR
> - * 	- under an rcu_read_lock() - from arch/xxx/kernel/kprobes.c
> + * 	- with preemption disabled - from arch/xxx/kernel/kprobes.c
>   */
>  struct kprobe __kprobes *get_kprobe(void *addr)
>  {
> Index: linux-2.6.14-rc4/arch/ia64/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.14-rc4.orig/arch/ia64/kernel/kprobes.c	2005-10-17 18:59:52.000000000 -0400
> +++ linux-2.6.14-rc4/arch/ia64/kernel/kprobes.c	2005-10-21 13:50:10.000000000 -0400
> @@ -389,11 +389,11 @@ int __kprobes trampoline_probe_handler(s
>  	spin_unlock_irqrestore(&kretprobe_lock, flags);
>  	preempt_enable_no_resched();
>  
> -        /*
> -         * By returning a non-zero value, we are telling
> -         * kprobe_handler() that we have handled unlocking
> -	 * and re-enabling preemption
> -         */
> +	/*
> +	 * 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)
> +	 */
>          return 1;
>  }
>  
> @@ -604,7 +604,14 @@ static int __kprobes pre_kprobes_handler
>  	int ret = 0;
>  	struct pt_regs *regs = args->regs;
>  	kprobe_opcode_t *addr = (kprobe_opcode_t *)instruction_pointer(regs);
> -	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> +	struct kprobe_ctlblk *kcb;
> +
> +	/*
> +	 * We don't want to be preempted for the entire
> +	 * duration of kprobe processing
> +	 */
> +	preempt_disable();
> +	kcb = get_kprobe_ctlblk();
>  
>  	/* Handle recursion cases */
>  	if (kprobe_running()) {
> @@ -659,11 +666,6 @@ static int __kprobes pre_kprobes_handler
>  		goto no_kprobe;
>  	}
>  
> -	/*
> -	 * This preempt_disable() matches the preempt_enable_no_resched()
> -	 * in post_kprobes_handler()
> -	 */
> -	preempt_disable();
>  	set_current_kprobe(p, kcb);
>  	kcb->kprobe_status = KPROBE_HIT_ACTIVE;
>  
> @@ -681,6 +683,7 @@ ss_probe:
>  	return 1;
>  
>  no_kprobe:
> +	preempt_enable_no_resched();
>  	return ret;
>  }
>  
> @@ -716,9 +719,6 @@ static int __kprobes kprobes_fault_handl
>  	struct kprobe *cur = kprobe_running();
>  	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
>  
> -	if (!cur)
> -		return 0;
> -
>  	if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
>  		return 1;
>  
> @@ -737,7 +737,6 @@ int __kprobes kprobe_exceptions_notify(s
>  	struct die_args *args = (struct die_args *)data;
>  	int ret = NOTIFY_DONE;
>  
> -	rcu_read_lock();
>  	switch(val) {
>  	case DIE_BREAK:
>  		if (pre_kprobes_handler(args))
> @@ -748,12 +747,15 @@ int __kprobes kprobe_exceptions_notify(s
>  			ret = NOTIFY_STOP;
>  		break;
>  	case DIE_PAGE_FAULT:
> -		if (kprobes_fault_handler(args->regs, args->trapnr))
> +		/* kprobe_running() needs smp_processor_id() */
> +		preempt_disable();
> +		if (kprobe_running() &&
> +			kprobes_fault_handler(args->regs, args->trapnr))
>  			ret = NOTIFY_STOP;
> +		preempt_enable();
>  	default:
>  		break;
>  	}
> -	rcu_read_unlock();
>  	return ret;
>  }
>  
> @@ -785,6 +787,7 @@ int __kprobes longjmp_break_handler(stru
>  	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
>  
>  	*regs = kcb->jprobe_saved_regs;
> +	preempt_enable_no_resched();
>  	return 1;
>  }
>  
> Index: linux-2.6.14-rc4/arch/ppc64/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.14-rc4.orig/arch/ppc64/kernel/kprobes.c	2005-10-17 18:59:52.000000000 -0400
> +++ linux-2.6.14-rc4/arch/ppc64/kernel/kprobes.c	2005-10-19 11:00:02.000000000 -0400
> @@ -148,7 +148,14 @@ static inline int kprobe_handler(struct 
>  	struct kprobe *p;
>  	int ret = 0;
>  	unsigned int *addr = (unsigned int *)regs->nip;
> -	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> +	struct kprobe_ctlblk *kcb;
> +
> +	/*
> +	 * We don't want to be preempted for the entire
> +	 * duration of kprobe processing
> +	 */
> +	preempt_disable();
> +	kcb = get_kprobe_ctlblk();
>  
>  	/* Check we're not actually recursing */
>  	if (kprobe_running()) {
> @@ -207,11 +214,6 @@ static inline int kprobe_handler(struct 
>  		goto no_kprobe;
>  	}
>  
> -	/*
> -	 * This preempt_disable() matches the preempt_enable_no_resched()
> -	 * in post_kprobe_handler().
> -	 */
> -	preempt_disable();
>  	kcb->kprobe_status = KPROBE_HIT_ACTIVE;
>  	set_current_kprobe(p, regs, kcb);
>  	if (p->pre_handler && p->pre_handler(p, regs))
> @@ -224,6 +226,7 @@ ss_probe:
>  	return 1;
>  
>  no_kprobe:
> +	preempt_enable_no_resched();
>  	return ret;
>  }
>  
> @@ -296,8 +299,8 @@ int __kprobes trampoline_probe_handler(s
>  
>          /*
>           * By returning a non-zero value, we are telling
> -         * kprobe_handler() that we have handled unlocking
> -         * and re-enabling preemption.
> +         * kprobe_handler() that we don't want the post_handler
> +         * to run (and have re-enabled preemption)
>           */
>          return 1;
>  }
> @@ -385,7 +388,6 @@ int __kprobes kprobe_exceptions_notify(s
>  	struct die_args *args = (struct die_args *)data;
>  	int ret = NOTIFY_DONE;
>  
> -	rcu_read_lock();
>  	switch (val) {
>  	case DIE_BPT:
>  		if (kprobe_handler(args->regs))
> @@ -397,14 +399,16 @@ int __kprobes kprobe_exceptions_notify(s
>  		break;
>  	case DIE_GPF:
>  	case DIE_PAGE_FAULT:
> +		/* kprobe_running() needs smp_processor_id() */
> +		preempt_disable();
>  		if (kprobe_running() &&
>  		    kprobe_fault_handler(args->regs, args->trapnr))
>  			ret = NOTIFY_STOP;
> +		preempt_enable();
>  		break;
>  	default:
>  		break;
>  	}
> -	rcu_read_unlock();
>  	return ret;
>  }
>  
> @@ -441,6 +445,7 @@ int __kprobes longjmp_break_handler(stru
>  	 * saved regs...
>  	 */
>  	memcpy(regs, &kcb->jprobe_saved_regs, sizeof(struct pt_regs));
> +	preempt_enable_no_resched();
>  	return 1;
>  }
>  
> Index: linux-2.6.14-rc4/arch/sparc64/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.14-rc4.orig/arch/sparc64/kernel/kprobes.c	2005-10-17 18:59:53.000000000 -0400
> +++ linux-2.6.14-rc4/arch/sparc64/kernel/kprobes.c	2005-10-19 11:00:21.000000000 -0400
> @@ -113,7 +113,14 @@ static int __kprobes kprobe_handler(stru
>  	struct kprobe *p;
>  	void *addr = (void *) regs->tpc;
>  	int ret = 0;
> -	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> +	struct kprobe_ctlblk *kcb;
> +
> +	/*
> +	 * We don't want to be preempted for the entire
> +	 * duration of kprobe processing
> +	 */
> +	preempt_disable();
> +	kcb = get_kprobe_ctlblk();
>  
>  	if (kprobe_running()) {
>  		p = get_kprobe(addr);
> @@ -159,11 +166,6 @@ static int __kprobes kprobe_handler(stru
>  		goto no_kprobe;
>  	}
>  
> -	/*
> -	 * This preempt_disable() matches the preempt_enable_no_resched()
> -	 * in post_kprobes_handler()
> -	 */
> -	preempt_disable();
>  	set_current_kprobe(p, regs, kcb);
>  	kcb->kprobe_status = KPROBE_HIT_ACTIVE;
>  	if (p->pre_handler && p->pre_handler(p, regs))
> @@ -175,6 +177,7 @@ ss_probe:
>  	return 1;
>  
>  no_kprobe:
> +	preempt_enable_no_resched();
>  	return ret;
>  }
>  
> @@ -321,7 +324,6 @@ int __kprobes kprobe_exceptions_notify(s
>  	struct die_args *args = (struct die_args *)data;
>  	int ret = NOTIFY_DONE;
>  
> -	rcu_read_lock();
>  	switch (val) {
>  	case DIE_DEBUG:
>  		if (kprobe_handler(args->regs))
> @@ -333,14 +335,16 @@ int __kprobes kprobe_exceptions_notify(s
>  		break;
>  	case DIE_GPF:
>  	case DIE_PAGE_FAULT:
> +		/* kprobe_running() needs smp_processor_id() */
> +		preempt_disable();
>  		if (kprobe_running() &&
>  		    kprobe_fault_handler(args->regs, args->trapnr))
>  			ret = NOTIFY_STOP;
> +		preempt_enable();
>  		break;
>  	default:
>  		break;
>  	}
> -	rcu_read_unlock();
>  	return ret;
>  }
>  
> @@ -426,6 +430,7 @@ int __kprobes longjmp_break_handler(stru
>  		       &(kcb->jprobe_saved_stack),
>  		       sizeof(kcb->jprobe_saved_stack));
>  
> +		preempt_enable_no_resched();
>  		return 1;
>  	}
>  	return 0;
> Index: linux-2.6.14-rc4/arch/x86_64/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.14-rc4.orig/arch/x86_64/kernel/kprobes.c	2005-10-17 18:59:53.000000000 -0400
> +++ linux-2.6.14-rc4/arch/x86_64/kernel/kprobes.c	2005-10-19 11:01:31.000000000 -0400
> @@ -286,16 +286,19 @@ void __kprobes arch_prepare_kretprobe(st
>          }
>  }
>  
> -/*
> - * Interrupts are disabled on entry as trap3 is an interrupt gate and they
> - * remain disabled thorough out this function.
> - */
>  int __kprobes kprobe_handler(struct pt_regs *regs)
>  {
>  	struct kprobe *p;
>  	int ret = 0;
>  	kprobe_opcode_t *addr = (kprobe_opcode_t *)(regs->rip - sizeof(kprobe_opcode_t));
> -	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> +	struct kprobe_ctlblk *kcb;
> +
> +	/*
> +	 * We don't want to be preempted for the entire
> +	 * duration of kprobe processing
> +	 */
> +	preempt_disable();
> +	kcb = get_kprobe_ctlblk();
>  
>  	/* Check we're not actually recursing */
>  	if (kprobe_running()) {
> @@ -359,11 +362,6 @@ int __kprobes kprobe_handler(struct pt_r
>  		goto no_kprobe;
>  	}
>  
> -	/*
> -	 * This preempt_disable() matches the preempt_enable_no_resched()
> -	 * in post_kprobe_handler()
> -	 */
> -	preempt_disable();
>  	set_current_kprobe(p, regs, kcb);
>  	kcb->kprobe_status = KPROBE_HIT_ACTIVE;
>  
> @@ -377,6 +375,7 @@ ss_probe:
>  	return 1;
>  
>  no_kprobe:
> +	preempt_enable_no_resched();
>  	return ret;
>  }
>  
> @@ -448,8 +447,8 @@ int __kprobes trampoline_probe_handler(s
>  
>          /*
>           * By returning a non-zero value, we are telling
> -         * kprobe_handler() that we have handled unlocking
> -	 * and re-enabling preemption
> +         * kprobe_handler() that we don't want the post_handler
> +	 * to run (and have re-enabled preemption)
>           */
>          return 1;
>  }
> @@ -594,7 +593,6 @@ int __kprobes kprobe_exceptions_notify(s
>  	struct die_args *args = (struct die_args *)data;
>  	int ret = NOTIFY_DONE;
>  
> -	rcu_read_lock();
>  	switch (val) {
>  	case DIE_INT3:
>  		if (kprobe_handler(args->regs))
> @@ -606,14 +604,16 @@ int __kprobes kprobe_exceptions_notify(s
>  		break;
>  	case DIE_GPF:
>  	case DIE_PAGE_FAULT:
> +		/* kprobe_running() needs smp_processor_id() */
> +		preempt_disable();
>  		if (kprobe_running() &&
>  		    kprobe_fault_handler(args->regs, args->trapnr))
>  			ret = NOTIFY_STOP;
> +		preempt_enable();
>  		break;
>  	default:
>  		break;
>  	}
> -	rcu_read_unlock();
>  	return ret;
>  }
>  
> @@ -675,6 +675,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();
>  		return 1;
>  	}
>  	return 0;
> 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux