On Tue, 2011-11-29 at 14:03 +0530, Srikar Dronamraju wrote:
> install_breakpoints cannot have !consumers to be true when called from
> register_uprobe. (Since unregister_uprobe() which does the removal of
> consumer cannot race with register_uprobe().)
Right, that's the easy case ;-)
> Now lets consider mmap_uprobe() being called from vm_adjust(), the
> preceding unmap_uprobe() has already decremented the count but left the
> count intact.
>
> if consumers is NULL, unregister_uprobes() has kicked already in, so
> there is no point in inserting the probe, Hence we return EEXIST. The
> following unregister_uprobe() (or the munmap_uprobe() which might race
> before unregister_uprobe) is also going to decrement the count. So we
> have a case where the same breakpoint is accounted as removed twice. To
> offset this, we pretend as if the breakpoint is around by incrementing
> the count.
There's 2 main cases,
A) vma_adjust() vs unregister_uprobe() and
B) mmap() vs unregister_uprobe().
The result of A should be -1 reference in total, since we're removing
the one probe. The result of B should be 0 since we're removing the
probe and we shouldn't be installing new ones.
A1)
vma_adjust()
munmap_uprobe()
unregister_uprobe()
mmap_uprobe()
delete_uprobe()
munmap will to -1, mmap will do +1, __unregister_uprobe() which is
serialized against vma_adjust() will do -1 on either the old or new vma,
resulting in a grand total of: -1+1-1=-1, OK
A2) breakpoint is in old, not in new, again two cases:
A2a) __unregister_uprobe() sees old
munmap -1, __unregister_uprobe -1, mmap 0: -2 FAIL
A2b) __unregister_uprobe() sees new
munmap -1, __unregister_uprobe 0, mmap 0: -1 OK
A3) breakpoint is in new, not in old, again two cases:
A3a) __unregister_uprobe() sees old
munmap 0, __unregister_uprobe 0, mmap: 1: 1 FAIL
A3b) __unregister_uprobe() seed new
munmap 0, __unregister_uprobe -1, mmap: 1: 0 FAIL
B1)
unregister_uprobe()
mmap()
mmap_uprobe()
__unregister_uprobe()
delete_uprobe()
mmap +1, __unregister_uprobe() -1: 0 OK
B2)
unregister_uprobe()
mmap()
__unregister_uprobe()
mmap_uprobe()
delete_uprobe()
mmap +1, __unregister_uprobe() 0: +1 FAIL
> Would it help if I add an extra check in mmap_uprobe?
>
> int mmap_uprobe(...) {
> ....
> ret = install_breakpoint(vma->vm_mm, uprobe);
> if (ret == -EEXIST) {
> if (!read_opcode(vma->vm_mm, vaddr, &opcode) &&
> (opcode == UPROBES_BKPT_INSN))
> atomic_inc(&vma->vm_mm->mm_uprobes_count);
> ret = 0;
> }
> ....
> }
> The extra read_opcode check will tell us if the breakpoint is still
> around and then only increment the count. (As in it will distinguish if
> the mmap_uprobe is from vm_adjust).
No, I don't see that fixing A2a for example.
Could be I confused myself above, but like said, this stuff hurt brain.
It might just be easiest not to optimize munmap and leave fancy stuff
for later.
--
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