Re: [RFC 4/4] speculative pag fault

["KAMEZAWA Hiroyuki" <kamezawa.hiroyu@jp.fujitsu.com>]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>