[PATCH] mm/pgtable: Move extern zero_pfn outside __HAVE_COLOR_ZERO_PAGE

[PATCH] mm/pgtable: Move extern zero_pfn outside __HAVE_COLOR_ZERO_PAGE

[Anshuman Khandual]

zero_pfn variable is required whether __HAVE_COLOR_ZERO_PAGE is enabled
or not. Also it should not really be declared individually in all functions
where it gets used. Just move the declaration outside, which also makes it
available for other potential users.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-arch@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
Applies on 5.8-rc1. If the earlier motivation was to hide zero_pfn from
general visibility, we could just put in a comment and update the commit
message that my_zero_pfn() should always be used rather than zero_pfn.
Build tested on many platforms and boot tested on arm64, x86.

 include/linux/pgtable.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index 32b6c52d41b9..078e9864abca 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -1020,10 +1020,11 @@ extern void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn,
 extern void untrack_pfn_moved(struct vm_area_struct *vma);
 #endif
 
+extern unsigned long zero_pfn;
+
 #ifdef __HAVE_COLOR_ZERO_PAGE
 static inline int is_zero_pfn(unsigned long pfn)
 {
-	extern unsigned long zero_pfn;
 	unsigned long offset_from_zero_pfn = pfn - zero_pfn;
 	return offset_from_zero_pfn <= (zero_page_mask >> PAGE_SHIFT);
 }
@@ -1033,13 +1034,11 @@ static inline int is_zero_pfn(unsigned long pfn)
 #else
 static inline int is_zero_pfn(unsigned long pfn)
 {
-	extern unsigned long zero_pfn;
 	return pfn == zero_pfn;
 }
 
 static inline unsigned long my_zero_pfn(unsigned long addr)
 {
-	extern unsigned long zero_pfn;
 	return zero_pfn;
 }
 #endif
-- 
2.20.1

Re: [PATCH] mm/pgtable: Move extern zero_pfn outside __HAVE_COLOR_ZERO_PAGE

[Anshuman Khandual]

On 06/16/2020 09:38 AM, Anshuman Khandual wrote:
> zero_pfn variable is required whether __HAVE_COLOR_ZERO_PAGE is enabled
> or not. Also it should not really be declared individually in all functions
> where it gets used. Just move the declaration outside, which also makes it
> available for other potential users.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-arch@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> Applies on 5.8-rc1. If the earlier motivation was to hide zero_pfn from
> general visibility, we could just put in a comment and update the commit
> message that my_zero_pfn() should always be used rather than zero_pfn.
> Build tested on many platforms and boot tested on arm64, x86.
> 
>  include/linux/pgtable.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index 32b6c52d41b9..078e9864abca 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -1020,10 +1020,11 @@ extern void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn,
>  extern void untrack_pfn_moved(struct vm_area_struct *vma);
>  #endif
>  
> +extern unsigned long zero_pfn;
> +
>  #ifdef __HAVE_COLOR_ZERO_PAGE
>  static inline int is_zero_pfn(unsigned long pfn)
>  {
> -	extern unsigned long zero_pfn;
>  	unsigned long offset_from_zero_pfn = pfn - zero_pfn;
>  	return offset_from_zero_pfn <= (zero_page_mask >> PAGE_SHIFT);
>  }
> @@ -1033,13 +1034,11 @@ static inline int is_zero_pfn(unsigned long pfn)
>  #else
>  static inline int is_zero_pfn(unsigned long pfn)
>  {
> -	extern unsigned long zero_pfn;
>  	return pfn == zero_pfn;
>  }
>  
>  static inline unsigned long my_zero_pfn(unsigned long addr)
>  {
> -	extern unsigned long zero_pfn;
>  	return zero_pfn;
>  }
>  #endif
> 

The CC list is incomplete. Adding Andrew, Mike and Kirill.

+Cc: Andrew Morton <akpm@linux-foundation.org>
+Cc: Mike Rapoport <rppt@linux.ibm.com>
+Cc: Kirill A . Shutemov <kirill.shutemov@linux.intel.com>

Will update the CC list next time around.

- Anshuman

Re: [PATCH] mm/pgtable: Move extern zero_pfn outside __HAVE_COLOR_ZERO_PAGE

[David Hildenbrand]

On 16.06.20 06:08, Anshuman Khandual wrote:
> zero_pfn variable is required whether __HAVE_COLOR_ZERO_PAGE is enabled

Why is that relevant for this patch?

> or not. Also it should not really be declared individually in all functions
> where it gets used. Just move the declaration outside, which also makes it
> available for other potential users.

So, all you're essentially doing is exposing zero_pfn in pgtable.h now.

If everybody should just use my_zero_pfn(), I don't really see the
benefit of this patch, sorry.

> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-arch@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> Applies on 5.8-rc1. If the earlier motivation was to hide zero_pfn from
> general visibility, we could just put in a comment and update the commit
> message that my_zero_pfn() should always be used rather than zero_pfn.
> Build tested on many platforms and boot tested on arm64, x86.
> 
>  include/linux/pgtable.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index 32b6c52d41b9..078e9864abca 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -1020,10 +1020,11 @@ extern void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn,
>  extern void untrack_pfn_moved(struct vm_area_struct *vma);
>  #endif
>  
> +extern unsigned long zero_pfn;
> +
>  #ifdef __HAVE_COLOR_ZERO_PAGE
>  static inline int is_zero_pfn(unsigned long pfn)
>  {
> -	extern unsigned long zero_pfn;
>  	unsigned long offset_from_zero_pfn = pfn - zero_pfn;
>  	return offset_from_zero_pfn <= (zero_page_mask >> PAGE_SHIFT);
>  }
> @@ -1033,13 +1034,11 @@ static inline int is_zero_pfn(unsigned long pfn)
>  #else
>  static inline int is_zero_pfn(unsigned long pfn)
>  {
> -	extern unsigned long zero_pfn;
>  	return pfn == zero_pfn;
>  }
>  
>  static inline unsigned long my_zero_pfn(unsigned long addr)
>  {
> -	extern unsigned long zero_pfn;
>  	return zero_pfn;
>  }
>  #endif
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH] mm/pgtable: Move extern zero_pfn outside __HAVE_COLOR_ZERO_PAGE

[Anshuman Khandual]

On 06/16/2020 01:09 PM, David Hildenbrand wrote:
> On 16.06.20 06:08, Anshuman Khandual wrote:
>> zero_pfn variable is required whether __HAVE_COLOR_ZERO_PAGE is enabled
> 
> Why is that relevant for this patch?

That just states how it is organized right now wrt __HAVE_COLOR_ZERO_PAGE.

> 
>> or not. Also it should not really be declared individually in all functions
>> where it gets used. Just move the declaration outside, which also makes it
>> available for other potential users.
> 
> So, all you're essentially doing is exposing zero_pfn in pgtable.h now.

Right, but it just happens in the process of consolidating three different
instances of 'extern unsigned long zero_pfn' in the same file which are
redundant.

> 
> If everybody should just use my_zero_pfn(), I don't really see the
> benefit of this patch, sorry.

It consolidates redundant declarations and reduces code. We could just have
a comment for zero_pfn stating that it should not be used directly.

Re: [PATCH] mm/pgtable: Move extern zero_pfn outside __HAVE_COLOR_ZERO_PAGE

[David Hildenbrand]

On 16.06.20 11:48, Anshuman Khandual wrote:
> 
> 
> On 06/16/2020 01:09 PM, David Hildenbrand wrote:
>> On 16.06.20 06:08, Anshuman Khandual wrote:
>>> zero_pfn variable is required whether __HAVE_COLOR_ZERO_PAGE is enabled
>>
>> Why is that relevant for this patch?
> 
> That just states how it is organized right now wrt __HAVE_COLOR_ZERO_PAGE.
> 
>>
>>> or not. Also it should not really be declared individually in all functions
>>> where it gets used. Just move the declaration outside, which also makes it
>>> available for other potential users.
>>
>> So, all you're essentially doing is exposing zero_pfn in pgtable.h now.
> 
> Right, but it just happens in the process of consolidating three different
> instances of 'extern unsigned long zero_pfn' in the same file which are
> redundant.
> 
>>
>> If everybody should just use my_zero_pfn(), I don't really see the
>> benefit of this patch, sorry.
> 
> It consolidates redundant declarations and reduces code. We could just have
> a comment for zero_pfn stating that it should not be used directly.
> 

... or just leave it as is and have self-documenting code.

-- 
Thanks,

David / dhildenb