On Thu, 20 Dec 2007, Peter Zijlstra wrote: > On Thu, 2007-12-20 at 13:14 +0000, Hugh Dickins wrote: > > On Wed, 19 Dec 2007, Dave Hansen wrote: > > > > - page_assign_page_cgroup(page, NULL); > > > > + VM_BUG_ON(page_get_page_cgroup(page)); > > > > > > Hi Balbir, > > > > > > You generally want to do these like: > > > > > > foo = page_assign_page_cgroup(page, NULL); > > > VM_BUG_ON(foo); > > > > > > Some embedded people have been known to optimize kernel size like this: > > > > > > #define VM_BUG_ON(x) do{}while(0) > > > > Balbir's patch looks fine to me: I don't get your point there, Dave. > > There was a lengthy discussion here: > http://lkml.org/lkml/2007/12/14/131 > > on the merit of debug statements with side effects.
Of course, but what's the relevance? > But looking at our definition: > > #ifdef CONFIG_DEBUG_VM > #define VM_BUG_ON(cond) BUG_ON(cond) > #else > #define VM_BUG_ON(condition) do { } while(0) > #endif > > disabling CONFIG_DEBUG_VM breaks the code as proposed by Balbir in that > it will no longer acquire the reference. But what reference? struct page_cgroup *page_get_page_cgroup(struct page *page) { return (struct page_cgroup *) (page->page_cgroup & ~PAGE_CGROUP_LOCK); } I guess the issue is that often a "get" function has a complementary "put" function, but this isn't one of them. Would page_page_cgroup be a better name, perhaps? I don't know. Hugh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/