> > The rules that I am using are: > > > > mmap_uprobe() increments the count if > > - it successfully adds a breakpoint. > > - it not add a breakpoint, but sees that there is a underlying > > breakpoint (via a read_opcode call). > > > > munmap_uprobe() decrements the count if > > - it sees a underlying breakpoint, (via a read_opcode call) > > - Subsequent unregister_uprobe wouldnt find the breakpoint > > unless a mmap_uprobe kicks in, since the old vma would be > > dropped just after munmap_uprobe. > > > > register_uprobe increments the count if: > > - it successfully adds a breakpoint. > > > > unregister_uprobe decrements the count if: > > - it sees a underlying breakpoint and removes successfully. > > (via a read_opcode call) > > - Subsequent munmap_uprobe wouldnt find the breakpoint > > since there is no underlying breakpoint after the > > breakpoint removal. > > The problem I'm having is that such stuff isn't included in the patch > set. > > We've got both comments in the C language and Changelog in our patch > system, yet you consistently fail to use either to convey useful > information on non-trivial bits like this. > Agree, I will put this as part of comments. > This leaves the reviewer wondering if you've actually considered stuff > properly, then me actually finding bugs in there does of course > undermine that even further. > > What I really would like is for this patch set not to have such subtle > stuff at all, esp. at first. Once its in and its been used a bit we can > start optimizing and add subtle crap like this. We actually started the discussion of why we increment the count in mmap_uprobe() in EEXIST case (and read_opcode()). It exists for two reasons. - To handle fork case (that I wrote in another mail). - To handle mremap.(the case where we are discussing now) I would contend that removing the breakpoint in munmap doesnt amount to optimization. Since the start of unmap(), there cannot be another remove_breakpoint called for the vma,vaddr tuple, until the vma is cleaned up, or the subsequent mmap() is done. So the case of accounting for an already decremented count should never occur. I was following the general convention being used within the kernel to not bother about the area that we are going to unmap. For example: If a ptraced area were to be unmapped or remapped, I dont see the breakpoint being removed and added back. Also if a ptrace process is exitting, we dont go about removing the installed breakpoints. Also we would still need the check for EEXIST and read_opcode for handling the fork() case. So even if we add extra line to remove the actual breakpoint in munmap, It doesnt make the code any more simpler. -- 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>