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

Groups > linux.kernel > #1295546 > unrolled thread

Re: [PATCH 1/2] x86/mm/pat: Change untrack_pfn() to handle unmapped vma

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 1/2] x86/mm/pat: Change untrack_pfn() to handle unmapped  vma Thomas Gleixner <tglx@linutronix.de> - 2015-12-20 10:30 +0100
    Re: [PATCH 1/2] x86/mm/pat: Change untrack_pfn() to handle unmapped  vma Toshi Kani <toshi.kani@hpe.com> - 2015-12-22 18:10 +0100

#1295546 — Re: [PATCH 1/2] x86/mm/pat: Change untrack_pfn() to handle unmapped vma

Toshi,

On Wed, 9 Dec 2015, Toshi Kani wrote:
> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> index 188e3e0..f3e391e 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -966,8 +966,14 @@ int track_pfn_insert(struct vm_area_struct *vma, pgprot_t *prot,
>  
>  /*
>   * untrack_pfn is called while unmapping a pfnmap for a region.
> - * untrack can be called for a specific region indicated by pfn and size or
> - * can be for the entire vma (in which case pfn, size are zero).
> + * untrack_pfn can be called for a specific region indicated by pfn and
> + * size or can be for the entire vma (in which case pfn, size are zero).
> + *
> + * NOTE: mremap may move a virtual address of VM_PFNMAP, but keeps the
> + * pfn and cache type.  In this case, untrack_pfn() is called with the
> + * old vma after its translation has removed.  Hence, when follow_phys()
> + * fails, track_pfn() keeps the pfn tracked and clears VM_PAT from the
> + * old vma.
>   */
>  void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn,
>  		 unsigned long size)
> @@ -981,14 +987,13 @@ void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn,
>  	/* free the chunk starting from pfn or the whole chunk */
>  	paddr = (resource_size_t)pfn << PAGE_SHIFT;
>  	if (!paddr && !size) {
> -		if (follow_phys(vma, vma->vm_start, 0, &prot, &paddr)) {
> -			WARN_ON_ONCE(1);
> -			return;
> -		}
> +		if (follow_phys(vma, vma->vm_start, 0, &prot, &paddr))
> +			goto out;

Shouldn't we have an explicit call in the mremap code which clears the
PAT flag on the mm instead of removing this sanity check?
  
Because that's what we end up with there. We just clear the PAT flag.

I rather prefer to do that explicitely, so the following call to
untrack_pfn() from move_vma()->do_munmap() ... will see the PAT flag
cleared. untrack_moved_pfn() or such.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1296899

On Sun, 2015-12-20 at 10:21 +0100, Thomas Gleixner wrote:
> Toshi,
> 
> On Wed, 9 Dec 2015, Toshi Kani wrote:
> > diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> > index 188e3e0..f3e391e 100644
> > --- a/arch/x86/mm/pat.c
> > +++ b/arch/x86/mm/pat.c
> > @@ -966,8 +966,14 @@ int track_pfn_insert(struct vm_area_struct *vma,
> > pgprot_t *prot,
> >  
> >  /*
> >   * untrack_pfn is called while unmapping a pfnmap for a region.
> > - * untrack can be called for a specific region indicated by pfn and
> > size or
> > - * can be for the entire vma (in which case pfn, size are zero).
> > + * untrack_pfn can be called for a specific region indicated by pfn
> > and
> > + * size or can be for the entire vma (in which case pfn, size are
> > zero).
> > + *
> > + * NOTE: mremap may move a virtual address of VM_PFNMAP, but keeps the
> > + * pfn and cache type.  In this case, untrack_pfn() is called with the
> > + * old vma after its translation has removed.  Hence, when
> > follow_phys()
> > + * fails, track_pfn() keeps the pfn tracked and clears VM_PAT from the
> > + * old vma.
> >   */
> >  void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn,
> >  		 unsigned long size)
> > @@ -981,14 +987,13 @@ void untrack_pfn(struct vm_area_struct *vma,
> > unsigned long pfn,
> >  	/* free the chunk starting from pfn or the whole chunk */
> >  	paddr = (resource_size_t)pfn << PAGE_SHIFT;
> >  	if (!paddr && !size) {
> > -		if (follow_phys(vma, vma->vm_start, 0, &prot, &paddr))
> > {
> > -			WARN_ON_ONCE(1);
> > -			return;
> > -		}
> > +		if (follow_phys(vma, vma->vm_start, 0, &prot, &paddr))
> > +			goto out;
> 
> Shouldn't we have an explicit call in the mremap code which clears the
> PAT flag on the mm instead of removing this sanity check?
>   
> Because that's what we end up with there. We just clear the PAT flag.
> 
> I rather prefer to do that explicitely, so the following call to
> untrack_pfn() from move_vma()->do_munmap() ... will see the PAT flag
> cleared. untrack_moved_pfn() or such.

Agreed.  I will add untrack_pfn_moved(), which clears the PAT flag.

Thanks!
-Toshi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web