Re: [RFC][ PATCH -tip v2 2/7] kprobes: introducing generic insn_slot framework

Masami Hiramatsu
2009-06-29T21:07:33Z

Thank you for reviewing.

Steven Rostedt wrote:
> Hi Masami,
>
> I'm currently traveling so my responses are very slow this week.
>
>
> On Mon, 22 Jun 2009, Masami Hiramatsu wrote:
>
>> Make insn_slot framework support various size slots.
>> Current insn_slot just supports one-size instruction buffer slot. However,
>> kprobes jump optimization needs larger size buffers.
>>
>> Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
>> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
>> Cc: Ingo Molnar <mingo@elte.hu>
>> Cc: Jim Keniston <jkenisto@us.ibm.com>
>> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
>> Cc: Christoph Hellwig <hch@infradead.org>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Cc: H. Peter Anvin <hpa@zytor.com>
>> Cc: Anders Kaseorg <andersk@ksplice.com>
>> Cc: Tim Abbott <tabbott@ksplice.com>
>> ---
>>
>> kernel/kprobes.c | 96 +++++++++++++++++++++++++++++++++---------------------
>> 1 files changed, 58 insertions(+), 38 deletions(-)
>>
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index 0b68fdc..bc9cfd0 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
>> @@ -100,26 +100,38 @@ static struct kprobe_blackpoint kprobe_blacklist[] = {
>> * stepping on the instruction on a vmalloced/kmalloced/data page
>> * is a recipe for disaster
>> */
>> -#define INSNS_PER_PAGE (PAGE_SIZE/(MAX_INSN_SIZE * sizeof(kprobe_opcode_t)))
>> -
>> struct kprobe_insn_page {
>> struct list_head list;
>> kprobe_opcode_t *insns; /* Page of instruction slots */
>> - char slot_used[INSNS_PER_PAGE];
>> int nused;
>> int ngarbage;
>> + char slot_used[1];
>
> I would recommend using [] instead of [1], that would help other
> developers know that it is a variable array.

Sure.

[...]
>> - list_for_each_entry(kip, &kprobe_insn_pages, list) {
>> - if (kip->nused < INSNS_PER_PAGE) {
>> + list_for_each_entry(kip, &c->pages, list) {
>> + if (kip->nused < slots_per_page(c)) {
>> int i;
>> - for (i = 0; i < INSNS_PER_PAGE; i++) {
>> + for (i = 0; i < slots_per_page(c); i++) {
>> if (kip->slot_used[i] == SLOT_CLEAN) {
>> kip->slot_used[i] = SLOT_USED;
>> kip->nused++;
>> - return kip->insns + (i * MAX_INSN_SIZE);
>> + return kip->insns + (i * c->insn_size);
>> }
>> }
>> - /* Surprise! No unused slots. Fix kip->nused. */
>> - kip->nused = INSNS_PER_PAGE;
>> + /* kip->nused is broken. */
>> + BUG();
>
> Does this deserve a bug, or can we get away with a WARN and find a way to
> fail nicely? Is it already too late to recover?

No, WARN() is enough here.

>
>> }
>> }
>>
>> /* If there are any garbage slots, collect it and try again. */
>> - if (kprobe_garbage_slots && collect_garbage_slots() == 0) {
>> + if (c->nr_garbage && collect_garbage_slots(c) == 0)
>> goto retry;
>> - }
>> +
>> /* All out of space. Need to allocate a new page. Use slot 0. */
>> - kip = kmalloc(sizeof(struct kprobe_insn_page), GFP_KERNEL);
>> + kip = kmalloc(sizeof(struct kprobe_insn_page) + slots_per_page(c) - 1,
>
> Why the '- 1'? Is it because of the char [1] above?
>
> Would be better to make the size of the kprobe_insn_page a macro:
>
> #define KPROBE_INSN_SIZE offsetof(struct kbrobe_insn_page, slot_used)
>
> and then you can do the following:
>
> kip = kmalloc(KPROBE_INSN_SIZE + slots_per_page(c));

Good idea!

Thanks