Hi Oleg, Thanks for your kind review!
> On Jun 5, 2019, at 3:02 AM, Oleg Nesterov <[email protected]> wrote: > > On 06/04, Song Liu wrote: >> >> Currently, uprobe swaps the target page with a anonymous page in both >> install_breakpoint() and remove_breakpoint(). When all uprobes on a page >> are removed, the given mm is still using an anonymous page (not the >> original page). > > Agreed, it would be nice to avoid this, > >> @@ -461,9 +471,10 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, >> struct mm_struct *mm, >> unsigned long vaddr, uprobe_opcode_t opcode) >> { >> struct uprobe *uprobe; >> - struct page *old_page, *new_page; >> + struct page *old_page, *new_page, *orig_page = NULL; >> struct vm_area_struct *vma; >> int ret, is_register, ref_ctr_updated = 0; >> + pgoff_t index; >> >> is_register = is_swbp_insn(&opcode); >> uprobe = container_of(auprobe, struct uprobe, arch); >> @@ -501,6 +512,19 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, >> struct mm_struct *mm, >> copy_highpage(new_page, old_page); >> copy_to_page(new_page, vaddr, &opcode, UPROBE_SWBP_INSN_SIZE); >> >> + index = vaddr_to_offset(vma, vaddr & PAGE_MASK) >> PAGE_SHIFT; >> + orig_page = find_get_page(vma->vm_file->f_inode->i_mapping, index); > > I think you should take is_register into account, if it is true we are going > to install the breakpoint so we can avoid find_get_page/pages_identical. Good idea! I will add this to v3. > >> + if (orig_page) { >> + if (pages_identical(new_page, orig_page)) { >> + /* if new_page matches orig_page, use orig_page */ >> + put_page(new_page); >> + new_page = orig_page; > > Hmm. can't we simply unmap the page in this case? I haven't found an easier way here. I tried with zap_vma_ptes() and unmap_page_range(). But neither of them works well here. Also, we need to deal with *_mm_counter, rmap, etc. So I guess reusing __replace_page() (as current patch) is probably the easiest solution. Did I miss anything? Thanks again, Song
