lkml.org 
[lkml]   [2013]   [Aug]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 19/23] truncate: support huge pages
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. :)


\
 
 \ /
  Last update: 2013-08-07 00:21    [W:1.587 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site