This is the mail archive of the systemtap@sources.redhat.com mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC] [PATCH] Multiple kprobes at an address redux (take4)


OK, how about taking the following route instead from the interface
standpoint:

Define a register_dynamic_trace_handler interface that implements 
the semantics for multiple probe handlers, which you can evolve and
optimize over time as all the systemtap requirements in terms of
multiple handler users get better crystallized and incorporated.
Generally, it is better to have some actual users in place before
finalising an interface. The return probes discussion is indicative
that things aren't completely settled yet. It'll also enable more
room for experimentation with alternative approaches as seems apt.

Meanwhile, keep register_kprobe *as it is* in the mainline for
the time being (no need for register_single_kprobe etc)

That will also keep me out of the way for the time being :), so you
can focus on getting things done rather than going through so many
iterations.

>From what I have understood so far, systemtap doesn't really have
a hard requirement to intercept/addon to already existing kprobes.
Most of the issues are around having the flexibility not to have
to worry about existing probes from other systemtap sessions or
other similar kinds of facilities that may be developed over time.
So overriding existing register_kprobe doesn't appear to be a
neccessity.

At a later point of time, once the code has settled down a bit and
the usage model has been successfully tried out in practice -- 
we might want to take a look at opportunities for optimization / 
code reduction / broadening of scope by making changes to the
kprobes low-level parts, or even making it the default interface
for kprobes if that seems like the right thing to do.

But I'm just not comfortable about doing that now - it is too
early.

Does that sound reasonable ?

Regards
Suparna

On Wed, Apr 13, 2005 at 10:32:08AM -0400, Ananth N Mavinakayanahalli wrote:
> Hi,
> 
> Here goes take4.
> 
> The only change between take3 and this patch is the exporting of the
> (un)register_kprobe_single() interfaces, for low-level users. Jprobes
> (un)registration now use these interfaces.
> 
> 
> Regards,
> Ananth

> 
>  include/linux/kprobes.h |    3 
>  kernel/kprobes.c        |  166 ++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 165 insertions(+), 4 deletions(-)
> 
> diff -Naurp temp/linux-2.6.12-rc2/include/linux/kprobes.h linux-2.6.12-rc2/include/linux/kprobes.h
> --- temp/linux-2.6.12-rc2/include/linux/kprobes.h	2005-04-13 10:17:04.000000000 -0400
> +++ linux-2.6.12-rc2/include/linux/kprobes.h	2005-04-13 09:26:32.000000000 -0400
> @@ -43,6 +43,9 @@ typedef int (*kprobe_fault_handler_t) (s
>  struct kprobe {
>  	struct hlist_node hlist;
>  
> +	/* list of kprobes for multi-handler support */
> +	struct list_head list;
> +
>  	/* location of the probe point */
>  	kprobe_opcode_t *addr;
>  
> diff -Naurp temp/linux-2.6.12-rc2/kernel/kprobes.c linux-2.6.12-rc2/kernel/kprobes.c
> --- temp/linux-2.6.12-rc2/kernel/kprobes.c	2005-04-13 10:17:04.000000000 -0400
> +++ linux-2.6.12-rc2/kernel/kprobes.c	2005-04-13 09:29:55.000000000 -0400
> @@ -44,6 +44,7 @@ static struct hlist_head kprobe_table[KP
>  
>  unsigned int kprobe_cpu = NR_CPUS;
>  static DEFINE_SPINLOCK(kprobe_lock);
> +static struct kprobe *curr_kprobe;
>  
>  /* Locks kprobe: irqs must be disabled */
>  void lock_kprobes(void)
> @@ -73,7 +74,49 @@ struct kprobe *get_kprobe(void *addr)
>  	return NULL;
>  }
>  
> -int register_kprobe(struct kprobe *p)
> +int aggr_pre_handler(struct kprobe *p, struct pt_regs *regs)
> +{
> +	struct kprobe *kp;
> +
> +	list_for_each_entry(kp, &p->list, list) {
> +		if (kp->pre_handler) {
> +			curr_kprobe = kp;
> +			kp->pre_handler(kp, regs);
> +			curr_kprobe = NULL;
> +		}
> +	}
> +	return 0;
> +}
> +
> +void aggr_post_handler(struct kprobe *p, struct pt_regs *regs, 
> +		unsigned long flags)
> +{
> +	struct kprobe *kp;
> +
> +	list_for_each_entry(kp, &p->list, list) {
> +		if (kp->post_handler) {
> +			curr_kprobe = kp;
> +			kp->post_handler(kp, regs, flags);
> +			curr_kprobe = NULL;
> +		}
> +	}
> +	return;
> +}
> +
> +int aggr_fault_handler(struct kprobe *p, struct pt_regs *regs, int trapnr)
> +{
> +	/* 
> +	 * if we faulted "during" the execution of a user specified
> +	 * probe handler, invoke just that probe's fault handler
> +	 */ 
> +	if (curr_kprobe && curr_kprobe->fault_handler) {
> +		if (curr_kprobe->fault_handler(curr_kprobe, regs, trapnr))
> +			return 1;
> +	}
> +	return 0;
> +}
> +
> +static int register_kprobe_single(struct kprobe *p)
>  {
>  	int ret = 0;
>  	unsigned long flags = 0;
> @@ -104,7 +147,7 @@ rm_kprobe:
>  	return ret;
>  }
>  
> -void unregister_kprobe(struct kprobe *p)
> +static void unregister_kprobe_single(struct kprobe *p)
>  {
>  	unsigned long flags;
>  	arch_remove_kprobe(p);
> @@ -116,6 +159,119 @@ void unregister_kprobe(struct kprobe *p)
>  	spin_unlock_irqrestore(&kprobe_lock, flags);
>  }
>  
> +static inline void add_aggr_kprobe(struct kprobe *ap, struct kprobe *p)
> +{
> +	ap->addr = p->addr;
> +	ap->opcode = p->opcode;
> +	memcpy(&ap->ainsn, &p->ainsn, 
> +			sizeof(struct arch_specific_insn));
> +
> +	ap->pre_handler = aggr_pre_handler;
> +	ap->post_handler = aggr_post_handler;
> +	ap->fault_handler = aggr_fault_handler;
> +
> +	INIT_LIST_HEAD(&ap->list);
> +	list_add(&p->list, &ap->list);
> +
> +	INIT_HLIST_NODE(&ap->hlist);
> +	hlist_del(&p->hlist);
> +	hlist_add_head(&ap->hlist,
> +		&kprobe_table[hash_ptr(ap->addr,
> +				KPROBE_HASH_BITS)]);
> +}
> +
> +static int register_aggr_kprobe(struct kprobe *p)
> +{
> +	int ret = 0;
> +	unsigned long flags = 0;
> +	struct kprobe *old_p, *ap;
> +
> +	ap = kcalloc(1, sizeof(struct kprobe), GFP_KERNEL);
> +	if (!ap) 
> +		return -ENOMEM;
> +
> +	spin_lock_irqsave(&kprobe_lock, flags);
> +	old_p = get_kprobe(p->addr);
> +	if (old_p) {
> +		if (old_p->break_handler || p->break_handler) {
> +			ret = -EEXIST;
> +			goto free_ap;
> +		} else if (old_p->pre_handler == aggr_pre_handler) {
> +			list_add(&p->list, &old_p->list);
> +			goto free_ap;
> +		} else {
> +			add_aggr_kprobe(ap, old_p);
> +			list_add(&p->list, &ap->list);
> +			goto out;
> +		}
> +	} else {
> +		/* 
> +		 * kprobe at this addr was deleted between the check
> +		 * in register_kprobe() and the get_kprobe() call above
> +		 */
> +		spin_unlock_irqrestore(&kprobe_lock, flags);
> +		ret = register_kprobe_single(p);
> +		if (ret == -EEXIST) {
> +			spin_lock_irqsave(&kprobe_lock, flags);
> +			old_p = get_kprobe(p->addr);
> +			if (old_p) {
> +				add_aggr_kprobe(ap, old_p);
> +				list_add(&p->list, &ap->list);
> +			}
> +			goto out;
> +		} else
> +			kfree(ap);
> +		return ret;
> +	}
> +free_ap:
> +	kfree(ap);
> +out:
> +	spin_unlock_irqrestore(&kprobe_lock, flags);
> +	return ret;
> +}
> +
> +static void unregister_aggr_kprobe(struct kprobe *old_p, 
> +		struct kprobe *p, unsigned long flags)
> +{
> +	/* we have taken the spinlock in unregister_kprobe() */
> +	list_del(&p->list);
> +
> +	if (list_empty(&old_p->list)) {
> +		spin_unlock_irqrestore(&kprobe_lock, flags);
> +		unregister_kprobe_single(old_p);
> +		kfree(old_p);
> +		return;
> +	} 
> +	spin_unlock_irqrestore(&kprobe_lock, flags);
> +}
> +
> +int register_kprobe(struct kprobe *p)
> +{
> +	int ret = 0;
> +
> +	ret = register_kprobe_single(p);
> +	if (ret == -EEXIST) 
> +		ret = register_aggr_kprobe(p);
> +
> +	return ret;
> +}
> +
> +void unregister_kprobe(struct kprobe *p)
> +{
> +	unsigned long flags;
> +	struct kprobe *old_p;
> +
> +	spin_lock_irqsave(&kprobe_lock, flags);
> +	old_p = get_kprobe(p->addr);
> +
> +	if (old_p && (old_p->pre_handler == aggr_pre_handler))
> +		unregister_aggr_kprobe(old_p, p, flags);
> +	else if (old_p == p) {
> +		spin_unlock_irqrestore(&kprobe_lock, flags);
> +		unregister_kprobe_single(p);
> +	}
> +}
> +
>  static struct notifier_block kprobe_exceptions_nb = {
>  	.notifier_call = kprobe_exceptions_notify,
>  	.priority = 0x7fffffff /* we need to notified first */
> @@ -127,12 +283,12 @@ int register_jprobe(struct jprobe *jp)
>  	jp->kp.pre_handler = setjmp_pre_handler;
>  	jp->kp.break_handler = longjmp_break_handler;
>  
> -	return register_kprobe(&jp->kp);
> +	return register_kprobe_single(&jp->kp);
>  }
>  
>  void unregister_jprobe(struct jprobe *jp)
>  {
> -	unregister_kprobe(&jp->kp);
> +	unregister_kprobe_single(&jp->kp);
>  }
>  
>  static int __init init_kprobes(void)
> @@ -152,6 +308,8 @@ __initcall(init_kprobes);
>  
>  EXPORT_SYMBOL_GPL(register_kprobe);
>  EXPORT_SYMBOL_GPL(unregister_kprobe);
> +EXPORT_SYMBOL_GPL(register_kprobe_single);
> +EXPORT_SYMBOL_GPL(unregister_kprobe_single);
>  EXPORT_SYMBOL_GPL(register_jprobe);
>  EXPORT_SYMBOL_GPL(unregister_jprobe);
>  EXPORT_SYMBOL_GPL(jprobe_return);


-- 
Suparna Bhattacharya (suparna@in.ibm.com)
Linux Technology Center
IBM Software Lab, India


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]