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    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site