lkml.org 
[lkml]   [2009]   [Jul]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 2/2] ZERO PAGE by pte_special
    From
    Linus Torvalds wrote:
    >
    >
    > On Thu, 9 Jul 2009, KAMEZAWA Hiroyuki wrote:
    >>
    >> + /* we can ignore zero page */
    >> + page = vm_normal_page(vma, addr, pte, 1);
    >
    >> - page = vm_normal_page(vma, addr, ptent);
    >> + page = vm_normal_page(vma, addr, ptent, 1);
    >
    >> - page = vm_normal_page(vma, address, pte);
    >> + page = vm_normal_page(vma, address, pte, (flags & FOLL_NOZERO));
    >
    >> + int ignore_zero = !!(flags & GUP_FLAGS_IGNORE_ZERO);
    >> ...
    >> + page = vm_normal_page(gate_vma, start,
    >> + *pte, ignore_zero);
    >
    >> + if (ignore_zero)
    >> + foll_flags |= FOLL_NOZERO;
    >
    >> + /* This returns NULL when we find ZERO page */
    >> + old_page = vm_normal_page(vma, address, orig_pte, 1);
    >
    >> + /* we can ignore zero page */
    >> + page = vm_normal_page(vma, addr, pte, 1);
    >
    >> + /* we avoid zero page here */
    >> + page = vm_normal_page(vma, addr, *pte, 1);
    >
    >> + /*
    >> + * Because we comes from try_to_unmap_file(), we'll never see
    >> + * ZERO_PAGE or ANON.
    >> + */
    >> + page = vm_normal_page(vma, address, *pte, 1);
    >
    >> struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long
    >> addr,
    >> - pte_t pte);
    >> + pte_t pte, int ignore_zero);
    >
    > So I'm quoting these different uses, because they show the pattern that
    > exists all over this patch: confusion about "no zero" vs "ignore zero" vs
    > just plain no explanation at all.
    >
    > Quite frankly, I hate the "ignore zero page" naming/comments. I can kind
    > of see why you named them that way - we'll not consider it a normal page.
    > But that's not "ignoring" it. That's very much noticing it, just saying we
    > don't want to get the "struct page" for it.
    >
    > I equally hate the anonymous "1" use, with or without comments. Does "1"
    > mean that you want the zero page, does it means you _don't_ want it, what
    > does it mean? Yes, I know that it means FOLL_NOZERO, and that when set, we
    > don't want the zero page, but regardless, it's just not very readable.
    >
    > So I would suggest:
    >
    > - never pass in "1".
    >
    > - never talk about "ignoring" it.
    >
    > - always pass in a _flag_, in this case FOLL_NOZERO.
    >
    > If you follow those rules, you almost don't need commentary. Assuming
    > somebody is knowledgeable about the Linux VM, and knows we have a zero
    > page, you can just see a line like
    >
    > page = vm_normal_page(vma, address, *pte, FOLL_NOZERO);
    >
    Ahh, yes. This looks much better. I'll do in this way in v4.



    > and you can understand that you don't want to see ZERO_PAGE. There's never
    > any question like "what does that '1' mean here?"
    >
    > In fact, I'd pass in all of "flags", and then inside vm_normal_page() just
    > do
    >
    > if (flags & FOLL_NOZERO) {
    > ...
    >
    > rather than ever have any boolean arguments.
    >
    > (Again, I think that we should unify all of FOLL_xyz and FAULT_FLAG_xyz
    > and GUP_xyz into _one_ namespace - probably all under FAULT_FLAG_xyz - but
    > that's still a separate issue from this particular patchset).
    >
    sure...it's confusing...I'll start some work to clean it up when I have
    a chance.


    > Anyway, that said, I think the patch looks pretty simple and fairly
    > straightforward. Looks very much like 2.6.32 material, assuming people
    > will test it heavily and clean it up as per above before the next merge
    > window.
    >

    Thanks,
    -Kame





    \
     
     \ /
      Last update: 2009-07-09 06:57    [W:0.040 / U:0.072 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site