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

[Masami Hiramatsu <mhiramat@redhat.com>]

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


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com