On Tue, Aug 14, 2012 at 05:00:49PM -0300, Rafael Aquini wrote:

On Tue, Aug 14, 2012 at 10:35:25PM +0300, Michael S. Tsirkin wrote:

quoted

quoted

quoted

quoted

+/* __isolate_lru_page() counterpart for a ballooned page */
+bool isolate_balloon_page(struct page *page)
+{
+	if (WARN_ON(!movable_balloon_page(page)))

Looks like this actually can happen if the page is leaked
between previous movable_balloon_page and here.

quoted

+		return false;

Yes, it surely can happen, and it does not harm to catch it here, print a warn and
return.

If it is legal, why warn? For that matter why test here at all?

As this is a public symbol, and despite the usage we introduce is sane, the warn
was placed as an insurance policy to let us know about any insane attempt to use
the procedure in the future. That was due to a nice review nitpick, actually.

Even though the code already had a test to properly avoid this race you
mention, I thought that sustaining the warn was a good thing. As I told you,
despite real, I've never got (un)lucky enough to stumble across that race window
while testing the patch.

If your concern is about being too much verbose on logging, under certain
conditions, perhaps we can change that test to a WARN_ON_ONCE() ?

Mel, what are your thoughts here?
 

I viewed it as being defensive programming. VM_BUG_ON would be less
useful as it can be compiled out. If the race can be routinely hit then
multiple warnings is instructive in itself. I have no strong feelings
about this though. I see little harm in making the check but in light of
this conversation add a short comment explaining that the check should
be redundant.

-- 
Mel Gorman
SUSE Labs

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