Jie Zhang wrote: > On 11/25/2009 02:16 PM, Jamie Lokier wrote: > >Mike Frysinger wrote: > >>From: Jie Zhang<jie.zh...@analog.com> > >> > >>The mmu code uses the copy_*_user_page() variants in access_process_vm() > >>rather than copy_*_user() as the former includes an icache flush. This is > >>important when doing things like setting software breakpoints with gdb. > >>So switch the nommu code over to do the same. > > > >Reasonable, but it's a bit subtle don't you think? > >How about a one-line comment saying why it's using copy_*_user_page()? > > > >(If it was called copy_*_user_flush_icache() I wouldn't say anything, > >but it isn't). > > > But I think it's well known in Linux kernel developers that > copy_to_user_page and copy_from_user_page should do cache flushing. It's > documented in Documentation/cachetlb.txt. I don't think it's necessary > to replicate it here.
You're right, however I now think the commit message is misleading. Since this is the *only place in the entire kernel* where these functions are used (plus the mmu equivalent), I'm not sure I'd agree about well known, and the name could be better (copy_*_user_ptrace()), but I agree now, it doesn't need a comment. It was the talk of icache flush which bothered me, as I (wrongly) assumed copy_*_user_page() was used elsewhere, without knowledge of icache vs non-icache differences - which are often the responsibility of userspace to get right, so often the kernel does not care. In fact, it's not just icache. copy_*_user_page() has to do some *data* cache flushing too, on some architecures. For example, see arch/sparc/include/asm/cacheflush_64.h: #define copy_to_user_page(vma, page, vaddr, dst, src, len) \ do { \ flush_cache_page(vma, vaddr, page_to_pfn(page)); \ memcpy(dst, src, len); \ flush_ptrace_access(vma, page, vaddr, src, len, 0); \ } while (0) #define copy_from_user_page(vma, page, vaddr, dst, src, len) \ do { \ flush_cache_page(vma, vaddr, page_to_pfn(page)); \ memcpy(dst, src, len); \ flush_ptrace_access(vma, page, vaddr, dst, len, 1); \ } while (0) I'm not sure why I don't see the same dcache flushing on ARM, so I wonder if the ARM implementation of these buggy. Anyway, that means the commit message is a little misleading, saying it's for the icache flush. It is for whatever icache and dcache flushes are needed for a ptrace access. Which is why, given they are only used for ptrace (have just grepped), I'm inclined to think it'd be clearer to rename the functions to copy_*_user_ptrace(). And make your no-mmu change of course :-) Any thoughts on the rename? -- Jamie _______________________________________________ uClinux-dev mailing list uClinux-dev@uclinux.org http://mailman.uclinux.org/mailman/listinfo/uclinux-dev This message was resent by uclinux-dev@uclinux.org To unsubscribe see: http://mailman.uclinux.org/mailman/options/uclinux-dev