linux-kernel - Re: [PATCH v2 1/4] mm: introduce compaction and migration for virtio ballooned pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120630013447.GA1545@x61.redhat.com>
Date:	Fri, 29 Jun 2012 22:34:48 -0300
From:	Rafael Aquini <aquini@...hat.com>
To:	Minchan Kim <minchan@...nel.org>
Cc:	linux-mm@...ck.org, Rik van Riel <riel@...hat.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	linux-kernel@...r.kernel.org,
	virtualization@...ts.linux-foundation.org,
	Andi Kleen <andi@...stfloor.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v2 1/4] mm: introduce compaction and migration for virtio
 ballooned pages

Howdy Minchan,

On Sat, Jun 30, 2012 at 07:03:33AM +0900, Minchan Kim wrote:
> > > > +static inline bool is_balloon_page(struct page *page)
> > > > +{
> > > > +        return (page->mapping == balloon_mapping) ? true : false;
> > > > +}
> > > 
> > > 
> > > What lock should it protect?
> > > 
> > I'm afraid I didn't quite get what you meant by that question. If you were
> > referring to lock protection to the address_space balloon_mapping, we don't need
> > it. balloon_mapping, once initialized lives forever (as long as driver is
> > loaded, actually) as a static reference that just helps us on identifying pages 
> > that are enlisted in a memory balloon as well as it keeps the callback pointers 
> > to functions that will make those pages mobility magic happens.
> 
> Thanks. That's what I want to know.
> If anyone(like me don't know of ballooning in detail) see this, it would be very helpful.
> 
Good point! I'll make sure this explanation gets properly registered either at commit log or
at a comment along with balloon_mapping declaration, then.



> > > > +		if (likely(PageLRU(page))) {
> > > 
> > > 
> > > We can't make sure it is likely because there might be so many pages for kernel.
> > > 
> > I thought that by that far in codepath that would be the likelihood since most
> > pages of an ordinary workload will be at LRU lists. Following that idea, it
> > sounded neat to hint the compiler to not branch for that block. But, if in the
> > end that is just a "bad hint", I'll get rid of it right away.
> 
> Yeb. I hope you remove this.
> If you want really, it should be separated patch because it's not related to your
> series.
> 
That will be removed, then.



> > > > +/* __isolate_lru_page() counterpart for a ballooned page */
> > > > +bool isolate_balloon_page(struct page *page)
> > > > +{
> > > > +	if (WARN_ON(!is_balloon_page(page)))
> > > > +		return false;
> > > > +
> > > > +	if (likely(get_page_unless_zero(page))) {
> > > > +		/*
> > > > +		 * We can race against move_to_new_page() & __unmap_and_move().
> > > > +		 * If we stumble across a locked balloon page and succeed on
> > > > +		 * isolating it, the result tends to be disastrous.
> > > > +		 */
> > > > +		if (likely(trylock_page(page))) {
> > > > +			/*
> > > > +			 * A ballooned page, by default, has just one refcount.
> > > > +			 * Prevent concurrent compaction threads from isolating
> > > > +			 * an already isolated balloon page.
> > > > +			 */
> > > > +			if (is_balloon_page(page) && (page_count(page) == 2)) {
> > > > +				page->mapping->a_ops->invalidatepage(page, 0);
> > > 
> > > 
> > > Could you add more meaningful name wrapping raw invalidatepage?
> > > But I don't know what is proper name. ;)
> > > 
> > If I understood you correctely, your suggestion is to add two extra callback
> > pointers to struct address_space_operations, instead of re-using those which are
> > already there, and are suitable for the mission. Is this really necessary? It
> > seems just like unecessary bloat to struct address_space_operations, IMHO.
> 
> I meant this. :)
> 
> void isolate_page_from_balloonlist(struct page* page)
> {
> 	page->mapping->a_ops->invalidatepage(page, 0);
> }
> 
> 	if (is_balloon_page(page) && (page_count(page) == 2)) {
> 		isolate_page_from_balloonlist(page);
> 	}
> 
Humm, my feelings on your approach here: just an unecessary indirection that
doesn't bring the desired code readability improvement.
If the header comment statement on balloon_mapping->a_ops is not clear enough 
on those methods usage for ballooned pages:

..... 
/*
 * Balloon pages special page->mapping.
 * users must properly allocate and initialize an instance of balloon_mapping,
 * and set it as the page->mapping for balloon enlisted page instances.
 *
 * address_space_operations necessary methods for ballooned pages:
 *   .migratepage    - used to perform balloon's page migration (as is)
 *   .invalidatepage - used to isolate a page from balloon's page list
 *   .freepage       - used to reinsert an isolated page to balloon's page list
 */
struct address_space *balloon_mapping;
EXPORT_SYMBOL_GPL(balloon_mapping);
.....

I can add an extra commentary, to recollect folks about that usage, next to the
points where those callbacks are used at isolate_balloon_page() &
putback_balloon_page(). What do you think?


> Thanks!
> 
Thank you for such attention and valuable feedback! Have a nice weekend!

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