Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757870Ab1CONkQ (ORCPT ); Tue, 15 Mar 2011 09:40:16 -0400 Received: from www.tglx.de ([62.245.132.106]:57160 "EHLO www.tglx.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757832Ab1CONkO (ORCPT ); Tue, 15 Mar 2011 09:40:14 -0400 Date: Tue, 15 Mar 2011 14:38:33 +0100 (CET) From: Thomas Gleixner To: Srikar Dronamraju cc: Peter Zijlstra , Ingo Molnar , Steven Rostedt , Linux-mm , Arnaldo Carvalho de Melo , Linus Torvalds , Masami Hiramatsu , Christoph Hellwig , Ananth N Mavinakayanahalli , Andi Kleen , Oleg Nesterov , Andrew Morton , Jim Keniston , Roland McGrath , SystemTap , LKML , "Paul E. McKenney" Subject: Re: [PATCH v2 2.6.38-rc8-tip 4/20] 4: uprobes: Adding and remove a uprobe in a rb tree. In-Reply-To: <20110314133444.27435.50684.sendpatchset@localhost6.localdomain6> Message-ID: References: <20110314133403.27435.7901.sendpatchset@localhost6.localdomain6> <20110314133444.27435.50684.sendpatchset@localhost6.localdomain6> User-Agent: Alpine 2.00 (LFD 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: 4340 Lines: 183 On Mon, 14 Mar 2011, Srikar Dronamraju wrote: > > +static int valid_vma(struct vm_area_struct *vma) bool perpaps ? > +{ > + if (!vma->vm_file) > + return 0; > + > + if ((vma->vm_flags & (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)) == > + (VM_READ|VM_EXEC)) Looks more correct than the code it replaces :) > + return 1; > + > + return 0; > +} > + > +static struct rb_root uprobes_tree = RB_ROOT; > +static DEFINE_MUTEX(uprobes_mutex); > +static DEFINE_SPINLOCK(treelock); Why do you need a mutex and a spinlock ? Also the mutex is not referenced. > +static int match_inode(struct uprobe *uprobe, struct inode *inode, > + struct rb_node **p) > +{ > + struct rb_node *n = *p; > + > + if (inode < uprobe->inode) > + *p = n->rb_left; > + else if (inode > uprobe->inode) > + *p = n->rb_right; > + else > + return 1; > + return 0; > +} > + > +static int match_offset(struct uprobe *uprobe, loff_t offset, > + struct rb_node **p) > +{ > + struct rb_node *n = *p; > + > + if (offset < uprobe->offset) > + *p = n->rb_left; > + else if (offset > uprobe->offset) > + *p = n->rb_right; > + else > + return 1; > + return 0; > +} > + > + > +/* Called with treelock held */ > +static struct uprobe *__find_uprobe(struct inode * inode, > + loff_t offset, struct rb_node **near_match) > +{ > + struct rb_node *n = uprobes_tree.rb_node; > + struct uprobe *uprobe, *u = NULL; > + > + while (n) { > + uprobe = rb_entry(n, struct uprobe, rb_node); > + if (match_inode(uprobe, inode, &n)) { > + if (near_match) > + *near_match = n; > + if (match_offset(uprobe, offset, &n)) { > + atomic_inc(&uprobe->ref); > + u = uprobe; > + break; > + } > + } > + } > + return u; > +} > + > +/* > + * Find a uprobe corresponding to a given inode:offset > + * Acquires treelock > + */ > +static struct uprobe *find_uprobe(struct inode * inode, loff_t offset) > +{ > + struct uprobe *uprobe; > + unsigned long flags; > + > + spin_lock_irqsave(&treelock, flags); > + uprobe = __find_uprobe(inode, offset, NULL); > + spin_unlock_irqrestore(&treelock, flags); What's the calling context ? Do we really need a spinlock here for walking the rb tree ? > + > +/* Should be called lock-less */ -ENOPARSE > +static void put_uprobe(struct uprobe *uprobe) > +{ > + if (atomic_dec_and_test(&uprobe->ref)) > + kfree(uprobe); > +} > + > +static struct uprobe *uprobes_add(struct inode *inode, loff_t offset) > +{ > + struct uprobe *uprobe, *cur_uprobe; > + > + __iget(inode); > + uprobe = kzalloc(sizeof(struct uprobe), GFP_KERNEL); > + > + if (!uprobe) { > + iput(inode); > + return NULL; > + } Please move the __iget() after the kzalloc() > + uprobe->inode = inode; > + uprobe->offset = offset; > + > + /* add to uprobes_tree, sorted on inode:offset */ > + cur_uprobe = insert_uprobe(uprobe); > + > + /* a uprobe exists for this inode:offset combination*/ > + if (cur_uprobe) { > + kfree(uprobe); > + uprobe = cur_uprobe; > + iput(inode); > + } else > + init_rwsem(&uprobe->consumer_rwsem); Please init stuff _before_ inserting not afterwards. > + > + return uprobe; > +} > + > +/* Acquires uprobe->consumer_rwsem */ > +static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs) > +{ > + struct uprobe_consumer *consumer; > + > + down_read(&uprobe->consumer_rwsem); > + consumer = uprobe->consumers; > + while (consumer) { > + if (!consumer->filter || consumer->filter(consumer, current)) > + consumer->handler(consumer, regs); > + > + consumer = consumer->next; > + } > + up_read(&uprobe->consumer_rwsem); > +} > + > +/* Acquires uprobe->consumer_rwsem */ > +static void add_consumer(struct uprobe *uprobe, > + struct uprobe_consumer *consumer) > +{ > + down_write(&uprobe->consumer_rwsem); > + consumer->next = uprobe->consumers; > + uprobe->consumers = consumer; > + up_write(&uprobe->consumer_rwsem); > + return; pointless return > +} > + > +/* Acquires uprobe->consumer_rwsem */ I'd prefer a comment about the return code over this redundant information. > +static int del_consumer(struct uprobe *uprobe, > + struct uprobe_consumer *consumer) > +{ -- 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/