Re: Re: Re: [PATCH 5/5] mm/readahead: Use find_get_pages instead of radix_tree_lookup.

[Fengguang Wu <fengguang.wu@intel.com>]

On Tue, Oct 16, 2012 at 10:29:31PM +0530, Raghavendra D Prabhu wrote:
> Hi,
> 
> 
> * On Fri, Sep 28, 2012 at 08:18:50PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
> >On Wed, Sep 26, 2012 at 08:28:20AM +0530, Raghavendra D Prabhu wrote:
> >>Hi,
> >>
> >>
> >>* On Sat, Sep 22, 2012 at 09:15:07PM +0800, Fengguang Wu <fengguang.wu@intel.com> wrote:
> >>>On Sat, Sep 22, 2012 at 04:03:14PM +0530, raghu.prabhu13@gmail.com wrote:
> >>>>From: Raghavendra D Prabhu <rprabhu@wnohang.net>
> >>>>
> >>>>Instead of running radix_tree_lookup in a loop and lock/unlocking in the
> >>>>process, find_get_pages is called once, which returns a page_list, some of which
> >>>>are not NULL and are in core.
> >>>>
> >>>>Also, since find_get_pages returns number of pages, if all pages are already
> >>>>cached, it can return early.
> >>>>
> >>>>This will be mostly helpful when a higher proportion of nr_to_read pages are
> >>>>already in the cache, which will mean less locking for page cache hits.
> >>>
> >>>Do you mean the rcu_read_lock()? But it's a no-op for most archs.  So
> >>>the benefit of this patch is questionable. Will need real performance
> >>>numbers to support it.
> >>
> >>Aside from the rcu lock/unlock, isn't it better to not make separate
> >>calls to radix_tree_lookup and merge them into one call? Similar
> >>approach is used with pagevec_lookup which is usually used when one
> >>needs to deal with a set of pages.
> >
> >Yeah, batching is generally good, however find_get_pages() is not the
> >right tool. It costs:
> >- get/release page counts
> >- likely a lot more searches in the address space, because it does not
> > limit the end index of the search.
> >
> >radix_tree_next_hole() will be the right tool, and I have a patch to
> >make it actually smarter than the current dumb loop.
> 
> Good to know.
> 
> >
> >>>>Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> >>>>---
> >>>> mm/readahead.c | 31 +++++++++++++++++++++++--------
> >>>> 1 file changed, 23 insertions(+), 8 deletions(-)
> >>>>
> >>>>diff --git a/mm/readahead.c b/mm/readahead.c
> >>>>index 3977455..3a1798d 100644
> >>>>--- a/mm/readahead.c
> >>>>+++ b/mm/readahead.c
> >>>>@@ -157,35 +157,42 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
> >>>> {
> >>>> 	struct inode *inode = mapping->host;
> >>>> 	struct page *page;
> >>>>+	struct page **page_list = NULL;
> >>>> 	unsigned long end_index;	/* The last page we want to read */
> >>>> 	LIST_HEAD(page_pool);
> >>>> 	int page_idx;
> >>>> 	int ret = 0;
> >>>> 	int ret_read = 0;
> >>>>+	unsigned long num;
> >>>>+	pgoff_t page_offset;
> >>>> 	loff_t isize = i_size_read(inode);
> >>>>
> >>>> 	if (isize == 0)
> >>>> 		goto out;
> >>>>
> >>>>+	page_list = kzalloc(nr_to_read * sizeof(struct page *), GFP_KERNEL);
> >>>>+	if (!page_list)
> >>>>+		goto out;
> >>>
> >>>That cost one more memory allocation and added code to maintain the
> >>>page list. The original code also don't have the cost of grabbing the
> >>>page count, which eliminate the trouble of page release.
> >>>
> >>>> 	end_index = ((isize - 1) >> PAGE_CACHE_SHIFT);
> >>>>+	num = find_get_pages(mapping, offset, nr_to_read, page_list);
> >>>
> >>>Assume we want to readahead pages for indexes [0, 100] and the cached
> >>>pages are in [1000, 1100]. find_get_pages() will return the latter.
> >>>Which is probably not the your expected results.
> >>
> >>I thought if I ask for pages in the range [0,100] it will return a
> >>sparse array [0,100] but with holes (NULL) for pages not in cache
> >>and references to pages in cache. Isn't that the expected behavior?
> >
> >Nope. The comments above find_get_pages() made it clear, that it's
> >limited by the number of pages rather than the end page index.
> 
> Yes, I noticed that, however since nr_to_read in this case is equal
> to nr_pages.
> 
> However, I think I understand what you are saying -- ie. if offset +

You seem to still don't understand it after all the explanations..

> nr_pages exceeds the end_index then it will return pages not
> belonging to the file, is that right?

Nope.

> In that case, won't capping nr_pages do, ie. check if offset +
> nr_pages > end_index  and if that is true, then reduce  nr_to_read
> by end_index. Won't that work?

Either you or me are get lost in the discussion. Please research the
code and emails and show me working code (that's actually tested) that
can serve as the new base for our discussions.

Thanks,
Fengguang

> >>>> 	/*
> >>>> 	 * Preallocate as many pages as we will need.
> >>>> 	 */
> >>>> 	for (page_idx = 0; page_idx < nr_to_read; page_idx++) {
> >>>>-		pgoff_t page_offset = offset + page_idx;
> >>>>+		if (page_list[page_idx]) {
> >>>>+			page_cache_release(page_list[page_idx]);
> >>>>+			continue;
> >>>>+		}
> >>>>+
> >>>>+		page_offset = offset + page_idx;
> >>>>
> >>>> 		if (page_offset > end_index)
> >>>> 			break;
> >>>>
> >>>>-		rcu_read_lock();
> >>>>-		page = radix_tree_lookup(&mapping->page_tree, page_offset);
> >>>>-		rcu_read_unlock();
> >>>>-		if (page)
> >>>>-			continue;
> >>>>-
> >>>> 		page = page_cache_alloc_readahead(mapping);
> >>>>-		if (!page)
> >>>>+		if (unlikely(!page))
> >>>> 			break;
> >>>
> >>>That break will leave the remaining pages' page_count lifted and lead
> >>>to memory leak.
> >>
> >>Thanks. Yes, I realized that now.
> >>>
> >>>> 		page->index = page_offset;
> >>>> 		list_add(&page->lru, &page_pool);
> >>>>@@ -194,6 +201,13 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
> >>>> 			lookahead_size = 0;
> >>>> 		}
> >>>> 		ret++;
> >>>>+
> >>>>+		/*
> >>>>+		 * Since num pages are already returned, bail out after
> >>>>+		 * nr_to_read - num pages are allocated and added.
> >>>>+		 */
> >>>>+		if (ret == nr_to_read - num)
> >>>>+			break;
> >>>
> >>>Confused. That break seems unnecessary?
> >>
> >>I fixed that:
> >>
> >>
> >> -               pgoff_t page_offset = offset + page_idx;
> >> -
> >> -               if (page_offset > end_index)
> >> -                       break;
> >> -
> >> -               rcu_read_lock();
> >> -               page = radix_tree_lookup(&mapping->page_tree, page_offset);
> >> -               rcu_read_unlock();
> >> -               if (page)
> >
> >> +               if (page_list[page_idx]) {
> >> +                       page_cache_release(page_list[page_idx]);
> >
> >No, you cannot expect:
> >
> >       page_list[page_idx]->index == page_idx
> >
> >Thanks,
> >Fengguang
> >
> >
> >> +                       num--;
> >>                         continue;
> >> +               }
> >> +
> >> +               page_offset = offset + page_idx;
> >> +
> >> +               /*
> >> +                * Break only if all the previous
> >> +                * references have been released
> >> +                */
> >> +               if (page_offset > end_index) {
> >> +                       if (!num)
> >> +                               break;
> >> +                       else
> >> +                               continue;
> >> +               }
> >>
> >>                 page = page_cache_alloc_readahead(mapping);
> >> -               if (!page)
> >> -                       break;
> >> +               if (unlikely(!page))
> >> +                       continue;
> >>
> >>>
> >>>Thanks,
> >>>Fengguang
> >>>
> >>>> 	}
> >>>>
> >>>> 	/*
> >>>>@@ -205,6 +219,7 @@ __do_page_cache_readahead(struct address_space *mapping, struct file *filp,
> >>>> 		ret_read = read_pages(mapping, filp, &page_pool, ret);
> >>>> 	BUG_ON(!list_empty(&page_pool));
> >>>> out:
> >>>>+	kfree(page_list);
> >>>> 	return (ret_read < 0 ? ret_read : ret);
> >>>> }
> >>>>
> >>>>--
> >>>>1.7.12.1
> >>>>
> >>>>--
> >>>>To unsubscribe, send a message with 'unsubscribe linux-mm' in
> >>>>the body to majordomo@kvack.org.  For more info on Linux MM,
> >>>>see: http://www.linux-mm.org/ .
> >>>>Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> >>>
> >>
> >>
> >>
> >>
> >>Regards,
> >>--
> >>Raghavendra Prabhu
> >>GPG Id : 0xD72BE977
> >>Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
> >>www: wnohang.net
> >
> >
> 
> 
> 
> 
> Regards,
> -- 
> Raghavendra Prabhu
> GPG Id : 0xD72BE977
> Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
> www: wnohang.net


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>