Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756637AbZF0UJn (ORCPT ); Sat, 27 Jun 2009 16:09:43 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751324AbZF0UJe (ORCPT ); Sat, 27 Jun 2009 16:09:34 -0400 Received: from hrndva-omtalb.mail.rr.com ([71.74.56.125]:40341 "EHLO hrndva-omtalb.mail.rr.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751257AbZF0UJd (ORCPT ); Sat, 27 Jun 2009 16:09:33 -0400 Date: Sat, 27 Jun 2009 16:09:33 -0400 (EDT) From: Steven Rostedt X-X-Sender: rostedt@gandalf.stny.rr.com To: Masami Hiramatsu cc: Ingo Molnar , Ananth N Mavinakayanahalli , lkml , systemtap , DLE , Jim Keniston , Srikar Dronamraju , Christoph Hellwig , Frederic Weisbecker , "H. Peter Anvin" , Anders Kaseorg , Tim Abbott Subject: Re: [RFC][ PATCH -tip v2 2/7] kprobes: introducing generic insn_slot framework In-Reply-To: <20090622212307.5384.41192.stgit@localhost.localdomain> Message-ID: References: <20090622212255.5384.53732.stgit@localhost.localdomain> <20090622212307.5384.41192.stgit@localhost.localdomain> User-Agent: Alpine 2.00 (DEB 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8469 Lines: 269 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 > Cc: Ananth N Mavinakayanahalli > Cc: Ingo Molnar > Cc: Jim Keniston > Cc: Srikar Dronamraju > Cc: Christoph Hellwig > Cc: Steven Rostedt > Cc: Frederic Weisbecker > Cc: H. Peter Anvin > Cc: Anders Kaseorg > Cc: Tim Abbott > --- > > 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. > +}; > + > +struct kprobe_insn_cache { > + struct list_head pages; /* list of kprobe_insn_page */ > + size_t insn_size; /* size of instruction slot */ > + int nr_garbage; > }; > > +static int slots_per_page(struct kprobe_insn_cache *c) > +{ > + return PAGE_SIZE/(c->insn_size * sizeof(kprobe_opcode_t)); > +} > + > enum kprobe_slot_state { > SLOT_CLEAN = 0, > SLOT_DIRTY = 1, > SLOT_USED = 2, > }; > > -static DEFINE_MUTEX(kprobe_insn_mutex); /* Protects kprobe_insn_pages */ > -static LIST_HEAD(kprobe_insn_pages); > -static int kprobe_garbage_slots; > -static int collect_garbage_slots(void); > +static DEFINE_MUTEX(kprobe_insn_mutex); /* Protects kprobe_insn_slots */ > +static struct kprobe_insn_cache kprobe_insn_slots = { > + .pages = LIST_HEAD_INIT(kprobe_insn_slots.pages), > + .insn_size = MAX_INSN_SIZE, > + .nr_garbage = 0, > +}; > +static int __kprobes collect_garbage_slots(struct kprobe_insn_cache *c); > > static int __kprobes check_safety(void) > { > @@ -149,32 +161,33 @@ loop_end: > * __get_insn_slot() - Find a slot on an executable page for an instruction. > * We allocate an executable page if there's no room on existing ones. > */ > -static kprobe_opcode_t __kprobes *__get_insn_slot(void) > +static kprobe_opcode_t __kprobes *__get_insn_slot(struct kprobe_insn_cache *c) > { > struct kprobe_insn_page *kip; > > retry: > - 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? > } > } > > /* 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)); -- Steve > + GFP_KERNEL); > if (!kip) > return NULL; > > @@ -189,19 +202,20 @@ static kprobe_opcode_t __kprobes *__get_insn_slot(void) > return NULL; > } > INIT_LIST_HEAD(&kip->list); > - list_add(&kip->list, &kprobe_insn_pages); > - memset(kip->slot_used, SLOT_CLEAN, INSNS_PER_PAGE); > + memset(kip->slot_used, SLOT_CLEAN, slots_per_page(c)); > kip->slot_used[0] = SLOT_USED; > kip->nused = 1; > kip->ngarbage = 0; > + list_add(&kip->list, &c->pages); > return kip->insns; > } > > + > kprobe_opcode_t __kprobes *get_insn_slot(void) > { > - kprobe_opcode_t *ret; > + kprobe_opcode_t *ret = NULL; > mutex_lock(&kprobe_insn_mutex); > - ret = __get_insn_slot(); > + ret = __get_insn_slot(&kprobe_insn_slots); > mutex_unlock(&kprobe_insn_mutex); > return ret; > } > @@ -218,7 +232,7 @@ static int __kprobes collect_one_slot(struct kprobe_insn_page *kip, int idx) > * so as not to have to set it up again the > * next time somebody inserts a probe. > */ > - if (!list_is_singular(&kprobe_insn_pages)) { > + if (!list_is_singular(&kip->list)) { > list_del(&kip->list); > module_free(NULL, kip->insns); > kfree(kip); > @@ -228,7 +242,7 @@ static int __kprobes collect_one_slot(struct kprobe_insn_page *kip, int idx) > return 0; > } > > -static int __kprobes collect_garbage_slots(void) > +static int __kprobes collect_garbage_slots(struct kprobe_insn_cache *c) > { > struct kprobe_insn_page *kip, *next; > int safety; > @@ -240,42 +254,48 @@ static int __kprobes collect_garbage_slots(void) > if (safety != 0) > return -EAGAIN; > > - list_for_each_entry_safe(kip, next, &kprobe_insn_pages, list) { > + list_for_each_entry_safe(kip, next, &c->pages, list) { > int i; > if (kip->ngarbage == 0) > continue; > kip->ngarbage = 0; /* we will collect all garbages */ > - for (i = 0; i < INSNS_PER_PAGE; i++) { > + for (i = 0; i < slots_per_page(c); i++) { > if (kip->slot_used[i] == SLOT_DIRTY && > collect_one_slot(kip, i)) > break; > } > } > - kprobe_garbage_slots = 0; > + c->nr_garbage = 0; > return 0; > } > > -void __kprobes free_insn_slot(kprobe_opcode_t * slot, int dirty) > +static void __kprobes __free_insn_slot(struct kprobe_insn_cache *c, > + kprobe_opcode_t *slot, int dirty) > { > struct kprobe_insn_page *kip; > > - mutex_lock(&kprobe_insn_mutex); > - list_for_each_entry(kip, &kprobe_insn_pages, list) { > - if (kip->insns <= slot && > - slot < kip->insns + (INSNS_PER_PAGE * MAX_INSN_SIZE)) { > - int i = (slot - kip->insns) / MAX_INSN_SIZE; > + list_for_each_entry(kip, &c->pages, list) { > + long idx = ((long)slot - (long)kip->insns) / c->insn_size; > + if (idx >= 0 && idx < slots_per_page(c)) { > + WARN_ON(kip->slot_used[idx] != SLOT_USED); > if (dirty) { > - kip->slot_used[i] = SLOT_DIRTY; > + kip->slot_used[idx] = SLOT_DIRTY; > kip->ngarbage++; > + if (++c->nr_garbage > slots_per_page(c)) > + collect_garbage_slots(c); > } else > - collect_one_slot(kip, i); > - break; > + collect_one_slot(kip, idx); > + return; > } > } > + /* Could not free this slot. */ > + WARN_ON(1); > +} > > - if (dirty && ++kprobe_garbage_slots > INSNS_PER_PAGE) > - collect_garbage_slots(); > - > +void __kprobes free_insn_slot(kprobe_opcode_t * slot, int dirty) > +{ > + mutex_lock(&kprobe_insn_mutex); > + __free_insn_slot(&kprobe_insn_slots, slot, dirty); > mutex_unlock(&kprobe_insn_mutex); > } > #endif > > > -- > Masami Hiramatsu > > Software Engineer > Hitachi Computer Products (America), Inc. > Software Solutions Division > > e-mail: mhiramat@redhat.com > -- 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/