Messages in this thread | | | Date | Thu, 20 Dec 2007 21:54:15 +0530 | From | Balbir Singh <> | Subject | Re: [PATCH] Move page_assign_page_cgroup to VM_BUG_ON in free_hot_cold_page |
| |
Peter Zijlstra wrote: > On Thu, 2007-12-20 at 14:16 +0000, Hugh Dickins wrote: >> 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. > > Ah, yes, I mistakenly assumed it was a reference get. In that case I > stand corrected and do not have any objections. >
I was going to say the same thing, page_get_page_cgroup() does not hold any references. May be _get_ in the name is confusing.
-- Warm Regards, Balbir Singh Linux Technology Center IBM, ISTL
| |