Re: [PATCHv2, RFC 02/30] mm: implement zero_huge_user_segment and friends

["Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>]

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