| Message ID | 20180823130732.9489-3-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | trivial code refine for sparsemem | expand |
[Cc Dave] On Thu 23-08-18 21:07:31, Wei Yang wrote: > When CONFIG_SPARSEMEM_EXTREME is not defined, mem_section is a static > two dimension array. This means !mem_section[SECTION_NR_TO_ROOT(nr)] is > always true. > > This patch expand the CONFIG_SPARSEMEM_EXTREME range to return a proper > mem_section when CONFIG_SPARSEMEM_EXTREME is not defined. As long as all callers provide a valid section number then yes. I am not really sure this is the case though. > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > --- > include/linux/mmzone.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h > index 32699b2dc52a..33086f86d1a7 100644 > --- a/include/linux/mmzone.h > +++ b/include/linux/mmzone.h > @@ -1155,9 +1155,9 @@ static inline struct mem_section *__nr_to_section(unsigned long nr) > #ifdef CONFIG_SPARSEMEM_EXTREME > if (!mem_section) > return NULL; > -#endif > if (!mem_section[SECTION_NR_TO_ROOT(nr)]) > return NULL; > +#endif > return &mem_section[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK]; > } > extern int __section_nr(struct mem_section* ms); > -- > 2.15.1 >
On Thu, Aug 23, 2018 at 03:21:12PM +0200, Michal Hocko wrote: >[Cc Dave] > >On Thu 23-08-18 21:07:31, Wei Yang wrote: >> When CONFIG_SPARSEMEM_EXTREME is not defined, mem_section is a static >> two dimension array. This means !mem_section[SECTION_NR_TO_ROOT(nr)] is >> always true. >> >> This patch expand the CONFIG_SPARSEMEM_EXTREME range to return a proper >> mem_section when CONFIG_SPARSEMEM_EXTREME is not defined. > >As long as all callers provide a valid section number then yes. I am not >really sure this is the case though. > I don't get your point. When CONFIG_SPARSEMEM_EXTREME is not defined, each section number is a valid one in this context. Because for eavry section number in [0, NR_MEM_SECTIONS - 1], we have a mem_sectioin structure there. This patch helps to reduce a meaningless check when CONFIG_SPARSEMEM_EXTREME=n. >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >> --- >> include/linux/mmzone.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h >> index 32699b2dc52a..33086f86d1a7 100644 >> --- a/include/linux/mmzone.h >> +++ b/include/linux/mmzone.h >> @@ -1155,9 +1155,9 @@ static inline struct mem_section *__nr_to_section(unsigned long nr) >> #ifdef CONFIG_SPARSEMEM_EXTREME >> if (!mem_section) >> return NULL; >> -#endif >> if (!mem_section[SECTION_NR_TO_ROOT(nr)]) >> return NULL; >> +#endif >> return &mem_section[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK]; >> } >> extern int __section_nr(struct mem_section* ms); >> -- >> 2.15.1 >> > >-- >Michal Hocko >SUSE Labs
On 08/23/2018 06:21 AM, Michal Hocko wrote: > --- a/include/linux/mmzone.h > +++ b/include/linux/mmzone.h > @@ -1155,9 +1155,9 @@ static inline struct mem_section *__nr_to_section(unsigned long nr) > #ifdef CONFIG_SPARSEMEM_EXTREME > if (!mem_section) > return NULL; > -#endif > if (!mem_section[SECTION_NR_TO_ROOT(nr)]) > return NULL; > +#endif > return &mem_section[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK]; > } This patch has no practical effect and only adds unnecessary churn. #ifdef CONFIG_SPARSEMEM_EXTREME ... #else struct mem_section mem_section[NR_SECTION_ROOTS][SECTIONS_PER_ROOT]; #endif The compiler knows that NR_SECTION_ROOTS==1 and that !mem_section[SECTION_NR_TO_ROOT(nr) is always false. It doesn't need our help. My goal with the sparsemem code, and code in general is t avoid #ifdefs whenever possible and limit their scope to the smallest possible area whenever possible.
On Thu, Aug 23, 2018 at 05:09:12PM -0700, Dave Hansen wrote: >On 08/23/2018 06:21 AM, Michal Hocko wrote: >> --- a/include/linux/mmzone.h >> +++ b/include/linux/mmzone.h >> @@ -1155,9 +1155,9 @@ static inline struct mem_section *__nr_to_section(unsigned long nr) >> #ifdef CONFIG_SPARSEMEM_EXTREME >> if (!mem_section) >> return NULL; >> -#endif >> if (!mem_section[SECTION_NR_TO_ROOT(nr)]) >> return NULL; >> +#endif >> return &mem_section[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK]; >> } > >This patch has no practical effect and only adds unnecessary churn. > >#ifdef CONFIG_SPARSEMEM_EXTREME >... >#else >struct mem_section mem_section[NR_SECTION_ROOTS][SECTIONS_PER_ROOT]; >#endif > >The compiler knows that NR_SECTION_ROOTS==1 and that >!mem_section[SECTION_NR_TO_ROOT(nr) is always false. It doesn't need >our help. > I didn't know the compile would optimize the code when this is a one dimension array. Just wrote a code and their assembly looks the same. Thanks for pointing out. >My goal with the sparsemem code, and code in general is t avoid #ifdefs >whenever possible and limit their scope to the smallest possible area >whenever possible.
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h index 32699b2dc52a..33086f86d1a7 100644 --- a/include/linux/mmzone.h +++ b/include/linux/mmzone.h @@ -1155,9 +1155,9 @@ static inline struct mem_section *__nr_to_section(unsigned long nr) #ifdef CONFIG_SPARSEMEM_EXTREME if (!mem_section) return NULL; -#endif if (!mem_section[SECTION_NR_TO_ROOT(nr)]) return NULL; +#endif return &mem_section[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK]; } extern int __section_nr(struct mem_section* ms);
When CONFIG_SPARSEMEM_EXTREME is not defined, mem_section is a static two dimension array. This means !mem_section[SECTION_NR_TO_ROOT(nr)] is always true. This patch expand the CONFIG_SPARSEMEM_EXTREME range to return a proper mem_section when CONFIG_SPARSEMEM_EXTREME is not defined. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> --- include/linux/mmzone.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)