Date:	Tue, 26 Jun 2012 19:01:56 -0300
From:	Rafael Aquini <aquini@...hat.com>
To:	Mel Gorman <mel@....ul.ie>
Cc:	linux-mm@...ck.org, Rik van Riel <riel@...hat.com>,
	Andi Kleen <andi@...stfloor.org>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	linux-kernel@...r.kernel.org,
	virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH 1/4] mm: introduce compaction and migration for virtio
 ballooned pages

Mel,

First and foremost, thank you for taking the time to review these bits and
provide such valuable feedback.

On Tue, Jun 26, 2012 at 11:17:29AM +0100, Mel Gorman wrote:
> > +/* return 1 if page is part of a guest's memory balloon, 0 otherwise */
> > +static inline int PageBalloon(struct page *page)
> > +{
> > +	return is_balloon_page(page);
> > +}
> 
> bool
> 
> Why is there both is_balloon_page and PageBalloon? 
> 
> is_ballon_page is so simple it should just be a static inline here
> 
> extern struct address_space *balloon_mapping;
> static inline bool is_balloon_page(page)
> {
> 	return page->mapping == balloon_mapping;
> }
> 	
I was thinking about sustain the same syntax other page tests utilize,
but I rather stick to your suggestion on this one.

 
> >  #if defined CONFIG_COMPACTION || defined CONFIG_CMA
> > @@ -312,6 +313,14 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
> >  			continue;
> >  		}
> >  
> > +		/*
> > +		 * For ballooned pages, we need to isolate them before testing
> > +		 * for PageLRU, as well as skip the LRU page isolation steps.
> > +		 */
> 
> This says what, but not why.
> 
> I didn't check the exact mechanics of a balloon page but I expect it's that
> balloon pages are not on the LRU. If they are on the LRU, that's pretty dumb.
> 
> 
> /*
>  * Balloon pages can be migrated but are not on the LRU. Isolate
>  * them before LRU checks.
>  */
> 
> 
> It would be nicer to do this without gotos
> 
> /*
>  * It is possible to migrate LRU pages and balloon pages. Skip
>  * any other type of page
>  */
> if (is_balloon_page(page)) {
> 	if (!isolate_balloon_page(page))
> 		continue;
> } else if (PageLRU(page)) {
> 	....
> }
> 
> You will need to shuffle things around a little to make it work properly
> but if we handle other page types in the future it will be neater
> overall.
>
I'm glad you've put things this way on this one. Despite I was thinking on doing it
the way you suggested, I took the goto approach because I was afraid of doing
otherwise could be considered as an unnecessary radical surgery on established code.
Will do it, certainly.

 	
> > +struct address_space *balloon_mapping;
> > +EXPORT_SYMBOL(balloon_mapping);
> > +
> 
> EXPORT_SYMBOL_GPL?
> 
> I don't mind how it is exported as such. I'm idly curious if there are
> external closed modules that use the driver.
> 
To be honest with you, that was picked with no particular case in mind. And, since
you've raised this question, I'm also curious. However, after giving a thought
on your feedback, I believe EXPORT_SYMBOL_GPL suits far better.


> > +/* ballooned page id check */
> > +int is_balloon_page(struct page *page)
> > +{
> > +	struct address_space *mapping = page->mapping;
> > +	if (mapping == balloon_mapping)
> > +		return 1;
> > +	return 0;
> > +}
> > +
> > +/* __isolate_lru_page() counterpart for a ballooned page */
> > +int isolate_balloon_page(struct page *page)
> > +{
> > +	struct address_space *mapping = page->mapping;
> 
> This is a publicly visible function and while your current usage looks
> correct it would not hurt to do something like this;
> 
> if (WARN_ON(!is_page_ballon(page))
> 	return 0;
>
Excellent point!
 

> > +	if (mapping->a_ops->invalidatepage) {
> > +		/*
> > +		 * We can race against move_to_new_page() and stumble across a
> > +		 * locked 'newpage'. If we succeed on isolating it, the result
> > +		 * tends to be disastrous. So, we sanely skip PageLocked here.
> > +		 */
> > +		if (likely(!PageLocked(page) && get_page_unless_zero(page))) {
> 
> But the page can get locked after this point.
> 
> Would it not be better to do a trylock_page() and unlock the page on
> exit after the isolation completes?
> 
Far better, for sure! thanks (again)


> > @@ -78,7 +78,10 @@ void putback_lru_pages(struct list_head *l)
> >  		list_del(&page->lru);
> >  		dec_zone_page_state(page, NR_ISOLATED_ANON +
> >  				page_is_file_cache(page));
> > -		putback_lru_page(page);
> > +		if (unlikely(PageBalloon(page)))
> > +			VM_BUG_ON(!putback_balloon_page(page));
> 
> Why not BUG_ON?
> 
> What shocked me actually is that VM_BUG_ON code is executed on
> !CONFIG_DEBUG_VM builds and has been since 2.6.36 due to commit [4e60c86bd:
> gcc-4.6: mm: fix unused but set warnings]. I thought the whole point of
> VM_BUG_ON was to avoid expensive and usually unnecessary checks. Andi,
> was this deliberate?
> 
> Either way, you always want to call putback_ballon_page() so BUG_ON is
> more appropriate although gracefully recovering from the situation and a
> WARN would be better.
> 
Shame on me!
 I was lazy enough to not carefully read VM_BUG_ON's definition and get its
original purpose. Will change it, for sure.


Once more, thank you!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists