Dave Hansen wrote:
> On 03/14/2013 10:50 AM, Kirill A. Shutemov wrote:
> > Let's add helpers to clear huge page segment(s). They provide the same
> > functionallity as zero_user_segment{,s} and zero_user, but for huge
> > pages
> ...
> > +static inline void zero_huge_user_segments(struct page *page,
> > + unsigned start1, unsigned end1,
> > + unsigned start2, unsigned end2)
> > +{
> > + zero_huge_user_segment(page, start1, end1);
> > + zero_huge_user_segment(page, start2, end2);
> > +}
>
> I'm not sure that this helper saves very much code. The one call later
> in these patches:
>
> + zero_huge_user_segments(page, 0, from,
> + from + len, HPAGE_PMD_SIZE);
>
> really only saves one line over this:
>
> zero_huge_user_segment(page, 0, from);
> zero_huge_user_segment(page, from + len,
> HPAGE_PMD_SIZE);
>
> and I think the second one is much more clear to read.
I've tried to mimic non-huge zero_user*, but, yeah, this is silly.
Will drop.
> I do see that there's a small-page variant of this, but I think that one
> was done to save doing two kmap_atomic() operations when you wanted to
> zero two separate operations. This variant doesn't have that kind of
> optimization, so it makes much less sense.
>
> > +void zero_huge_user_segment(struct page *page, unsigned start, unsigned end)
> > +{
> > + int i;
> > +
> > + BUG_ON(end < start);
> > +
> > + might_sleep();
> > +
> > + if (start == end)
> > + return;
>
> I've really got to wonder how much of an optimization this is in
> practice. Was there a specific reason this was added?
It's likely for simple_write_begin() to call zero[_huge]_user_segments()
with one of two segments start == end.
But, honestly, it was just easier to cut the corner case first and don't
bother about it in following code. ;)
> > + /* start and end are on the same small page */
> > + if ((start & PAGE_MASK) == ((end - 1) & PAGE_MASK))
> > + return zero_user_segment(page + (start >> PAGE_SHIFT),
> > + start & ~PAGE_MASK,
> > + ((end - 1) & ~PAGE_MASK) + 1);
>
> It wasn't immediately obvious to me why we need to optimize the "on the
> same page" case. I _think_ it's because using zero_user_segments()
> saves us a kmap_atomic() over the code below. Is that right? It might
> be worth a comment.
The code below will call zero_user_segment() twice for the same small
page, but here we can use just one.
I'll document it.
> > + zero_user_segment(page + (start >> PAGE_SHIFT),
> > + start & ~PAGE_MASK, PAGE_SIZE);
> > + for (i = (start >> PAGE_SHIFT) + 1; i < (end >> PAGE_SHIFT) - 1; i++) {
> > + cond_resched();
> > + clear_highpage(page + i);
>
> zero_user_segments() does a flush_dcache_page(), which wouldn't get done
> on these middle pages. Is that a problem?
I think, it is. Will fix.
> > + }
> > + zero_user_segment(page + i, 0, ((end - 1) & ~PAGE_MASK) + 1);
> > +}
>
> This code is dying for some local variables. It could really use a
> 'start_pfn_offset' and 'end_pfn_offset' or something similar. All of
> the shifting and masking is a bit hard to read and it would be nice to
> think of some real names for what it is doing.
>
> It also desperately needs some comments about how it works. Some
> one-liners like:
>
> /* zero the first (possibly partial) page */
> for()..
> /* zero the full pages in the middle */
> /* zero the last (possibly partial) page */
>
> would be pretty sweet.
Okay, will rework it.
--
Kirill A. Shutemov
--
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
