On Wed, Aug 22, 2012 at 03:07:41AM +0300, Michael S. Tsirkin wrote:

On Tue, Aug 21, 2012 at 05:45:56PM -0300, Rafael Aquini wrote:

quoted

On Tue, Aug 21, 2012 at 10:30:31PM +0300, Michael S. Tsirkin wrote:

quoted

On Tue, Aug 21, 2012 at 04:23:58PM -0300, Rafael Aquini wrote:

quoted

On Tue, Aug 21, 2012 at 10:13:30PM +0300, Michael S. Tsirkin wrote:

quoted

quoted

I believe rcu_dereference_protected() is what I want/need here, since this code
is always called for pages which we hold locked (PG_locked bit).

It would only help if we locked the page while updating the mapping,
as far as I can see we don't.

But we can do it. In fact, by doing it (locking the page) we can easily avoid
the nasty race balloon_isolate_page / leak_balloon, in a much simpler way, IMHO.

Absolutely. Further, we should look hard at whether most RCU uses
in this patchset can be replaced with page lock.

Yeah, In fact, by testing/grabbing the page lock at leak_balloon() even the
module unload X migration / putback race seems to fade away, since migration
code holds the page locked all the way.
And that seems a quite easy task to be accomplished:

....

@@ -169,21 +197,61 @@ static void leak_balloon(struct virtio_balloon *vb, size_t

num)
        /* We can only do one array worth at a time. */
        num = min(num, ARRAY_SIZE(vb->pfns));

+       mutex_lock(&vb->balloon_lock);
        for (vb->num_pfns = 0; vb->num_pfns < num;
             vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
+               spin_lock(&vb->pages_lock);
+               /*
+                * 'virtballoon_isolatepage()' can drain vb->pages list
+                * making us to stumble across a _temporarily_ empty list.

This still worries me. If this happens we do not
lock the page so module can go away?
if not need to document why.

The module won't unload unless it leaks all its pages. If we hit that test that
worries you, leak_balloon() will get back to its caller -- remove_common(), and
it will kept looping at:

        /* There might be pages left in the balloon: free them. */
        while (vb->num_pages)
                leak_balloon(vb, vb->num_pages);

This is true because we do not mess with vb->num_pages while isolating/migrating
balloon pages, so the module will only unload when all isolated pages get back
to vb->pages_list and leak_balloon() reap them appropriatelly. As we will be
doing isolation/migration/putback steps under 'page lock' that race is gone.

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