From: Andrew Morton <akpm@linux-foundation.org> To: Wu Fengguang <fengguang.wu@intel.com> Cc: linux-kernel@vger.kernel.org, kosaki.motohiro@jp.fujitsu.com, andi@firstfloor.org, mpm@selenic.com, adobriyan@gmail.com, fengguang.wu@intel.com, linux-mm@kvack.org Subject: Re: [PATCH 5/5] proc: export more page flags in /proc/kpageflags Date: Tue, 28 Apr 2009 14:32:44 -0700 [thread overview] Message-ID: <20090428143244.4e424d36.akpm@linux-foundation.org> (raw) In-Reply-To: <20090428014920.769723618@intel.com> On Tue, 28 Apr 2009 09:09:12 +0800 Wu Fengguang <fengguang.wu@intel.com> wrote: > +/* > + * Kernel flags are exported faithfully to Linus and his fellow hackers. > + * Otherwise some details are masked to avoid confusing the end user: > + * - some kernel flags are completely invisible > + * - some kernel flags are conditionally invisible on their odd usages > + */ > +#ifdef CONFIG_DEBUG_KERNEL > +static inline int genuine_linus(void) { return 1; } Although he's a fine chap, the use of the "_linus" tag isn't terribly clear (to me). I think what you're saying here is that this enables kernel-developer-only features, yes? If so, perhaps we could come up with an identifier which expresses that more clearly. But I'd expect that everyone and all distros enable CONFIG_DEBUG_KERNEL for _some_ reason, so what's the point? It is preferable that we always implement the same interface for all Kconfig settings. If this exposes information which is confusing or not useful to end-users then so be it - we should be able to cover that in supporting documentation. Also, as mentioned in the other email, it would be good if we were to publish a little userspace app which people can use to access this raw data. We could give that application an `--i-am-a-kernel-developer' option! > +#else > +static inline int genuine_linus(void) { return 0; } > +#endif This isn't an appropriate use of CONFIG_DEBUG_KERNEL. DEBUG_KERNEL is a Kconfig-only construct which is use to enable _other_ debugging features. The way you've used it here, if the person who is configuring the kernel wants to enable any other completely-unrelated debug feature, they have to enable DEBUG_KERNEL first. But when they do that, they unexpectedly alter the behaviour of pagemap! There are two other places where CONFIG_DEBUG_KERNEL affects code generation in .c files: arch/parisc/mm/init.c and arch/powerpc/kernel/sysfs.c. These are both wrong, and need slapping ;) > +#define kpf_copy_bit(uflags, kflags, visible, ubit, kbit) \ > + do { \ > + if (visible || genuine_linus()) \ > + uflags |= ((kflags >> kbit) & 1) << ubit; \ > + } while (0); Did this have to be implemented as a macro? It's bad, because it might or might not reference its argument, so if someone passes it an expression-with-side-effects, the end result is unpredictable. A C function is almost always preferable if possible. > +/* a helper function _not_ intended for more general uses */ > +static inline int page_cap_writeback_dirty(struct page *page) > +{ > + struct address_space *mapping; > + > + if (!PageSlab(page)) > + mapping = page_mapping(page); > + else > + mapping = NULL; > + > + return mapping && mapping_cap_writeback_dirty(mapping); > +} If the page isn't locked then page->mapping can be concurrently removed and freed. This actually happened to me in real-life testing several years ago.
WARNING: multiple messages have this Message-ID (diff)
From: Andrew Morton <akpm@linux-foundation.org> To: Wu Fengguang <fengguang.wu@intel.com> Cc: linux-kernel@vger.kernel.org, kosaki.motohiro@jp.fujitsu.com, andi@firstfloor.org, mpm@selenic.com, adobriyan@gmail.com, linux-mm@kvack.org Subject: Re: [PATCH 5/5] proc: export more page flags in /proc/kpageflags Date: Tue, 28 Apr 2009 14:32:44 -0700 [thread overview] Message-ID: <20090428143244.4e424d36.akpm@linux-foundation.org> (raw) In-Reply-To: <20090428014920.769723618@intel.com> On Tue, 28 Apr 2009 09:09:12 +0800 Wu Fengguang <fengguang.wu@intel.com> wrote: > +/* > + * Kernel flags are exported faithfully to Linus and his fellow hackers. > + * Otherwise some details are masked to avoid confusing the end user: > + * - some kernel flags are completely invisible > + * - some kernel flags are conditionally invisible on their odd usages > + */ > +#ifdef CONFIG_DEBUG_KERNEL > +static inline int genuine_linus(void) { return 1; } Although he's a fine chap, the use of the "_linus" tag isn't terribly clear (to me). I think what you're saying here is that this enables kernel-developer-only features, yes? If so, perhaps we could come up with an identifier which expresses that more clearly. But I'd expect that everyone and all distros enable CONFIG_DEBUG_KERNEL for _some_ reason, so what's the point? It is preferable that we always implement the same interface for all Kconfig settings. If this exposes information which is confusing or not useful to end-users then so be it - we should be able to cover that in supporting documentation. Also, as mentioned in the other email, it would be good if we were to publish a little userspace app which people can use to access this raw data. We could give that application an `--i-am-a-kernel-developer' option! > +#else > +static inline int genuine_linus(void) { return 0; } > +#endif This isn't an appropriate use of CONFIG_DEBUG_KERNEL. DEBUG_KERNEL is a Kconfig-only construct which is use to enable _other_ debugging features. The way you've used it here, if the person who is configuring the kernel wants to enable any other completely-unrelated debug feature, they have to enable DEBUG_KERNEL first. But when they do that, they unexpectedly alter the behaviour of pagemap! There are two other places where CONFIG_DEBUG_KERNEL affects code generation in .c files: arch/parisc/mm/init.c and arch/powerpc/kernel/sysfs.c. These are both wrong, and need slapping ;) > +#define kpf_copy_bit(uflags, kflags, visible, ubit, kbit) \ > + do { \ > + if (visible || genuine_linus()) \ > + uflags |= ((kflags >> kbit) & 1) << ubit; \ > + } while (0); Did this have to be implemented as a macro? It's bad, because it might or might not reference its argument, so if someone passes it an expression-with-side-effects, the end result is unpredictable. A C function is almost always preferable if possible. > +/* a helper function _not_ intended for more general uses */ > +static inline int page_cap_writeback_dirty(struct page *page) > +{ > + struct address_space *mapping; > + > + if (!PageSlab(page)) > + mapping = page_mapping(page); > + else > + mapping = NULL; > + > + return mapping && mapping_cap_writeback_dirty(mapping); > +} If the page isn't locked then page->mapping can be concurrently removed and freed. This actually happened to me in real-life testing several years ago. -- 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:[~2009-04-28 21:38 UTC|newest] Thread overview: 137+ messages / expand[flat|nested] mbox.gz Atom feed top 2009-04-28 1:09 [PATCH 0/5] proc: export more page flags in /proc/kpageflags (take 4) Wu Fengguang 2009-04-28 1:09 ` Wu Fengguang 2009-04-28 1:09 ` [PATCH 1/5] pagemap: document clarifications Wu Fengguang 2009-04-28 1:09 ` Wu Fengguang 2009-04-28 7:11 ` Tommi Rantala 2009-04-28 7:11 ` Tommi Rantala 2009-04-28 1:09 ` [PATCH 2/5] pagemap: documentation 9 more exported page flags Wu Fengguang 2009-04-28 1:09 ` Wu Fengguang 2009-04-28 1:09 ` [PATCH 3/5] mm: introduce PageHuge() for testing huge/gigantic pages Wu Fengguang 2009-04-28 1:09 ` Wu Fengguang 2009-04-28 1:09 ` [PATCH 4/5] proc: kpagecount/kpageflags code cleanup Wu Fengguang 2009-04-28 1:09 ` Wu Fengguang 2009-04-28 1:09 ` [PATCH 5/5] proc: export more page flags in /proc/kpageflags Wu Fengguang 2009-04-28 1:09 ` Wu Fengguang 2009-04-28 6:55 ` Ingo Molnar 2009-04-28 6:55 ` Ingo Molnar 2009-04-28 7:40 ` Andi Kleen 2009-04-28 7:40 ` Andi Kleen 2009-04-28 9:04 ` Pekka Enberg 2009-04-28 9:04 ` Pekka Enberg 2009-04-28 9:10 ` Andi Kleen 2009-04-28 9:10 ` Andi Kleen 2009-04-28 9:15 ` Pekka Enberg 2009-04-28 9:15 ` Pekka Enberg 2009-04-28 9:15 ` Ingo Molnar 2009-04-28 9:15 ` Ingo Molnar 2009-04-28 9:19 ` Pekka Enberg 2009-04-28 9:19 ` Pekka Enberg 2009-04-28 9:25 ` Pekka Enberg 2009-04-28 9:25 ` Pekka Enberg 2009-04-28 9:36 ` Wu Fengguang 2009-04-28 9:36 ` Wu Fengguang 2009-04-28 9:36 ` Ingo Molnar 2009-04-28 9:36 ` Ingo Molnar 2009-04-28 9:57 ` Pekka Enberg 2009-04-28 9:57 ` Pekka Enberg 2009-04-28 10:10 ` KOSAKI Motohiro 2009-04-28 10:10 ` KOSAKI Motohiro 2009-04-28 10:21 ` Pekka Enberg 2009-04-28 10:21 ` Pekka Enberg 2009-04-28 10:56 ` Ingo Molnar 2009-04-28 10:56 ` Ingo Molnar 2009-04-28 11:09 ` KOSAKI Motohiro 2009-04-28 11:09 ` KOSAKI Motohiro 2009-04-28 12:42 ` Ingo Molnar 2009-04-28 12:42 ` Ingo Molnar 2009-04-28 11:03 ` Ingo Molnar 2009-04-28 11:03 ` Ingo Molnar 2009-04-28 17:42 ` Matt Mackall 2009-04-28 17:42 ` Matt Mackall 2009-04-28 9:29 ` Ingo Molnar 2009-04-28 9:29 ` Ingo Molnar 2009-04-28 9:34 ` KOSAKI Motohiro 2009-04-28 9:34 ` KOSAKI Motohiro 2009-04-28 9:38 ` Ingo Molnar 2009-04-28 9:38 ` Ingo Molnar 2009-04-28 9:55 ` Wu Fengguang 2009-04-28 9:55 ` Wu Fengguang 2009-04-28 10:11 ` KOSAKI Motohiro 2009-04-28 10:11 ` KOSAKI Motohiro 2009-04-28 11:05 ` Ingo Molnar 2009-04-28 11:05 ` Ingo Molnar 2009-04-28 11:36 ` Wu Fengguang 2009-04-28 11:36 ` Wu Fengguang 2009-04-28 12:17 ` [rfc] object collection tracing (was: [PATCH 5/5] proc: export more page flags in /proc/kpageflags) Ingo Molnar 2009-04-28 12:17 ` Ingo Molnar 2009-04-28 13:31 ` Wu Fengguang 2009-04-28 13:31 ` Wu Fengguang 2009-05-12 13:01 ` Frederic Weisbecker 2009-05-12 13:01 ` Frederic Weisbecker 2009-05-17 13:36 ` Wu Fengguang 2009-05-17 13:55 ` Frederic Weisbecker 2009-05-17 13:55 ` Frederic Weisbecker 2009-05-17 14:12 ` Wu Fengguang 2009-05-17 14:12 ` Wu Fengguang 2009-05-18 11:44 ` KOSAKI Motohiro 2009-05-18 11:44 ` KOSAKI Motohiro 2009-05-18 11:47 ` Wu Fengguang 2009-05-18 11:47 ` Wu Fengguang 2009-04-28 10:18 ` [PATCH 5/5] proc: export more page flags in /proc/kpageflags Andi Kleen 2009-04-28 10:18 ` Andi Kleen 2009-04-28 8:33 ` Wu Fengguang 2009-04-28 8:33 ` Wu Fengguang 2009-04-28 9:24 ` Ingo Molnar 2009-04-28 9:24 ` Ingo Molnar 2009-04-28 18:11 ` Tony Luck 2009-04-28 18:11 ` Tony Luck 2009-04-28 18:34 ` Matt Mackall 2009-04-28 18:34 ` Matt Mackall 2009-04-28 20:47 ` Tony Luck 2009-04-28 20:47 ` Tony Luck 2009-04-28 20:54 ` Andi Kleen 2009-04-28 20:54 ` Andi Kleen 2009-04-28 20:59 ` Matt Mackall 2009-04-28 20:59 ` Matt Mackall 2009-04-28 21:17 ` Andrew Morton 2009-04-28 21:17 ` Andrew Morton 2009-04-28 21:49 ` Matt Mackall 2009-04-28 21:49 ` Matt Mackall 2009-04-29 0:02 ` Robin Holt 2009-04-29 0:02 ` Robin Holt 2009-04-28 17:49 ` Matt Mackall 2009-04-28 17:49 ` Matt Mackall 2009-04-29 8:05 ` Wu Fengguang 2009-04-29 8:05 ` Wu Fengguang 2009-04-29 19:13 ` Matt Mackall 2009-04-29 19:13 ` Matt Mackall 2009-04-30 1:00 ` Wu Fengguang 2009-04-30 1:00 ` Wu Fengguang 2009-04-28 21:32 ` Andrew Morton [this message] 2009-04-28 21:32 ` Andrew Morton 2009-04-28 22:46 ` Matt Mackall 2009-04-28 22:46 ` Matt Mackall 2009-04-28 23:02 ` Andrew Morton 2009-04-28 23:02 ` Andrew Morton 2009-04-28 23:31 ` Matt Mackall 2009-04-28 23:31 ` Matt Mackall 2009-04-28 23:42 ` Andrew Morton 2009-04-28 23:42 ` Andrew Morton 2009-04-28 23:55 ` Matt Mackall 2009-04-28 23:55 ` Matt Mackall 2009-04-29 3:33 ` Wu Fengguang 2009-04-29 3:33 ` Wu Fengguang 2009-04-29 2:38 ` Wu Fengguang 2009-04-29 2:38 ` Wu Fengguang 2009-04-29 2:55 ` Andrew Morton 2009-04-29 2:55 ` Andrew Morton 2009-04-29 3:48 ` Wu Fengguang 2009-04-29 3:48 ` Wu Fengguang 2009-04-29 5:09 ` Wu Fengguang 2009-04-29 5:09 ` Wu Fengguang 2009-04-29 4:41 ` Nathan Lynch 2009-04-29 4:41 ` Nathan Lynch 2009-04-29 4:41 ` Nathan Lynch 2009-04-29 4:50 ` Andrew Morton 2009-04-29 4:50 ` Andrew Morton 2009-04-29 4:50 ` Andrew Morton
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=20090428143244.4e424d36.akpm@linux-foundation.org \ --to=akpm@linux-foundation.org \ --cc=adobriyan@gmail.com \ --cc=andi@firstfloor.org \ --cc=fengguang.wu@intel.com \ --cc=kosaki.motohiro@jp.fujitsu.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mpm@selenic.com \ /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.