Messages in this thread | | | Date | Mon, 4 Jun 2012 23:07:11 +0530 | From | Srikar Dronamraju <> | Subject | Re: [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; > } >
| |