Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1442560 > unrolled thread

Re: [PATCH 4/4] x86: use pte_none() to test for empty PTE

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 4/4] x86: use pte_none() to test for empty PTE Michal Hocko <mhocko@kernel.org> - 2016-07-13 17:40 +0200
    Re: [PATCH 4/4] x86: use pte_none() to test for empty PTE Julia Lawall <julia.lawall@lip6.fr> - 2016-07-13 18:00 +0200
      Re: [PATCH 4/4] x86: use pte_none() to test for empty PTE Dave Hansen <dave.hansen@intel.com> - 2016-07-13 18:40 +0200

#1442560 — Re: [PATCH 4/4] x86: use pte_none() to test for empty PTE

[CCing Julia]

On Thu 07-07-16 17:19:15, Dave Hansen wrote:
> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> The page table manipulation code seems to have grown a couple of
> sites that are looking for empty PTEs.  Just in case one of these
> entries got a stray bit set, use pte_none() instead of checking
> for a zero pte_val().

This looks like something that coccinelle could help with and automate.
Especially when the patch seems interesting for applying to older kernel
code streams.

Julia would it be hard to generate a metapatch which would check the
{pte,pmd}_val() usage in conditions and replace them with {pte,pmd}_none
equivalents?

> The use pte_same() makes me a bit nervous.  If we were doing a
> pte_same() check against two cleared entries and one of them had
> a stray bit set, it might fail the pte_same() check.  But, I
> don't think we ever _do_ pte_same() for cleared entries.  It is
> almost entirely used for checking for races in fault-in paths.
> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>

Other than that looks good to me. Feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
> 
>  b/arch/x86/mm/init_64.c    |   12 ++++++------
>  b/arch/x86/mm/pageattr.c   |    2 +-
>  b/arch/x86/mm/pgtable_32.c |    2 +-
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff -puN arch/x86/mm/init_64.c~knl-strays-50-pte_val-cleanups arch/x86/mm/init_64.c
> --- a/arch/x86/mm/init_64.c~knl-strays-50-pte_val-cleanups	2016-07-07 17:17:44.942808493 -0700
> +++ b/arch/x86/mm/init_64.c	2016-07-07 17:17:44.949808807 -0700
> @@ -354,7 +354,7 @@ phys_pte_init(pte_t *pte_page, unsigned
>  		 * pagetable pages as RO. So assume someone who pre-setup
>  		 * these mappings are more intelligent.
>  		 */
> -		if (pte_val(*pte)) {
> +		if (!pte_none(*pte)) {
>  			if (!after_bootmem)
>  				pages++;
>  			continue;
> @@ -396,7 +396,7 @@ phys_pmd_init(pmd_t *pmd_page, unsigned
>  			continue;
>  		}
>  
> -		if (pmd_val(*pmd)) {
> +		if (!pmd_none(*pmd)) {
>  			if (!pmd_large(*pmd)) {
>  				spin_lock(&init_mm.page_table_lock);
>  				pte = (pte_t *)pmd_page_vaddr(*pmd);
> @@ -470,7 +470,7 @@ phys_pud_init(pud_t *pud_page, unsigned
>  			continue;
>  		}
>  
> -		if (pud_val(*pud)) {
> +		if (!pud_none(*pud)) {
>  			if (!pud_large(*pud)) {
>  				pmd = pmd_offset(pud, 0);
>  				last_map_addr = phys_pmd_init(pmd, addr, end,
> @@ -673,7 +673,7 @@ static void __meminit free_pte_table(pte
>  
>  	for (i = 0; i < PTRS_PER_PTE; i++) {
>  		pte = pte_start + i;
> -		if (pte_val(*pte))
> +		if (!pte_none(*pte))
>  			return;
>  	}
>  
> @@ -691,7 +691,7 @@ static void __meminit free_pmd_table(pmd
>  
>  	for (i = 0; i < PTRS_PER_PMD; i++) {
>  		pmd = pmd_start + i;
> -		if (pmd_val(*pmd))
> +		if (!pmd_none(*pmd))
>  			return;
>  	}
>  
> @@ -710,7 +710,7 @@ static bool __meminit free_pud_table(pud
>  
>  	for (i = 0; i < PTRS_PER_PUD; i++) {
>  		pud = pud_start + i;
> -		if (pud_val(*pud))
> +		if (!pud_none(*pud))
>  			return false;
>  	}
>  
> diff -puN arch/x86/mm/pageattr.c~knl-strays-50-pte_val-cleanups arch/x86/mm/pageattr.c
> --- a/arch/x86/mm/pageattr.c~knl-strays-50-pte_val-cleanups	2016-07-07 17:17:44.944808582 -0700
> +++ b/arch/x86/mm/pageattr.c	2016-07-07 17:17:44.950808852 -0700
> @@ -1185,7 +1185,7 @@ repeat:
>  		return __cpa_process_fault(cpa, address, primary);
>  
>  	old_pte = *kpte;
> -	if (!pte_val(old_pte))
> +	if (pte_none(old_pte))
>  		return __cpa_process_fault(cpa, address, primary);
>  
>  	if (level == PG_LEVEL_4K) {
> diff -puN arch/x86/mm/pgtable_32.c~knl-strays-50-pte_val-cleanups arch/x86/mm/pgtable_32.c
> --- a/arch/x86/mm/pgtable_32.c~knl-strays-50-pte_val-cleanups	2016-07-07 17:17:44.946808672 -0700
> +++ b/arch/x86/mm/pgtable_32.c	2016-07-07 17:17:44.950808852 -0700
> @@ -47,7 +47,7 @@ void set_pte_vaddr(unsigned long vaddr,
>  		return;
>  	}
>  	pte = pte_offset_kernel(pmd, vaddr);
> -	if (pte_val(pteval))
> +	if (!pte_none(pteval))
>  		set_pte_at(&init_mm, vaddr, pte, pteval);
>  	else
>  		pte_clear(&init_mm, vaddr, pte);
> _

-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1442569

My results are below.  There are a couple of cases in arch/mn10300/mm that
were not in the original patch.

julia

diff -u -p a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -1185,7 +1185,7 @@ repeat:
 		return __cpa_process_fault(cpa, address, primary);

 	old_pte = *kpte;
-	if (!pte_val(old_pte))
+	if (pte_none(old_pte))
 		return __cpa_process_fault(cpa, address, primary);

 	if (level == PG_LEVEL_4K) {
diff -u -p a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -354,7 +354,7 @@ phys_pte_init(pte_t *pte_page, unsigned
 		 * pagetable pages as RO. So assume someone who pre-setup
 		 * these mappings are more intelligent.
 		 */
-		if (pte_val(*pte)) {
+		if (!pte_none(*pte)) {
 			if (!after_bootmem)
 				pages++;
 			continue;
@@ -396,7 +396,7 @@ phys_pmd_init(pmd_t *pmd_page, unsigned
 			continue;
 		}

-		if (pmd_val(*pmd)) {
+		if (!pmd_none(*pmd)) {
 			if (!pmd_large(*pmd)) {
 				spin_lock(&init_mm.page_table_lock);
 				pte = (pte_t *)pmd_page_vaddr(*pmd);
@@ -470,7 +470,7 @@ phys_pud_init(pud_t *pud_page, unsigned
 			continue;
 		}

-		if (pud_val(*pud)) {
+		if (!pud_none(*pud)) {
 			if (!pud_large(*pud)) {
 				pmd = pmd_offset(pud, 0);
 				last_map_addr = phys_pmd_init(pmd, addr, end,
@@ -673,7 +673,7 @@ static void __meminit free_pte_table(pte

 	for (i = 0; i < PTRS_PER_PTE; i++) {
 		pte = pte_start + i;
-		if (pte_val(*pte))
+		if (!pte_none(*pte))
 			return;
 	}

@@ -691,7 +691,7 @@ static void __meminit free_pmd_table(pmd

 	for (i = 0; i < PTRS_PER_PMD; i++) {
 		pmd = pmd_start + i;
-		if (pmd_val(*pmd))
+		if (!pmd_none(*pmd))
 			return;
 	}

@@ -710,7 +710,7 @@ static bool __meminit free_pud_table(pud

 	for (i = 0; i < PTRS_PER_PUD; i++) {
 		pud = pud_start + i;
-		if (pud_val(*pud))
+		if (!pud_none(*pud))
 			return false;
 	}

diff -u -p a/arch/x86/mm/pgtable_32.c b/arch/x86/mm/pgtable_32.c
--- a/arch/x86/mm/pgtable_32.c
+++ b/arch/x86/mm/pgtable_32.c
@@ -47,7 +47,7 @@ void set_pte_vaddr(unsigned long vaddr,
 		return;
 	}
 	pte = pte_offset_kernel(pmd, vaddr);
-	if (pte_val(pteval))
+	if (!pte_none(pteval))
 		set_pte_at(&init_mm, vaddr, pte, pteval);
 	else
 		pte_clear(&init_mm, vaddr, pte);
diff -u -p a/arch/mn10300/mm/cache-flush-icache.c b/arch/mn10300/mm/cache-flush-icache.c
--- a/arch/mn10300/mm/cache-flush-icache.c
+++ b/arch/mn10300/mm/cache-flush-icache.c
@@ -67,11 +67,11 @@ static void flush_icache_page_range(unsi
 		return;

 	pud = pud_offset(pgd, start);
-	if (!pud || !pud_val(*pud))
+	if (!pud || pud_none(*pud))
 		return;

 	pmd = pmd_offset(pud, start);
-	if (!pmd || !pmd_val(*pmd))
+	if (!pmd || pmd_none(*pmd))
 		return;

 	ppte = pte_offset_map(pmd, start);
diff -u -p a/arch/mn10300/mm/cache-inv-icache.c b/arch/mn10300/mm/cache-inv-icache.c
--- a/arch/mn10300/mm/cache-inv-icache.c
+++ b/arch/mn10300/mm/cache-inv-icache.c
@@ -45,11 +45,11 @@ static void flush_icache_page_range(unsi
 		return;

 	pud = pud_offset(pgd, start);
-	if (!pud || !pud_val(*pud))
+	if (!pud || pud_none(*pud))
 		return;

 	pmd = pmd_offset(pud, start);
-	if (!pmd || !pmd_val(*pmd))
+	if (!pmd || pmd_none(*pmd))
 		return;

 	ppte = pte_offset_map(pmd, start);

[toc] | [prev] | [next] | [standalone]

#1442624

On 07/13/2016 08:49 AM, Julia Lawall wrote:
> My results are below.  There are a couple of cases in arch/mn10300/mm that
> were not in the original patch.

Yeah, so mn10300 is obviously unaffected by the erratum in question, and
I didn't look for non-x86 architectures for this patch.

But, this code definitely _looks_ like it should be using pte_none(),
especially since mn10300 defines it the same way as x86 (well, as x86
_did_ before this series).

	#define pte_none(x)		(!pte_val(x))

> diff -u -p a/arch/mn10300/mm/cache-inv-icache.c b/arch/mn10300/mm/cache-inv-icache.c
> --- a/arch/mn10300/mm/cache-inv-icache.c
> +++ b/arch/mn10300/mm/cache-inv-icache.c
> @@ -45,11 +45,11 @@ static void flush_icache_page_range(unsi
>  		return;
> 
>  	pud = pud_offset(pgd, start);
> -	if (!pud || !pud_val(*pud))
> +	if (!pud || pud_none(*pud))
>  		return;
> 
>  	pmd = pmd_offset(pud, start);
> -	if (!pmd || !pmd_val(*pmd))
> +	if (!pmd || pmd_none(*pmd))
>  		return;

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web