lkml.org 
[lkml]   [2009]   [Jul]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/2] ZERO PAGE by pte_special


    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);

    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).

    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.

    Linus


    \
     
     \ /
      Last update: 2009-10-18 23:28    [W:0.025 / U:59.124 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site