lkml.org 
[lkml]   [2011]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v5 3.1.0-rc4-tip 4/26] uprobes: Define hooks for mmap/munmap.
    * Peter Zijlstra <peterz@infradead.org> [2011-09-27 13:37:15]:

    > On Mon, 2011-09-26 at 21:14 +0530, Srikar Dronamraju wrote:
    > >
    > > > > +
    > > > > +/*
    > > > > + * Called from mmap_region.
    > > > > + * called with mm->mmap_sem acquired.
    > > > > + *
    > > > > + * Return -ve no if we fail to insert probes and we cannot
    > > > > + * bail-out.
    > > > > + * Return 0 otherwise. i.e :
    > > > > + * - successful insertion of probes
    > > > > + * - (or) no possible probes to be inserted.
    > > > > + * - (or) insertion of probes failed but we can bail-out.
    > > > > + */
    > > > > +int mmap_uprobe(struct vm_area_struct *vma)
    > > > > +{
    > > > > + struct list_head tmp_list;
    > > > > + struct uprobe *uprobe, *u;
    > > > > + struct inode *inode;
    > > > > + int ret = 0;
    > > > > +
    > > > > + if (!valid_vma(vma))
    > > > > + return ret; /* Bail-out */
    > > > > +
    > > > > + inode = igrab(vma->vm_file->f_mapping->host);
    > > > > + if (!inode)
    > > > > + return ret;
    > > > > +
    > > > > + INIT_LIST_HEAD(&tmp_list);
    > > > > + mutex_lock(&uprobes_mmap_mutex);
    > > > > + build_probe_list(inode, &tmp_list);
    > > > > + list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
    > > > > + loff_t vaddr;
    > > > > +
    > > > > + list_del(&uprobe->pending_list);
    > > > > + if (!ret && uprobe->consumers) {
    > > > > + vaddr = vma->vm_start + uprobe->offset;
    > > > > + vaddr -= vma->vm_pgoff << PAGE_SHIFT;
    > > > > + if (vaddr < vma->vm_start || vaddr >= vma->vm_end)
    > > > > + continue;
    > > > > + ret = install_breakpoint(vma->vm_mm, uprobe);
    > > > > +
    > > > > + if (ret && (ret == -ESRCH || ret == -EEXIST))
    > > > > + ret = 0;
    > > > > + }
    > > > > + put_uprobe(uprobe);
    > > > > + }
    > > > > +
    > > > > + mutex_unlock(&uprobes_mmap_mutex);
    > > > > + iput(inode);
    > > > > + return ret;
    > > > > +}
    > > > > +
    > > > > +static void dec_mm_uprobes_count(struct vm_area_struct *vma,
    > > > > + struct inode *inode)
    > > > > +{
    > > > > + struct uprobe *uprobe;
    > > > > + struct rb_node *n;
    > > > > + unsigned long flags;
    > > > > +
    > > > > + n = uprobes_tree.rb_node;
    > > > > + spin_lock_irqsave(&uprobes_treelock, flags);
    > > > > + uprobe = __find_uprobe(inode, 0, &n);
    > > > > +
    > > > > + /*
    > > > > + * If indeed there is a probe for the inode and with offset zero,
    > > > > + * then lets release its reference. (ref got thro __find_uprobe)
    > > > > + */
    > > > > + if (uprobe)
    > > > > + put_uprobe(uprobe);
    > > > > + for (; n; n = rb_next(n)) {
    > > > > + loff_t vaddr;
    > > > > +
    > > > > + uprobe = rb_entry(n, struct uprobe, rb_node);
    > > > > + if (uprobe->inode != inode)
    > > > > + break;
    > > > > + vaddr = vma->vm_start + uprobe->offset;
    > > > > + vaddr -= vma->vm_pgoff << PAGE_SHIFT;
    > > > > + if (vaddr < vma->vm_start || vaddr >= vma->vm_end)
    > > > > + continue;
    > > > > + atomic_dec(&vma->vm_mm->mm_uprobes_count);
    > > > > + }
    > > > > + spin_unlock_irqrestore(&uprobes_treelock, flags);
    > > > > +}
    > > > > +
    > > > > +/*
    > > > > + * Called in context of a munmap of a vma.
    > > > > + */
    > > > > +void munmap_uprobe(struct vm_area_struct *vma)
    > > > > +{
    > > > > + struct inode *inode;
    > > > > +
    > > > > + if (!valid_vma(vma))
    > > > > + return; /* Bail-out */
    > > > > +
    > > > > + if (!atomic_read(&vma->vm_mm->mm_uprobes_count))
    > > > > + return;
    > > > > +
    > > > > + inode = igrab(vma->vm_file->f_mapping->host);
    > > > > + if (!inode)
    > > > > + return;
    > > > > +
    > > > > + dec_mm_uprobes_count(vma, inode);
    > > > > + iput(inode);
    > > > > + return;
    > > > > +}
    > > >
    > > > One has to wonder why mmap_uprobe() can be one function but
    > > > munmap_uprobe() cannot.
    > > >
    > >
    > > I didnt understand this comment, Can you please elaborate?
    > > mmap_uprobe uses build_probe_list and munmap_uprobe uses
    > > dec_mm_uprobes_count.
    >
    > Ah, I missed build_probe_list(), but I didn't see a reason for the
    > existence of dec_mm_uprobe_count(), the name doesn't make sense and the
    > content is 'small' enough to just put in munmap_uprobe.
    >
    > To me it looks similar to the list iteration you have in mmap_uprobe(),
    > you didn't split that out into another function either.

    Hmm, For me whats done in dec_mm_uprobe_count() is similar to whats
    done in build_probe_list(), just that build_probe_list does a atomic_inc
    + list_add while dec_mm_uprobe_count() does a atomic_dec.

    When I kept dec_mm_uprobe_count() inside munmap_uprobe(), I found most
    of the code nested too deep. Hence carved it out as a separate function.

    I open to suggestions for dec_mm_uprobe_count()

    --
    Thanks and Regards
    Srikar


    \
     
     \ /
      Last update: 2011-09-27 15:27    [W:0.030 / U:358.508 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site