This is the mail archive of the
systemtap@sources.redhat.com
mailing list for the systemtap project.
Re: [RFC] Design + prototype: Multiple kprobes at an address -take 2
- From: Jim Keniston <jkenisto at us dot ibm dot com>
- To: Ananth N Mavinakayanahalli <amavin at redhat dot com>
- Cc: SystemTAP <systemtap at sources dot redhat dot com>
- Date: 06 Apr 2005 16:08:41 -0700
- Subject: Re: [RFC] Design + prototype: Multiple kprobes at an address -take 2
- Organization:
- References: <4253F6B5.5050000@redhat.com>
On Wed, 2005-04-06 at 07:48, Ananth N Mavinakayanahalli wrote:
I missed take 1. Sorry if these comments have already been addressed,
but here goes...
...
> +
> +static void copy_kprobe(struct kprobe *src, struct kprobe *dst)
> +{
> + dst->opcode = src->opcode;
> + memcpy(&dst->ainsn, &src->ainsn, sizeof(struct arch_specific_insn));
> +}
x86_64 stores the copy of the instruction on a different (executable)
page. (See arch_prepare_kprobe().) It's not clear to me that you take
this into account.
> +
> +struct aggr_probe *register_aggr_probe(struct kprobe *old_p)
> +{
> + struct aggr_probe *ap;
> +
> + ap = kcalloc(1, sizeof(struct aggr_probe), GFP_ATOMIC);
I understand why you have to use GFP_ATOMIC here (lock held), but
consider that there may be many probepoints where there are multiple
kprobes. (E.g., Hien is thinking of re-implementing retprobes using
your multiple-kprobes feature, to allow register/unregister of a
retprobe to be independent of the entry probe. Think about those two
probes at the entry for every system call.) Anyway, it's probably best
to avoid eating up GFP_ATOMIC memory.
Instead, why not pre-allocate this memory early in register_kprobe(),
before you acquire the lock? If you don't need it (the usual case), you
can just free it.
...
> int register_kprobe(struct kprobe *p)
> {
> int ret = 0;
> unsigned long flags = 0;
> + struct aggr_probe *ap;
> + struct kprobe *old_p;
>
> if ((ret = arch_prepare_kprobe(p)) != 0) {
> goto rm_kprobe;
> }
> +
> spin_lock_irqsave(&kprobe_lock, flags);
> INIT_HLIST_NODE(&p->hlist);
> - if (get_kprobe(p->addr)) {
> - ret = -EEXIST;
> + old_p = get_kprobe(p->addr);
> + if (old_p) {
> + if (old_p->break_handler) {
Seems like you could change this to
if (old_p->break_handler || p->break_handler)
and remove the two checks below.
> + /* jprobe present, can't register another probe */
> + ret = -EEXIST;
> + goto out;
> + }
> + if (old_p->pre_handler == aggr_pre_handler) {
> + /*
> + * Check if registration is for a jprobe
> + * Fail if so till we figure out how a jprobe
> + * and a kprobe can coexist at the same addr
> + */
> + if (p->break_handler) {
> + ret = -EEXIST;
> + goto out;
> + }
> + /* addr is already initialized */
> + copy_kprobe(old_p, p);
> + ap = container_of(old_p, struct aggr_probe, kp);
> + list_add(&p->list, &ap->kprobes);
> + } else {
> + /* check for same reason as above */
> + if (p->break_handler) {
> + ret = -EEXIST;
> + goto out;
> + }
...
>
> void unregister_kprobe(struct kprobe *p)
> {
> unsigned long flags;
> - arch_remove_kprobe(p);
> + struct kprobe *kp;
> + struct aggr_probe *ap;
> +
> spin_lock_irqsave(&kprobe_lock, flags);
> + kp = get_kprobe(p->addr);
> + if (kp && (kp->pre_handler == aggr_pre_handler)) {
> +
> + /* this is one of the possibly many kprobes at the address */
> + ap = container_of(kp, struct aggr_probe, kp);
> + list_del(&p->list);
> +
> + /*
> + * if we just unregistered the last probe at the address,
> + * its time to cleanup
> + */
> + if (list_empty(&ap->kprobes)) {
> + arch_remove_kprobe(p);
Unfortunately, on x86_64, arch_remove_kprobe() can sleep, so you can't
call it holding a lock.
...
Jim