Re: [PATCH 2/2] ZERO PAGE by pte_special

[KAMEZAWA Hiroyuki]

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



--
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/