lkml.org 
[lkml]   [2012]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 0/3] uprobes: make register/unregister O(n)

    I read this code a few times, but I still think I am getting confused
    about few scenarios.

    So please correct me.

    > struct map_info {
    > struct map_info *next;
    > struct mm_struct *mm;
    > loff_t vaddr;
    > };
    >
    > static inline struct map_info *free_map_info(struct map_info *info)
    > {
    > struct map_info *next = info->next;
    > kfree(info);
    > return next;
    > }
    >
    > static struct map_info *
    > build_map_info(struct address_space *mapping, loff_t offset, bool is_register)
    > {
    > unsigned long pgoff = offset >> PAGE_SHIFT;
    > struct prio_tree_iter iter;
    > struct vm_area_struct *vma;
    > struct map_info *curr = NULL;
    > struct map_info *prev = NULL;
    > struct map_info *info;
    > int more = 0;
    >
    > again:
    > mutex_lock(&mapping->i_mmap_mutex);
    > vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) {
    > if (!valid_vma(vma, is_register))
    > continue;
    >
    > if (!prev) {
    > prev = kmalloc(sizeof(struct map_info),
    > GFP_NOWAIT | __GFP_NOMEMALLOC | __GFP_NOWARN);
    > if (!prev) {
    > more++;
    > continue;
    > }
    > prev->next = NULL;
    > }
    >
    > if (!atomic_inc_not_zero(&vma->vm_mm->mm_users))
    > continue;
    >
    > info = prev;
    > prev = prev->next;
    > info->next = curr;
    > curr = info;
    >
    > info->mm = vma->vm_mm;
    > info->vaddr = vma_address(vma, offset);
    > }
    > mutex_unlock(&mapping->i_mmap_mutex);
    >
    > if (!more)
    > goto out;
    >
    > prev = curr;
    > while (curr) {
    > mmput(curr->mm);
    > curr = curr->next;
    > }
    >
    > do {
    > info = kmalloc(sizeof(struct map_info), GFP_KERNEL);
    > if (!info) {
    > curr = ERR_PTR(-ENOMEM);
    > goto out;
    > }
    > info->next = prev;
    > prev = info;
    > } while (--more);
    >
    > goto again;

    This is more theory
    If the number of vmas in the priority tree keeps increasing in every
    iteration, and the kmalloc(GFP_NOWAIT) fails i.e more is !0, then
    dont we end up in a forever loop?

    Cant we just restrict this to just 2 iterations? [And depend on
    uprobe_mmap() to do the necessary if new vmas come in].

    > out:

    > while (prev)
    > prev = free_map_info(prev);

    If we were able to allocate all map_info objects in the first pass but
    the last vma belonged to a mm thats at exit, i.e atomic_inc_non_zero
    returned 0 , then prev is !NULL and more is 0. Then we seem to clear
    all the map_info objects without even decreasing the mm counts for which
    atomic_inc_non_zero() was successful. Will curr be proper in this case.

    Should this while be an if?

    I am sure I am missing something here. Probably I should take a look
    again.

    > return curr;
    > }
    >
    > static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
    > {
    > struct map_info *info;
    > int err = 0;
    >
    > info = build_map_info(uprobe->inode->i_mapping,
    > uprobe->offset, is_register);
    > if (IS_ERR(info))
    > return PTR_ERR(info);
    >
    > while (info) {
    > struct mm_struct *mm = info->mm;
    > struct vm_area_struct *vma;
    > loff_t vaddr;
    >
    > if (err)
    > goto free;
    >
    > down_write(&mm->mmap_sem);
    > vma = find_vma(mm, (unsigned long)info->vaddr);
    > if (!vma || !valid_vma(vma, is_register))
    > goto unlock;
    >
    > vaddr = vma_address(vma, uprobe->offset);
    > if (vma->vm_file->f_mapping->host != uprobe->inode ||
    > vaddr != info->vaddr)
    > goto unlock;
    >
    > if (is_register) {
    > err = install_breakpoint(uprobe, mm, vma, info->vaddr);
    > /*
    > * We can race against uprobe_register(), see the
    > * comment near uprobe_hash().
    > */
    > if (err == -EEXIST)
    > err = 0;
    > } else {
    > remove_breakpoint(uprobe, mm, info->vaddr);
    > }
    > unlock:
    > up_write(&mm->mmap_sem);
    > free:
    > mmput(mm);
    > info = free_map_info(info);
    > }
    >
    > return err;
    > }
    >



    \
     
     \ /
      Last update: 2012-06-04 20:21    [W:3.053 / U:0.300 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site