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

[Dave Hansen]

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

> +	/* 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.

> +	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?

> +	}
> +	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.

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