On 08/03/2013 07:17 PM, Kirill A. Shutemov wrote:
> If a huge page is only partly in the range we zero out the part,
> exactly like we do for partial small pages.
What's the logic behind this behaviour? Seems like the kind of place
that we would really want to be splitting pages.
> + if (partial_thp_start || lstart & ~PAGE_CACHE_MASK) {
> + pgoff_t off;
> + struct page *page;
> + unsigned pstart, pend;
> + void (*zero_segment)(struct page *page,
> + unsigned start, unsigned len);
> +retry_partial_start:
> + if (partial_thp_start) {
> + zero_segment = zero_huge_user_segment;
That's a pretty hackish way to conditionally call a function, especially
since its done twice in one function. :)
I seem to recall zero_user_segment() vs. zero_huge_user_segment() being
something that caused some ugliness in the previous versions too.
What's the barrier to just having a smart zero_..._user_segment()
function that can conditionally perform huge or base page-zeroing?
> + if (partial_thp_end) {
> + zero_segment = zero_huge_user_segment;
> + off = end & ~HPAGE_CACHE_INDEX_MASK;
> + pend = (lend - 1) & ~HPAGE_PMD_MASK;
> + } else {
> + zero_segment = zero_user_segment;
> + off = end;
> + pend = (lend - 1) & ~PAGE_CACHE_MASK;
> + }
We went though a similar exercise for the fault code (I think), but I
really think you need to refactor this. Way too much of the code is in
the style:
if (thp) {
// new behavior
} else {
// old behavior
}
To me, that's just a recipe that makes it hard to review, and I also bet
it'll make the thp much more prone to bitrot. Maybe something like this?
size_t page_cache_mask = PAGE_CACHE_MASK;
unsigned long end_mask = 0UL;
if (partial_thp_end) {
page_cache_mask = HPAGE_PMD_MASK;
end_mask = HPAGE_CACHE_INDEX_MASK;
}
...
magic_zero_user_segment(...);
off = end & ~end_mask;
pend = (lend - 1) & ~page_cache_mask;
Like I said before, I somehow like to rewrite your code. :)
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
