On Nov 17, 2007 4:39 AM, Jim Keniston <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
