From: Andrew Morton <akpm@linux-foundation.org> To: Rafael Aquini <aquini@redhat.com> Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Rusty Russell <rusty@rustcorp.com.au>, "Michael S. Tsirkin" <mst@redhat.com>, Rik van Riel <riel@redhat.com>, Mel Gorman <mel@csn.ul.ie>, Andi Kleen <andi@firstfloor.org>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, Minchan Kim <minchan@kernel.org>, Rafael Aquini <aquini@linux.com> Subject: Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages Date: Wed, 18 Jul 2012 18:29:44 -0700 [thread overview] Message-ID: <20120718182944.24f59012.akpm@linux-foundation.org> (raw) In-Reply-To: <20120719010047.GD2313@t510.redhat.com> On Wed, 18 Jul 2012 22:00:48 -0300 Rafael Aquini <aquini@redhat.com> wrote: > > So the function needs a better name - one which communicates that it is > > a balloon page *for the purposes of processing by the compaction code*. > > Making the function private to compaction.c would help with that, if > > feasible. > > > > How about this (adjusted) approach: it fails checkpatch ;) > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1629,8 +1629,7 @@ static inline unsigned int debug_guardpage_minorder(void) > { return 0; } > static inline bool page_is_guard(struct page *page) { return false; } > #endif /* CONFIG_DEBUG_PAGEALLOC */ > > -#if (defined(CONFIG_VIRTIO_BALLOON) || \ > - defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION) > +#if (defined(CONFIG_VIRTIO_BALLOON) ||defined(CONFIG_VIRTIO_BALLOON_MODULE)) > extern bool putback_balloon_page(struct page *); > extern struct address_space *balloon_mapping; > > @@ -1638,11 +1637,13 @@ static inline bool is_balloon_page(struct page *page) > { > return (page->mapping && page->mapping == balloon_mapping); > } > +#if defined(CONFIG_COMPACTION) > +static inline bool balloon_compaction_enabled(void) { return true; } > #else > -static inline bool is_balloon_page(struct page *page) { return false; } > -static inline bool isolate_balloon_page(struct page *page) { return false; } > -static inline bool putback_balloon_page(struct page *page) { return false; } > -#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */ > +static inline bool putback_balloon_page(struct page *page) { return false; } > +static inline bool balloon_compaction_enabled(void) { return false; } > +#endif /* CONFIG_COMPACTION */ > +#endif /* (CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE) */ > > #endif /* __KERNEL__ */ > #endif /* _LINUX_MM_H */ > diff --git a/mm/migrate.c b/mm/migrate.c > index 59c7bc5..f5f6a7d 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -78,7 +78,8 @@ 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)); > - if (unlikely(is_balloon_page(page))) > + if (unlikely(is_balloon_page(page)) && > + balloon_compaction_enabled()) well, that helps readability. But what does is_balloon_page() return when invoked on a balloon page when CONFIG_COMPACTION=n? False, methinks. I think the code as you previously had it was OK, but the is_balloon_page() name is misleading. It really wants to be called is_potentially_compactible_balloon_page() :( Maybe rename it to compactible_balloon_page()?
WARNING: multiple messages have this Message-ID (diff)
From: Andrew Morton <akpm@linux-foundation.org> To: Rafael Aquini <aquini@redhat.com> Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Rusty Russell <rusty@rustcorp.com.au>, "Michael S. Tsirkin" <mst@redhat.com>, Rik van Riel <riel@redhat.com>, Mel Gorman <mel@csn.ul.ie>, Andi Kleen <andi@firstfloor.org>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, Minchan Kim <minchan@kernel.org>, Rafael Aquini <aquini@linux.com> Subject: Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages Date: Wed, 18 Jul 2012 18:29:44 -0700 [thread overview] Message-ID: <20120718182944.24f59012.akpm@linux-foundation.org> (raw) In-Reply-To: <20120719010047.GD2313@t510.redhat.com> On Wed, 18 Jul 2012 22:00:48 -0300 Rafael Aquini <aquini@redhat.com> wrote: > > So the function needs a better name - one which communicates that it is > > a balloon page *for the purposes of processing by the compaction code*. > > Making the function private to compaction.c would help with that, if > > feasible. > > > > How about this (adjusted) approach: it fails checkpatch ;) > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1629,8 +1629,7 @@ static inline unsigned int debug_guardpage_minorder(void) > { return 0; } > static inline bool page_is_guard(struct page *page) { return false; } > #endif /* CONFIG_DEBUG_PAGEALLOC */ > > -#if (defined(CONFIG_VIRTIO_BALLOON) || \ > - defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION) > +#if (defined(CONFIG_VIRTIO_BALLOON) ||defined(CONFIG_VIRTIO_BALLOON_MODULE)) > extern bool putback_balloon_page(struct page *); > extern struct address_space *balloon_mapping; > > @@ -1638,11 +1637,13 @@ static inline bool is_balloon_page(struct page *page) > { > return (page->mapping && page->mapping == balloon_mapping); > } > +#if defined(CONFIG_COMPACTION) > +static inline bool balloon_compaction_enabled(void) { return true; } > #else > -static inline bool is_balloon_page(struct page *page) { return false; } > -static inline bool isolate_balloon_page(struct page *page) { return false; } > -static inline bool putback_balloon_page(struct page *page) { return false; } > -#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */ > +static inline bool putback_balloon_page(struct page *page) { return false; } > +static inline bool balloon_compaction_enabled(void) { return false; } > +#endif /* CONFIG_COMPACTION */ > +#endif /* (CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE) */ > > #endif /* __KERNEL__ */ > #endif /* _LINUX_MM_H */ > diff --git a/mm/migrate.c b/mm/migrate.c > index 59c7bc5..f5f6a7d 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -78,7 +78,8 @@ 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)); > - if (unlikely(is_balloon_page(page))) > + if (unlikely(is_balloon_page(page)) && > + balloon_compaction_enabled()) well, that helps readability. But what does is_balloon_page() return when invoked on a balloon page when CONFIG_COMPACTION=n? False, methinks. I think the code as you previously had it was OK, but the is_balloon_page() name is misleading. It really wants to be called is_potentially_compactible_balloon_page() :( Maybe rename it to compactible_balloon_page()? -- 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>
next prev parent reply other threads:[~2012-07-19 1:25 UTC|newest] Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top 2012-07-17 16:50 [PATCH v4 0/3] make balloon pages movable by compaction Rafael Aquini 2012-07-17 16:50 ` Rafael Aquini 2012-07-17 16:50 ` [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages Rafael Aquini 2012-07-17 16:50 ` Rafael Aquini 2012-07-17 16:50 ` Rafael Aquini 2012-07-18 5:48 ` Minchan Kim 2012-07-18 5:48 ` Minchan Kim 2012-07-18 5:48 ` Minchan Kim 2012-07-20 19:48 ` Rafael Aquini 2012-07-20 19:48 ` Rafael Aquini 2012-07-20 19:48 ` Rafael Aquini 2012-07-23 2:33 ` Minchan Kim 2012-07-23 2:33 ` Minchan Kim 2012-07-23 2:33 ` Minchan Kim 2012-07-23 2:35 ` Minchan Kim 2012-07-23 2:35 ` Minchan Kim 2012-07-23 2:35 ` Minchan Kim 2012-07-23 18:19 ` Rafael Aquini 2012-07-23 18:19 ` Rafael Aquini 2012-07-23 18:19 ` Rafael Aquini 2012-08-01 20:53 ` Rik van Riel 2012-08-01 20:53 ` Rik van Riel 2012-08-03 11:13 ` Rafael Aquini 2012-08-03 11:13 ` Rafael Aquini 2012-08-03 11:13 ` Rafael Aquini 2012-08-01 20:53 ` Rik van Riel 2012-08-01 20:51 ` Rik van Riel 2012-08-01 20:51 ` Rik van Riel 2012-08-01 20:51 ` Rik van Riel 2012-08-03 12:26 ` Rafael Aquini 2012-08-03 12:26 ` Rafael Aquini 2012-08-03 12:26 ` Rafael Aquini 2012-07-18 22:46 ` Andrew Morton 2012-07-18 22:46 ` Andrew Morton 2012-07-18 22:46 ` Andrew Morton 2012-07-18 23:07 ` Rafael Aquini 2012-07-18 23:07 ` Rafael Aquini 2012-07-18 23:07 ` Rafael Aquini 2012-07-18 23:12 ` Andrew Morton 2012-07-18 23:12 ` Andrew Morton 2012-07-18 23:12 ` Andrew Morton 2012-07-19 1:00 ` Rafael Aquini 2012-07-19 1:00 ` Rafael Aquini 2012-07-19 1:00 ` Rafael Aquini 2012-07-19 1:29 ` Andrew Morton 2012-07-19 1:29 ` Andrew Morton [this message] 2012-07-19 1:29 ` Andrew Morton 2012-07-19 12:32 ` Rafael Aquini 2012-07-19 12:32 ` Rafael Aquini 2012-07-19 12:32 ` Rafael Aquini 2012-07-17 16:50 ` [PATCH v4 2/3] virtio_balloon: introduce migration primitives to balloon pages Rafael Aquini 2012-07-17 16:50 ` Rafael Aquini 2012-07-18 22:49 ` Andrew Morton 2012-07-18 22:49 ` Andrew Morton 2012-07-18 22:49 ` Andrew Morton 2012-07-18 23:16 ` Rafael Aquini 2012-07-18 23:16 ` Rafael Aquini 2012-07-18 23:16 ` Rafael Aquini 2012-07-17 16:50 ` Rafael Aquini 2012-07-17 16:50 ` [PATCH v4 3/3] mm: add vm event counters for balloon pages compaction Rafael Aquini 2012-07-17 16:50 ` Rafael Aquini 2012-07-17 16:50 ` Rafael Aquini
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20120718182944.24f59012.akpm@linux-foundation.org \ --to=akpm@linux-foundation.org \ --cc=andi@firstfloor.org \ --cc=aquini@linux.com \ --cc=aquini@redhat.com \ --cc=konrad.wilk@oracle.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mel@csn.ul.ie \ --cc=minchan@kernel.org \ --cc=mst@redhat.com \ --cc=riel@redhat.com \ --cc=rusty@rustcorp.com.au \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.