lkml.org 
[lkml]   [2005]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/9] mm: parisc pte atomicity
On Sat, Oct 22, 2005 at 05:23:27PM +0100, Hugh Dickins wrote:
> There's a worrying function translation_exists in parisc cacheflush.h,
> unaffected by split ptlock since flush_dcache_page is using it on some
> other mm, without any relevant lock. Oh well, make it a slightly more
> robust by factoring the pfn check within it. And it looked liable to
> confuse a camouflaged swap or file entry with a good pte: fix that too.

I have to say I really don't understand VM at all. cc'ing the
parisc-linux list in case anyone there has a better understanding than I
do.

> Signed-off-by: Hugh Dickins <hugh@veritas.com>
> ---
>
> arch/parisc/kernel/cache.c | 24 +++++++++---------------
> include/asm-parisc/cacheflush.h | 35 +++++++++++++++++++----------------
> 2 files changed, 28 insertions(+), 31 deletions(-)
>
> --- mm2/arch/parisc/kernel/cache.c 2005-03-02 07:38:56.000000000 +0000
> +++ mm3/arch/parisc/kernel/cache.c 2005-10-22 14:06:30.000000000 +0100
> @@ -266,7 +266,6 @@ void flush_dcache_page(struct page *page
> unsigned long offset;
> unsigned long addr;
> pgoff_t pgoff;
> - pte_t *pte;
> unsigned long pfn = page_to_pfn(page);
>
>
> @@ -297,21 +296,16 @@ void flush_dcache_page(struct page *page
> * taking a page fault if the pte doesn't exist.
> * This is just for speed. If the page translation
> * isn't there, there's no point exciting the
> - * nadtlb handler into a nullification frenzy */
> -
> -
> - if(!(pte = translation_exists(mpnt, addr)))
> - continue;
> -
> - /* make sure we really have this page: the private
> + * nadtlb handler into a nullification frenzy.
> + *
> + * Make sure we really have this page: the private
> * mappings may cover this area but have COW'd this
> - * particular page */
> - if(pte_pfn(*pte) != pfn)
> - continue;
> -
> - __flush_cache_page(mpnt, addr);
> -
> - break;
> + * particular page.
> + */
> + if (translation_exists(mpnt, addr, pfn)) {
> + __flush_cache_page(mpnt, addr);
> + break;
> + }
> }
> flush_dcache_mmap_unlock(mapping);
> }
> --- mm2/include/asm-parisc/cacheflush.h 2005-10-11 12:07:49.000000000 +0100
> +++ mm3/include/asm-parisc/cacheflush.h 2005-10-22 14:06:30.000000000 +0100
> @@ -100,30 +100,34 @@ static inline void flush_cache_range(str
>
> /* Simple function to work out if we have an existing address translation
> * for a user space vma. */
> -static inline pte_t *__translation_exists(struct mm_struct *mm,
> - unsigned long addr)
> +static inline int translation_exists(struct vm_area_struct *vma,
> + unsigned long addr, unsigned long pfn)
> {
> - pgd_t *pgd = pgd_offset(mm, addr);
> + pgd_t *pgd = pgd_offset(vma->vm_mm, addr);
> pmd_t *pmd;
> - pte_t *pte;
> + pte_t pte;
>
> if(pgd_none(*pgd))
> - return NULL;
> + return 0;
>
> pmd = pmd_offset(pgd, addr);
> if(pmd_none(*pmd) || pmd_bad(*pmd))
> - return NULL;
> + return 0;
>
> - pte = pte_offset_map(pmd, addr);
> + /* We cannot take the pte lock here: flush_cache_page is usually
> + * called with pte lock already held. Whereas flush_dcache_page
> + * takes flush_dcache_mmap_lock, which is lower in the hierarchy:
> + * the vma itself is secure, but the pte might come or go racily.
> + */
> + pte = *pte_offset_map(pmd, addr);
> + /* But pte_unmap() does nothing on this architecture */
> +
> + /* Filter out coincidental file entries and swap entries */
> + if (!(pte_val(pte) & (_PAGE_FLUSH|_PAGE_PRESENT)))
> + return 0;
>
> - /* The PA flush mappings show up as pte_none, but they're
> - * valid none the less */
> - if(pte_none(*pte) && ((pte_val(*pte) & _PAGE_FLUSH) == 0))
> - return NULL;
> - return pte;
> + return pte_pfn(pte) == pfn;
> }
> -#define translation_exists(vma, addr) __translation_exists((vma)->vm_mm, addr)
> -
>
> /* Private function to flush a page from the cache of a non-current
> * process. cr25 contains the Page Directory of the current user
> @@ -175,9 +179,8 @@ flush_cache_page(struct vm_area_struct *
> {
> BUG_ON(!vma->vm_mm->context);
>
> - if(likely(translation_exists(vma, vmaddr)))
> + if (likely(translation_exists(vma, vmaddr, pfn)))
> __flush_cache_page(vma, vmaddr);
>
> }
> #endif
> -
-
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/

\
 
 \ /
  Last update: 2005-10-22 18:36    [from the cache]
©2003-2011 Jasper Spaans