diff mbox series

[2/3] mm/sparse: expand the CONFIG_SPARSEMEM_EXTREME range in __nr_to_section()

Message ID 20180823130732.9489-3-richard.weiyang@gmail.com (mailing list archive)
State New, archived
Headers show
Series trivial code refine for sparsemem | expand

Commit Message

Wei Yang Aug. 23, 2018, 1:07 p.m. UTC
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(-)

Comments

Michal Hocko Aug. 23, 2018, 1:21 p.m. UTC | #1
[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
>
Wei Yang Aug. 23, 2018, 11:03 p.m. UTC | #2
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
Dave Hansen Aug. 24, 2018, 12:09 a.m. UTC | #3
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.
Wei Yang Aug. 24, 2018, 3:24 p.m. UTC | #4
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 mbox series

Patch

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);