Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1577289 > unrolled thread

Re: [PATCHv6 01/37] mm, shmem: swich huge tmpfs to multi-order radix-tree entries

Started byMatthew Wilcox <willy@infradead.org>
First post2017-02-09 05:00 +0100
Last post2017-02-13 14:50 +0100
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCHv6 01/37] mm, shmem: swich huge tmpfs to multi-order  radix-tree entries Matthew Wilcox <willy@infradead.org> - 2017-02-09 05:00 +0100
    Re: [PATCHv6 01/37] mm, shmem: swich huge tmpfs to multi-order  radix-tree entries "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-02-09 18:00 +0100
      Re: [PATCHv6 01/37] mm, shmem: swich huge tmpfs to multi-order  radix-tree entries "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-02-13 14:50 +0100

#1577289 — Re: [PATCHv6 01/37] mm, shmem: swich huge tmpfs to multi-order radix-tree entries

FromMatthew Wilcox <willy@infradead.org>
Date2017-02-09 05:00 +0100
SubjectRe: [PATCHv6 01/37] mm, shmem: swich huge tmpfs to multi-order radix-tree entries
Message-ID<t8O3M-Pu-5@gated-at.bofh.it>
On Thu, Jan 26, 2017 at 02:57:43PM +0300, Kirill A. Shutemov wrote:
> +++ b/include/linux/pagemap.h
> @@ -332,6 +332,15 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping,
>  			mapping_gfp_mask(mapping));
>  }
>  
> +static inline struct page *find_subpage(struct page *page, pgoff_t offset)
> +{
> +	VM_BUG_ON_PAGE(PageTail(page), page);
> +	VM_BUG_ON_PAGE(page->index > offset, page);
> +	VM_BUG_ON_PAGE(page->index + (1 << compound_order(page)) < offset,
> +			page);
> +	return page - page->index + offset;
> +}

What would you think to:

static inline void check_page_index(struct page *page, pgoff_t offset)
{
	VM_BUG_ON_PAGE(PageTail(page), page);
	VM_BUG_ON_PAGE(page->index > offset, page);
	VM_BUG_ON_PAGE(page->index + (1 << compound_order(page)) <= offset,
			page);
}

(I think I fixed an off-by-one error up there ...  if
index + (1 << order) == offset, this is also a bug, right?
because offset would then refer to the next page, not this page)

static inline struct page *find_subpage(struct page *page, pgoff_t offset)
{
	check_page_index(page, offset);
	return page + (offset - page->index);
}

... then you can use check_page_index down ...

> @@ -1250,7 +1233,6 @@ struct page *find_lock_entry(struct address_space *mapping, pgoff_t offset)
>  			put_page(page);
>  			goto repeat;
>  		}
> -		VM_BUG_ON_PAGE(page_to_pgoff(page) != offset, page);

... here?

> @@ -1472,25 +1451,35 @@ unsigned find_get_pages(struct address_space *mapping, pgoff_t start,
>  			goto repeat;
>  		}
>  
> +		/* For multi-order entries, find relevant subpage */
> +		if (PageTransHuge(page)) {
> +			VM_BUG_ON(index - page->index < 0);
> +			VM_BUG_ON(index - page->index >= HPAGE_PMD_NR);
> +			page += index - page->index;
> +		}

Use find_subpage() here?

>  		pages[ret] = page;
>  		if (++ret == nr_pages)
>  			break;
> +		if (!PageTransCompound(page))
> +			continue;
> +		for (refs = 0; ret < nr_pages &&
> +				(index + 1) % HPAGE_PMD_NR;
> +				ret++, refs++, index++)
> +			pages[ret] = ++page;
> +		if (refs)
> +			page_ref_add(compound_head(page), refs);
> +		if (ret == nr_pages)
> +			break;

Can we avoid referencing huge pages specifically in the page cache?  I'd
like us to get to the point where we can put arbitrary compound pages into
the page cache.  For example, I think this can be written as:

		if (!PageCompound(page))
			continue;
		for (refs = 0; ret < nr_pages; refs++, index++) {
			if (index > page->index + (1 << compound_order(page)))
				break;
			pages[ret++] = ++page;
		}
		if (refs)
			page_ref_add(compound_head(page), refs);
		if (ret == nr_pages)
			break;

> @@ -1541,19 +1533,12 @@ unsigned find_get_pages_contig(struct address_space *mapping, pgoff_t index,
>  
> +		/* For multi-order entries, find relevant subpage */
> +		if (PageTransHuge(page)) {
> +			VM_BUG_ON(index - page->index < 0);
> +			VM_BUG_ON(index - page->index >= HPAGE_PMD_NR);
> +			page += index - page->index;
> +		}
> +
>  		pages[ret] = page;
>  		if (++ret == nr_pages)
>  			break;
> +		if (!PageTransCompound(page))
> +			continue;
> +		for (refs = 0; ret < nr_pages &&
> +				(index + 1) % HPAGE_PMD_NR;
> +				ret++, refs++, index++)
> +			pages[ret] = ++page;
> +		if (refs)
> +			page_ref_add(compound_head(page), refs);
> +		if (ret == nr_pages)
> +			break;
>  	}
>  	rcu_read_unlock();
>  	return ret;

Ugh, the same code again.  Hmm ... we only need to modify 'ret' as a result
of this ... so could we split it out like this?

static unsigned populate_pages(struct page **pages, unsigned i, unsigned max,
                                struct page *page)
{
        unsigned refs = 0;
        for (;;) {
                pages[i++] = page;
                if (i == max)
                        break;
                if (PageHead(page + 1))
                        break;
                page++;
                refs++;
        }
        if (refs)
                page_ref_add(compound_head(page), refs);
        return i;
}

> +unsigned find_get_pages_tag(struct address_space *mapping, pgoff_t *indexp,
>  			int tag, unsigned int nr_pages, struct page **pages)

>  			break;
> +		if (!PageTransCompound(page))
> +			continue;
> +		for (refs = 0; ret < nr_pages &&
> +				(index + 1) % HPAGE_PMD_NR;
> +				ret++, refs++, index++)
> +			pages[ret] = ++page;
> +		if (refs)
> +			page_ref_add(compound_head(page), refs);
> +		if (ret == nr_pages)
> +			break;
>  	}

... and again!

> @@ -2326,25 +2337,26 @@ void filemap_map_pages(struct vm_fault *vmf,
> +		/* For multi-order entries, find relevant subpage */
> +		if (PageTransHuge(page)) {
> +			VM_BUG_ON(index - page->index < 0);
> +			VM_BUG_ON(index - page->index >= HPAGE_PMD_NR);
> +			page += index - page->index;
> +		}
> +
>  		if (!PageUptodate(page) ||
>  				PageReadahead(page) ||

Readahead is PF_NO_COMPOUND ... so I don't see why this works?

> @@ -2378,8 +2390,14 @@ void filemap_map_pages(struct vm_fault *vmf,
>  		/* Huge page is mapped? No need to proceed. */
>  		if (pmd_trans_huge(*vmf->pmd))
>  			break;
> -		if (iter.index == end_pgoff)
> +		if (index == end_pgoff)
>  			break;
> +		if (page && PageTransCompound(page) &&
> +				(index & (HPAGE_PMD_NR - 1)) !=
> +				HPAGE_PMD_NR - 1) {
> +			index++;
> +			goto repeat;

Do we really have to go all the way back to the beginning of the loop?  It'd
be nice to be able to insert all of the relevant PTEs in a shorter loop here.
We'd need to bump the reference count N more times, and I think we do need
to check HWPoison for each subpage.

[toc] | [next] | [standalone]

#1577783

From"Kirill A. Shutemov" <kirill@shutemov.name>
Date2017-02-09 18:00 +0100
Message-ID<t90eC-98-23@gated-at.bofh.it>
In reply to#1577289
On Wed, Feb 08, 2017 at 07:57:27PM -0800, Matthew Wilcox wrote:
> On Thu, Jan 26, 2017 at 02:57:43PM +0300, Kirill A. Shutemov wrote:
> > +++ b/include/linux/pagemap.h
> > @@ -332,6 +332,15 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping,
> >  			mapping_gfp_mask(mapping));
> >  }
> >  
> > +static inline struct page *find_subpage(struct page *page, pgoff_t offset)
> > +{
> > +	VM_BUG_ON_PAGE(PageTail(page), page);
> > +	VM_BUG_ON_PAGE(page->index > offset, page);
> > +	VM_BUG_ON_PAGE(page->index + (1 << compound_order(page)) < offset,
> > +			page);
> > +	return page - page->index + offset;
> > +}
> 
> What would you think to:
> 
> static inline void check_page_index(struct page *page, pgoff_t offset)
> {
> 	VM_BUG_ON_PAGE(PageTail(page), page);
> 	VM_BUG_ON_PAGE(page->index > offset, page);
> 	VM_BUG_ON_PAGE(page->index + (1 << compound_order(page)) <= offset,
> 			page);
> }
> 
> (I think I fixed an off-by-one error up there ...  if
> index + (1 << order) == offset, this is also a bug, right?
> because offset would then refer to the next page, not this page)

Right, thanks.

> 
> static inline struct page *find_subpage(struct page *page, pgoff_t offset)
> {
> 	check_page_index(page, offset);
> 	return page + (offset - page->index);
> }
> 
> ... then you can use check_page_index down ...

Okay, makes sense.

> 
> > @@ -1250,7 +1233,6 @@ struct page *find_lock_entry(struct address_space *mapping, pgoff_t offset)
> >  			put_page(page);
> >  			goto repeat;
> >  		}
> > -		VM_BUG_ON_PAGE(page_to_pgoff(page) != offset, page);
> 
> ... here?

Ok.

> > @@ -1472,25 +1451,35 @@ unsigned find_get_pages(struct address_space *mapping, pgoff_t start,
> >  			goto repeat;
> >  		}
> >  
> > +		/* For multi-order entries, find relevant subpage */
> > +		if (PageTransHuge(page)) {
> > +			VM_BUG_ON(index - page->index < 0);
> > +			VM_BUG_ON(index - page->index >= HPAGE_PMD_NR);
> > +			page += index - page->index;
> > +		}
> 
> Use find_subpage() here?

Ok.

> >  		pages[ret] = page;
> >  		if (++ret == nr_pages)
> >  			break;
> > +		if (!PageTransCompound(page))
> > +			continue;
> > +		for (refs = 0; ret < nr_pages &&
> > +				(index + 1) % HPAGE_PMD_NR;
> > +				ret++, refs++, index++)
> > +			pages[ret] = ++page;
> > +		if (refs)
> > +			page_ref_add(compound_head(page), refs);
> > +		if (ret == nr_pages)
> > +			break;
> 
> Can we avoid referencing huge pages specifically in the page cache?  I'd
> like us to get to the point where we can put arbitrary compound pages into
> the page cache.  For example, I think this can be written as:
> 
> 		if (!PageCompound(page))
> 			continue;
> 		for (refs = 0; ret < nr_pages; refs++, index++) {
> 			if (index > page->index + (1 << compound_order(page)))
> 				break;
> 			pages[ret++] = ++page;
> 		}
> 		if (refs)
> 			page_ref_add(compound_head(page), refs);
> 		if (ret == nr_pages)
> 			break;

That's slightly more costly, but I guess that's fine.

> > @@ -1541,19 +1533,12 @@ unsigned find_get_pages_contig(struct address_space *mapping, pgoff_t index,
> >  
> > +		/* For multi-order entries, find relevant subpage */
> > +		if (PageTransHuge(page)) {
> > +			VM_BUG_ON(index - page->index < 0);
> > +			VM_BUG_ON(index - page->index >= HPAGE_PMD_NR);
> > +			page += index - page->index;
> > +		}
> > +
> >  		pages[ret] = page;
> >  		if (++ret == nr_pages)
> >  			break;
> > +		if (!PageTransCompound(page))
> > +			continue;
> > +		for (refs = 0; ret < nr_pages &&
> > +				(index + 1) % HPAGE_PMD_NR;
> > +				ret++, refs++, index++)
> > +			pages[ret] = ++page;
> > +		if (refs)
> > +			page_ref_add(compound_head(page), refs);
> > +		if (ret == nr_pages)
> > +			break;
> >  	}
> >  	rcu_read_unlock();
> >  	return ret;
> 
> Ugh, the same code again.  Hmm ... we only need to modify 'ret' as a result
> of this ... so could we split it out like this?
> 
> static unsigned populate_pages(struct page **pages, unsigned i, unsigned max,
>                                 struct page *page)
> {
>         unsigned refs = 0;
>         for (;;) {
>                 pages[i++] = page;
>                 if (i == max)
>                         break;
>                 if (PageHead(page + 1))
>                         break;

Hm? PageHead()? No. The next page can head or small.

I *guess* we can get away with !PageTail(page + 1)...

>                 page++;
>                 refs++;
>         }
>         if (refs)
>                 page_ref_add(compound_head(page), refs);
>         return i;
> }
> 
> > +unsigned find_get_pages_tag(struct address_space *mapping, pgoff_t *indexp,
> >  			int tag, unsigned int nr_pages, struct page **pages)
> 
> >  			break;
> > +		if (!PageTransCompound(page))
> > +			continue;
> > +		for (refs = 0; ret < nr_pages &&
> > +				(index + 1) % HPAGE_PMD_NR;
> > +				ret++, refs++, index++)
> > +			pages[ret] = ++page;
> > +		if (refs)
> > +			page_ref_add(compound_head(page), refs);
> > +		if (ret == nr_pages)
> > +			break;
> >  	}
> 
> ... and again!
> 
> > @@ -2326,25 +2337,26 @@ void filemap_map_pages(struct vm_fault *vmf,
> > +		/* For multi-order entries, find relevant subpage */
> > +		if (PageTransHuge(page)) {
> > +			VM_BUG_ON(index - page->index < 0);
> > +			VM_BUG_ON(index - page->index >= HPAGE_PMD_NR);
> > +			page += index - page->index;
> > +		}
> > +
> >  		if (!PageUptodate(page) ||
> >  				PageReadahead(page) ||
> 
> Readahead is PF_NO_COMPOUND ... so I don't see why this works?

We wouldn't see pages with readahead flag set/cleared until huge pages
support in ext4 is enabled at very end of the patchset.

> 
> > @@ -2378,8 +2390,14 @@ void filemap_map_pages(struct vm_fault *vmf,
> >  		/* Huge page is mapped? No need to proceed. */
> >  		if (pmd_trans_huge(*vmf->pmd))
> >  			break;
> > -		if (iter.index == end_pgoff)
> > +		if (index == end_pgoff)
> >  			break;
> > +		if (page && PageTransCompound(page) &&
> > +				(index & (HPAGE_PMD_NR - 1)) !=
> > +				HPAGE_PMD_NR - 1) {
> > +			index++;
> > +			goto repeat;
> 
> Do we really have to go all the way back to the beginning of the loop?  It'd
> be nice to be able to insert all of the relevant PTEs in a shorter loop here.
> We'd need to bump the reference count N more times, and I think we do need
> to check HWPoison for each subpage.

I'll look into it.

Thanks for review.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1579796

From"Kirill A. Shutemov" <kirill@shutemov.name>
Date2017-02-13 14:50 +0100
Message-ID<tapaW-4mz-13@gated-at.bofh.it>
In reply to#1577783
On Thu, Feb 09, 2017 at 07:58:20PM +0300, Kirill A. Shutemov wrote:
> I'll look into it.

I ended up with this (I'll test it more later):

void filemap_map_pages(struct vm_fault *vmf,
		pgoff_t start_pgoff, pgoff_t end_pgoff)
{
	struct radix_tree_iter iter;
	void **slot;
	struct file *file = vmf->vma->vm_file;
	struct address_space *mapping = file->f_mapping;
	pgoff_t last_pgoff = start_pgoff;
	loff_t size;
	struct page *page;
	bool mapped;

	rcu_read_lock();
	radix_tree_for_each_slot(slot, &mapping->page_tree, &iter,
			start_pgoff) {
		unsigned long index = iter.index;
		if (index < start_pgoff)
			index = start_pgoff;
		if (index > end_pgoff)
			break;
repeat:
		page = radix_tree_deref_slot(slot);
		if (unlikely(!page))
			continue;
		if (radix_tree_exception(page)) {
			if (radix_tree_deref_retry(page))
				slot = radix_tree_iter_retry(&iter);
			continue;
		}

		if (!page_cache_get_speculative(page))
			goto repeat;

		/* Has the page moved? */
		if (unlikely(page != *slot)) {
			put_page(page);
			goto repeat;
		}

		/* For multi-order entries, find relevant subpage */
		page = find_subpage(page, index);

		if (!PageUptodate(page) || PageReadahead(page))
			goto skip;
		if (!trylock_page(page))
			goto skip;

		if (page_mapping(page) != mapping || !PageUptodate(page))
			goto skip_unlock;

		size = round_up(i_size_read(mapping->host), PAGE_SIZE);
		if (compound_head(page)->index >= size >> PAGE_SHIFT)
			goto skip_unlock;

		if (file->f_ra.mmap_miss > 0)
			file->f_ra.mmap_miss--;
map_next_subpage:
		if (PageHWPoison(page))
			goto next;

		vmf->address += (index - last_pgoff) << PAGE_SHIFT;
		if (vmf->pte)
			vmf->pte += index - last_pgoff;
		last_pgoff = index;
		mapped = !alloc_set_pte(vmf, NULL, page);

		/* Huge page is mapped or last index? No need to proceed. */
		if (pmd_trans_huge(*vmf->pmd) ||
				index == end_pgoff) {
			unlock_page(page);
			break;
		}
next:
		if (page && PageCompound(page)) {
			/* Last subpage handled? */
			if ((index & (compound_nr_pages(page) - 1)) ==
					compound_nr_pages(page) - 1)
				goto skip_unlock;
			index++;
			page++;

			/*
			 * One page reference goes to page table mapping.
			 * Need additional reference, if last alloc_set_pte()
			 * succeed.
			 */
			if (mapped)
				get_page(page);
			goto map_next_subpage;
		}
skip_unlock:
		unlock_page(page);
skip:
		iter.index = compound_head(page)->index +
			compound_nr_pages(page) - 1;
		/* Only give up reference if alloc_set_pte() failed. */
		if (!mapped)
			put_page(page);
	}
	rcu_read_unlock();
}

-- 
 Kirill A. Shutemov

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web