On Fri, Oct 1, 2010 at 8:31 AM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> Also, I think the "RELEASE" naming is too much about the
> implementation, not about the context. I think it would be more
> sensible to call it "ALLOW_RETRY" or "ATOMIC" or something like this,
> and not make it about releasing the page lock so much as about what
> you want to happen.
>
> Because quite frankly, I could imagine other reasons to allow page fault retry.
>
> (Similarly, I would rename VM_FAULT_RELEASED to VM_FAULT_RETRY. Again:
> name things for the _concept_, not for some odd implementation issue)
All right, I changed for your names and I think they do help. There is
still one annoyance though (and this is why I had not made this purely
about retry in the first iteration): the up_read(mmap_sem) and the
wait_on_page_locked(page) still happen within filemap_fault(). I think
ideally we would prefer to move this into do_page_fault so that the
interface could *really* be about retry; however we can't easily do
that because the struct page is not exposed at that level.
>
>> - if (fault & VM_FAULT_MAJOR) {
>> - tsk->maj_flt++;
>> - perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MAJ, 1, 0,
>> - regs, address);
>> - } else {
>> - tsk->min_flt++;
>> - perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MIN, 1, 0,
>> - regs, address);
>> + if (release_flag) { /* Did not go through a retry */
>> + if (fault & VM_FAULT_MAJOR) {
>
> I really don't know if this is correct. What if you have two major
> faults due to the retry? What if the first one is a minor fault, but
> when we retry it's a major fault because the page got released? The
> nesting of the conditionals doesn't seem to make conceptual sense.
>
> I dunno. I can see what you're doing ("only do statistics for the
> first return"), but at the same time it just feels a bit icky.
In a way filemap_fault() already has that problem - during a minor
fault, the page could go away before we have a chance to lock it, and
the fault would still be counted as minor. So I just took that
property (first find_get_page() determines if we call the fault minor
or major) and extended it into the retry case.
One reasonable alternative, I think, would be to always count the
fault as major if we had to go through the retry path. The main
difference this would make, I think, is if two threads hit the exact
same page before we get a chance to load it from disk - in which case
they would both get counted as major faults, vs the current accounting
that would charge one as major and the other one as minor.
>> - lock_page(page);
>> + /* Lock the page. */
>> + if (!trylock_page(page)) {
>> + if (!(vmf->flags & FAULT_FLAG_RELEASE))
>> + __lock_page(page);
>> + else {
>> + /*
>> + * Caller passed FAULT_FLAG_RELEASE flag.
>> + * This indicates it has read-acquired mmap_sem,
>> + * and requests that it be released if we have to
>> + * wait for the page to be transferred from disk.
>> + * Caller will then retry starting with the
>> + * mmap_sem read-acquire