Re: [RFC v1][PATCH]page_fault retry with NOPAGE_RETRY

[Ying Han]

Thanks Ingo for your comments and now i am working on V2 which should
be posted later today.

On Sun, Nov 23, 2008 at 1:18 AM, Ingo Molnar <mingo@xxxxxxx> wrote:
>
> * Ying Han <yinghan@xxxxxxxxxx> wrote:
>
>> page fault retry with NOPAGE_RETRY
>
> Interesting patch.
Thank you, glad to know that.
>
>> Allow major faults to drop the mmap_sem read lock while waitting for
>> synchronous disk read. This allows another thread which wishes to grab
>> down_read(mmap_sem) to proceed while the current is waitting the disk IO.
>
> Do you mean down_write()? down_read() can already be nested
> arbitrarily.
fixed. it should be down_write()

>> The patch flags current->flags to PF_FAULT_MAYRETRY as identify that
>> the caller can tolerate the retry in the filemap_fault call patch.
>>
>> Benchmark is done by mmap in huge file and spaw 64 thread each
>> faulting in pages in reverse order, the the result shows 8%
>> porformance hit with the patch.
>
> I suspect we also want to see the cases where this change helps?
i am working on more benchmark to show performance improvement.
>
> Also, constructs like this are pretty ugly:
>
>> +#ifdef CONFIG_X86_64
>> +asmlinkage
>> +#endif
>> +void do_page_fault(struct pt_regs *regs, unsigned long error_code)
>> +{
>> +     current->flags |= PF_FAULT_MAYRETRY;
>> +     __do_page_fault(regs, error_code);
>> +     current->flags &= ~PF_FAULT_MAYRETRY;
>> +}
>
> This seems to be unnecessary runtime overhead to pass in a flag to
> handle_mm_fault(). Why not extend the 'write' flag of
> handle_mm_fault() to also signal "arch is able to retry"?
thanks and fixed in V2

>
> Also, _if_ we decide that from-scratch pagefault retries are good, i
> see no reason why this should not be extended to all architectures:
>
> The retry should happen purely in the MM layer - all information is
> available already, and much of do_page_fault() could generally be
> moved into mm/memory.c, with one or two arch-provided standard
> callbacks to express certain page fault quirks. (such as vm86 mode on
> x86)
>
> (Such a design would allow more nice cleanups - handle_mm_fault()
> could inline inside the pagefault handler, etc.)
I will make the megapatch in V2 for each architecture support and send
to Andrew,
linux-kernel and linux-arch. thanks.

>
> Also, a few small details. Please use this proper multi-line comment
> style:
>
>> +                     /*
>> +                      * Page is already locked by someone else.
>> +                      *
>> +                      * We don't want to be holding down_read(mmap_sem)
>> +                      * inside lock_page(). We use wait_on_page_lock here
>> +                      * to just wait until the page is unlocked, but we
>> +                      * don't really need
>> +                      * to lock it.
>> +                      */
thanks and fixed.
> Not this one:
>
>> +     /* page may be available, but we have to restart the process
>> +      * because mmap_sem was dropped during the ->fault */
>
>        Ingo
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/