Re: [RFC 4/4] speculative pag fault

KAMEZAWA Hiroyuki
2009-12-19T06:49:57Z

Minchan Kim wrote:
>
>
> KAMEZAWA Hiroyuki wrote:
>> Lookup vma in lockless style, do page fault, and check mm's version
>> after takine page table lock. If racy, mm's version is invalid .
>> Then, retry page fault.
>>
>> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>> ---
>> arch/x86/mm/fault.c | 28 +++++++++++++++++++++++++---
>> mm/memory.c | 21 ++++++++++++++-------
>> 2 files changed, 39 insertions(+), 10 deletions(-)
>>
>> Index: mmotm-mm-accessor/arch/x86/mm/fault.c
>> ===================================================================
>> --- mmotm-mm-accessor.orig/arch/x86/mm/fault.c
>> +++ mmotm-mm-accessor/arch/x86/mm/fault.c
>> @@ -11,6 +11,7 @@
>> #include <linux/kprobes.h> /* __kprobes, ... */
>> #include <linux/mmiotrace.h> /* kmmio_handler, ... */
>> #include <linux/perf_event.h> /* perf_sw_event */
>> +#include <linux/hugetlb.h> /* is_vm_hugetlb...*/
>
> De we need this header file?
>
Sorry, not necessary. (I checked HUGETLB flag in early version..)

>>
>> #include <asm/traps.h> /* dotraplinkage, ... */
>> #include <asm/pgalloc.h> /* pgd_*(), ... */
>> @@ -952,6 +953,7 @@ do_page_fault(struct pt_regs *regs, unsi
>> struct mm_struct *mm;
>> int write;
>> int fault;
>> + int speculative;
>>
>> tsk = current;
>> mm = tsk->mm;
>> @@ -1040,6 +1042,17 @@ do_page_fault(struct pt_regs *regs, unsi
>> return;
>> }
>>
>> + if ((error_code & PF_USER) && mm_version_check(mm)) {
>> + vma = lookup_vma_cache(mm, address);
>> + if (vma && mm_version_check(mm) &&
>> + (vma->vm_start <= address) && (address < vma->vm_end)) {
>> + speculative = 1;
>> + goto found_vma;
>> + }
>> + if (vma)
>> + vma_release(vma);
>> + }
>> +
>> /*
>> * When running in the kernel we expect faults to occur only to
>> * addresses in user space. All other faults represent errors in
>> @@ -1056,6 +1069,8 @@ do_page_fault(struct pt_regs *regs, unsi
>> * validate the source. If this is invalid we can skip the address
>> * space check, thus avoiding the deadlock:
>> */
>> +retry_with_lock:
>> + speculative = 0;
>> if (unlikely(!mm_read_trylock(mm))) {
>> if ((error_code & PF_USER) == 0 &&
>> !search_exception_tables(regs->ip)) {
>> @@ -1073,6 +1088,7 @@ do_page_fault(struct pt_regs *regs, unsi
>> }
>>
>> vma = find_vma(mm, address);
>> +found_vma:
>> if (unlikely(!vma)) {
>> bad_area(regs, error_code, address);
>> return;
>> @@ -1119,6 +1135,7 @@ good_area:
>> */
>> fault = handle_mm_fault(mm, vma, address, write ? FAULT_FLAG_WRITE :
>> 0);
>>
>> +
>> if (unlikely(fault & VM_FAULT_ERROR)) {
>> mm_fault_error(regs, error_code, address, fault);
>> return;
>> @@ -1128,13 +1145,18 @@ good_area:
>> tsk->maj_flt++;
>> perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MAJ, 1, 0,
>> regs, address);
>> - } else {
>> + } else if (!speculative || mm_version_check(mm)) {
>
> How about define VM_FAULT_FAIL_SPECULATIVE_VMACACHE
> although mm guys don't like new VM_FAULT_XXX?
>
Yes, I just hesitated to do that. And anotehr reason is
Assing VM_FAULT_FAIL_SPE.. makes do_anonymous_page, do_wp_page,....etc
more complicated (for adding new pte code..)
I'd like to find good coding style, here.

> It would remove double check of mm_version_check. :)
>
> It's another topic.
> How about counting failure of speculative easily and expose it in perf or
> statm.
> During we can step into mainline, it helps our test case is good, I think.
>
Yes, I agree. While developping, I checked with "printk" and
found some races happen even in boot sequence :)

Thanks,
-Kame