Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754059Ab2HNTut (ORCPT ); Tue, 14 Aug 2012 15:50:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:13391 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751467Ab2HNTur (ORCPT ); Tue, 14 Aug 2012 15:50:47 -0400 Date: Tue, 14 Aug 2012 22:51:39 +0300 From: "Michael S. Tsirkin" To: Rafael Aquini Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Rusty Russell , Rik van Riel , Mel Gorman , Andi Kleen , Andrew Morton , Konrad Rzeszutek Wilk , Minchan Kim Subject: Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages Message-ID: <20120814195139.GA28870@redhat.com> References: <20120813084123.GF14081@redhat.com> <20120814182244.GB13338@t510.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120814182244.GB13338@t510.redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5644 Lines: 165 On Tue, Aug 14, 2012 at 03:22:45PM -0300, Rafael Aquini wrote: > On Mon, Aug 13, 2012 at 11:41:23AM +0300, Michael S. Tsirkin wrote: > > > @@ -141,7 +151,10 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num) > > > set_page_pfns(vb->pfns + vb->num_pfns, page); > > > vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE; > > > totalram_pages--; > > > + spin_lock(&pages_lock); > > > list_add(&page->lru, &vb->pages); > > > > If list_add above is reordered with mapping assignment below, > > then nothing bad happens because balloon_mapping takes > > pages_lock. > > > > > + page->mapping = balloon_mapping; > > > + spin_unlock(&pages_lock); > > > } > > > > > > /* Didn't get any? Oh well. */ > > > @@ -149,6 +162,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num) > > > return; > > > > > > tell_host(vb, vb->inflate_vq); > > > + mutex_unlock(&balloon_lock); > > > } > > > > > > static void release_pages_by_pfn(const u32 pfns[], unsigned int num) > > > @@ -169,10 +183,22 @@ 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(&balloon_lock); > > > for (vb->num_pfns = 0; vb->num_pfns < num; > > > vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { > > > + /* > > > + * We can race against virtballoon_isolatepage() and end up > > > + * stumbling across a _temporarily_ empty 'pages' list. > > > + */ > > > + spin_lock(&pages_lock); > > > + if (unlikely(list_empty(&vb->pages))) { > > > + spin_unlock(&pages_lock); > > > + break; > > > + } > > > page = list_first_entry(&vb->pages, struct page, lru); > > > + page->mapping = NULL; > > > > Unlike the case above, here > > if = NULL write above is reordered with list_del below, > > then isolate_page can run on a page that is not > > on lru. > > > > So I think this needs a wmb(). > > And maybe a comment above explaining why it is safe? > > Good point. Presumably, this nit has potential to explain your guessing on the > read barrier stuff at movable_balloon_page() on the other patch. > What I think you should do is use rcu for access. And here sync rcu before freeing. Maybe an overkill but at least a documented synchronization primitive, and it is very light weight. > > > list_del(&page->lru); > > > > I wonder why changing page->lru here is safe against > > races with unmap_and_move in the previous patch. > > leak_balloon() doesn't race against unmap_and_move() because the later works on > an isolated page set. So, theoretically, pages being dequeued from balloon page > list here are either migrated (already) or they were not isolated yet. > So add a comment explaining why it is safe pls. > > > +/* > > > + * '*vb_ptr' allows virtballoon_migratepage() & virtballoon_putbackpage() to > > > + * access pertinent elements from struct virtio_balloon > > > + */ > > > > What if there is more than one balloon device? > > Is it possible to load this driver twice, or are you foreseeing a future case > where this driver will be able to manage several distinct memory balloons for > the same guest? > Second. It is easy to create several balloons they are just pci devices. > > > + /* Init the ballooned page->mapping special balloon_mapping */ > > > + balloon_mapping = kmalloc(sizeof(*balloon_mapping), GFP_KERNEL); > > > + if (!balloon_mapping) { > > > + err = -ENOMEM; > > > + goto out_free_vb; > > > + } > > > > Can balloon_mapping be dereferenced at this point? > > Then what happens? > > Since there's no balloon page enqueued for this balloon driver yet, there's no > chance of balloon_mapping being dereferenced at this point. > > > > > + > > > + INIT_RADIX_TREE(&balloon_mapping->page_tree, GFP_ATOMIC | __GFP_NOWARN); > > > + INIT_LIST_HEAD(&balloon_mapping->i_mmap_nonlinear); > > > + spin_lock_init(&balloon_mapping->tree_lock); > > > + balloon_mapping->a_ops = &virtio_balloon_aops; > > > > > > err = init_vqs(vb); > > > if (err) > > > @@ -373,6 +493,7 @@ out_del_vqs: > > > vdev->config->del_vqs(vdev); > > > out_free_vb: > > > kfree(vb); > > > + kfree(balloon_mapping); > > > > No need to set it to NULL? It seems if someone else allocates a mapping > > and gets this chunk of memory by chance, the logic in mm will get > > confused. > > Good point. It surely doesn't hurt be asured of this sort of safety. > > > > out: > > > return err; > > > } > > > @@ -397,6 +518,7 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev) > > > kthread_stop(vb->thread); > > > remove_common(vb); > > > kfree(vb); > > > + kfree(balloon_mapping); > > > > Neither here? > > ditto. > > > > > } > > > > > > #ifdef CONFIG_PM > > > diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h > > > index 652dc8b..930f1b7 100644 > > > --- a/include/linux/virtio_balloon.h > > > +++ b/include/linux/virtio_balloon.h > > > @@ -56,4 +56,8 @@ struct virtio_balloon_stat { > > > u64 val; > > > } __attribute__((packed)); > > > > > > +#if !defined(CONFIG_COMPACTION) > > > +struct address_space *balloon_mapping; > > > +#endif > > > + > > > > Anyone including this header will get a different copy of > > balloon_mapping. Besides, need to be ifdef KERNEL. > > Good point. I'll move this hunk to the balloon driver itself. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/