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! _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization