>
> static void unmap_single_vma(struct mmu_gather *tlb,
> struct vm_area_struct *vma, unsigned long start_addr,
> unsigned long end_addr,
> struct zap_details *details)
> {
> unsigned long start = max(vma->vm_start, start_addr);
> unsigned long end;
>
> if (start >= vma->vm_end)
> return;
> end = min(vma->vm_end, end_addr);
> if (end <= vma->vm_start)
> return;
>
> <<<<<<< HEAD
> =======
> if (vma->vm_file)
> uprobe_munmap(vma, start, end);
>
> if (vma->vm_flags & VM_ACCOUNT)
> *nr_accounted += (end - start) >> PAGE_SHIFT;
>
> >>>>>>> linux-next/akpm-base
> if (unlikely(is_pfn_mapping(vma)))
> untrack_pfn_vma(vma, 0, 0);
>
>
> It made me look at uprobes. Noticed a few things...
>
I have responded to why I had to add a callback in unmap_single_vma in
response to Linus.
> > ...
> >
> > +static struct rb_root uprobes_tree = RB_ROOT;
> > +static DEFINE_SPINLOCK(uprobes_treelock); /* serialize rbtree access */
> > +
> > +#define UPROBES_HASH_SZ 13
> > +/* serialize (un)register */
> > +static struct mutex uprobes_mutex[UPROBES_HASH_SZ];
> > +#define uprobes_hash(v) (&uprobes_mutex[((unsigned long)(v)) %\
> > + UPROBES_HASH_SZ])
> > +
> > +/* serialize uprobe->pending_list */
> > +static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
> > +#define uprobes_mmap_hash(v) (&uprobes_mmap_mutex[((unsigned long)(v)) %\
> > + UPROBES_HASH_SZ])
>
> Presumably these locks were hashed for scalability reasons?
Yes,
uprobe_mmap_mutex is taken on every mmap/munmap operation.
Since we do a per file operation per mm operation, (walk thro the rmap and
insert/remove breakpoints), we looked at using i_mutex. However
Christoph wasnt happy to overload the usage of i_mutex. He suggested two
options,
1. adding another mutex in the inode structure
2. adding global hash locks. (which he recommended)
Adding a mutex in the inode structure, is a overkill.
But having just one mutex to guard all uprobe_mmap is a contention on
different mmaps. So we narrowed down to a hash mutex.
>
> If so, this won't be terribly effective when we have multiple mutexes
> occupying a single cacheline - the array entries should be padded out.
> Of course, that's all a complete waste of space on uniprocessor
> machines, but nobody seems to think of that any more ;(
>
Okay, I agree that having each mutex in a different cacheline helps.
If everyone agrees to this, I will have a addon patch that will move the
mutexes.
> There was no need to code the accessor functions as macros. It is, as
> always, better to use a nice C function which takes an argument which
> is as strictly typed as possible. ie, it *could* take a void*, but it
> would be better if it required an inode*.
>
I will add this change as part of the add-on patch.
> >
> > ...
> >
> > +static int read_opcode(struct mm_struct *mm, unsigned long vaddr,
> > + uprobe_opcode_t *opcode)
> > +{
[.....]
> > + vaddr_new = kmap_atomic(page);
> > + vaddr &= ~PAGE_MASK;
> > + memcpy(opcode, vaddr_new + vaddr, uprobe_opcode_sz);
> > + kunmap_atomic(vaddr_new);
>
> This is modifying user memory? flush_dcache_page() needed? Or perhaps
> we will need different primitives to diddle the instruction memory on
> architectures which care.
>
Here, we are just reading from the user memory,
The part where we insert/remove the breakpoint (write_opcode) does the flush.
> > +int mmap_uprobe(struct vm_area_struct *vma)
> > +{
> > + struct list_head tmp_list;
> > + struct uprobe *uprobe, *u;
> > + struct inode *inode;
> > + int ret = 0;
> > +
> > + if (!atomic_read(&uprobe_events) || !valid_vma(vma, true))
> > + return ret; /* Bail-out */
> > +
> > + inode = vma->vm_file->f_mapping->host;
> > + if (!inode)
> > + return ret;
> > +
> > + INIT_LIST_HEAD(&tmp_list);
> > + mutex_lock(uprobes_mmap_hash(inode));
> > + 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) {
> > + vaddr = vma_address(vma, uprobe->offset);
> > + if (vaddr < vma->vm_start || vaddr >= vma->vm_end) {
> > + put_uprobe(uprobe);
> > + continue;
> > + }
> > + ret = install_breakpoint(vma->vm_mm, uprobe, vma,
> > + vaddr);
> > + if (ret == -EEXIST)
> > + ret = 0;
>
> This now has the comment "Ignore double add:". That is a poor
> comment, because it doesn't tell us *why* a double-add is ignored.
>
We actually dont ignore the "Double-add".
install_breakpoint() has comments on when we return EEXIST.
uprobe_mmap() has comments on why EEXIST should be considered successful
as part of commit 682968e0 (uprobes/core: Optimize probe hits with the
help of a counter) which is
/*
* Unable to insert a breakpoint, but
* breakpoint lies underneath. Increment the
* probe count
*/
i.e insert_breakpoint() needs to insert a breakpoint, but if a
breakpoint is already there, then it doesnt need to do anything.
I will go ahead and remove the "Ignore double-add" comment.
--
thanks and regards
Srikar
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>