Re: [PATCH][RFC] kprobes: Add user entry-handler in kretprobes

["Abhishek Sagar" <sagar.abhishek@gmail.com>]

On Nov 17, 2007 4:39 AM, Jim Keniston <jkenisto@us.ibm.com> wrote:
> First of all, as promised, here's what would be different if it were
> implemented using the data-pouch approach:
>
> --- abhishek1.c 2007-11-16 13:57:13.000000000 -0800
> +++ jim1.c      2007-11-16 14:20:39.000000000 -0800
> @@ -50,15 +50,12 @@
>         if (stats)
>                 return 1; /* recursive/nested call */
>
> -       stats = kmalloc(sizeof(struct prof_data), GFP_ATOMIC);
> -       if (!stats)
> -               return 1;
> +       stats = (struct prof_data *) ri->entry_info;
>
>         stats->entry_stamp = sched_clock();
>         stats->task = current;
>         INIT_LIST_HEAD(&stats->list);
>         list_add(&stats->list, &data_nodes);
> -       ri->data = stats;
>         return 0;
>  }
>
> @@ -66,10 +63,9 @@
>  static int return_handler(struct kretprobe_instance *ri, struct pt_regs
> *regs)
>  {
>         unsigned long flags;
> -       struct prof_data *stats = (struct prof_data *)ri->data;
> +       struct prof_data *stats = (struct prof_data *)ri->entry_info;
>         u64 elapsed;
>
> -       BUG_ON(ri->data == NULL);
>         elapsed = (long long)sched_clock() - (long long)stats->entry_stamp;
>
>         /* update stats */
> @@ -79,13 +75,13 @@
>         spin_unlock_irqrestore(&time_lock, flags);
>
>         list_del(&stats->list);
> -       kfree(stats);
>         return 0;
>  }
>
>  static struct kretprobe my_kretprobe = {
>         .handler = return_handler,
>         .entry_handler = entry_handler,
> +       .entry_info_sz = sizeof(struct prof_data)
>  };
>
>  /* called after every PRINT_DELAY seconds */
>
> So the data-pouch approach saves you a little code and a kmalloc/kfree
> round trip on each kretprobe hit.  A kmalloc/kfree round trip is about
> 80 ns on my system, or about 20% of the base cost of a kretprobe hit.  I
> don't know how important that is to people.
>
> I also note that this particular example maintains a per-task list of
> prof_data objects to avoid overcounting the time spent in a recursive
> function.  That adds about 30% to the size of your module source (136
> lines vs. 106, by my count).  I suspect that many instrumentation
> modules wouldn't need such a list.  However, without your ri->data
> pointer (or Kevin's ri->entry_info pouch), every instrumentation module
> that uses your enhancement would need such a list in order to map the ri
> to the per-instance.

Those are interesting numbers. Will incorporate pouching in the next
patch. Even with a data pointer or pouch, the mapping of ri (or
ri->data) would sometimes be necessary. It's required to catch
recursive/nested invocation cases. In case of time measurment test
module, these invocations needed to be weeded out and therefore such a
list was required. Other scenarios might not care for it. E.g a module
which measures the change in some global system state across every
call.

Thanks for the comments.

> Jim

- Abhishek
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/