On 14 April 2014 11:59, Oleg Nesterov oleg@redhat.com wrote:
On 04/11, Linus Torvalds wrote:
On Fri, Apr 11, 2014 at 10:24 AM, Oleg Nesterov oleg@redhat.com wrote:
+static void arch_uprobe_copy_ixol(struct xol_area *area, unsigned long vaddr,
struct arch_uprobe *auprobe)
+{ +#ifndef ARCH_UPROBE_XXX
copy_to_page(area->page, vaddr, &auprobe->ixol, sizeof(&auprobe->ixol));
/*
* We probably need flush_icache_user_range() but it needs vma.
* If this doesn't work define ARCH_UPROBE_XXX.
*/
flush_dcache_page(area->page);
+#else
struct mm_struct *mm = current->mm;
struct vm_area_struct *vma;
down_read(&mm->mmap_sem);
vma = find_exact_vma(mm, area->vaddr, area->vaddr + PAGE_SIZE);
if (vma) {
void *kaddr = kmap_atomic(area->page);
copy_to_user_page(vma, area->page,
vaddr, kaddr + (vaddr & ~PAGE_MASK),
&auprobe->ixol, sizeof(&auprobe->ixol));
kunmap_atomic(kaddr);
}
up_read(&mm->mmap_sem);
+#endif
Yeah, no, this is wrong.
Yesss, agreed.
So I really think we should just have a fixed "flush_icache_page(page,vaddr)" function. ... Then the uprobe case can just do
copy_to_page() flush_dcache_page() flush_icache_page()
And I obviously like this idea because (iiuc) it more or less matches flush_icache_page_xxx() I tried to suggest.
Would not page granularity to be too expensive? Note you need to do that on each probe hit and you flushing whole data and instruction page every time. IMHO it will work correctly when you flush just few dcache/icache lines that correspond to xol slot that got modified. Note copy_to_user_page takes len that describes size of area that has to be flushed. Given that we are flushing xol area page at this case; and nothing except one xol slot is any interest for current task, and if CPU can flush one dcache and icache page as quickly as it can flush range, my remark may not matter.
I personally would prefer if we could have function like copy_to_user_page but without requirement to pass vma to it.
Thanks, Victor
But we need a short term solution for arm. And unless I misunderstood Russell (this is quite possible), arm needs to disable preemption around copy + flush.
Russel, so what do you think we can do for arm right now? Does the patch above (and subsequent discussion) answer the "why reinvent" question ?
Oleg.