Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   SeongJae Park <sj@kernel.org>
To:     Ryan Roberts <ryan.roberts@arm.com>
Cc:     "SeongJae Park" <sj@kernel.org>,
        "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        "Mike Rapoport" <rppt@kernel.org>, "Yu Zhao" <yuzhao@google.com>,
        "Jason Gunthorpe" <jgg@ziepe.ca>,
        "David Airlie" <airlied@gmail.com>,
        "Daniel Vetter" <daniel@ffwll.ch>,
        "Dimitri Sivanich" <dimitri.sivanich@hpe.com>,
        "Alex Williamson" <alex.williamson@redhat.com>,
        "Oleksandr Tyshchenko" <oleksandr_tyshchenko@epam.com>,
        "Alexander Viro" <viro@zeniv.linux.org.uk>,
        "Christian Brauner" <brauner@kernel.org>,
        "Mike Kravetz" <mike.kravetz@oracle.com>,
        "Muchun Song" <muchun.song@linux.dev>,
        "Mark Rutland" <mark.rutland@arm.com>,
        "Alexander Shishkin" <alexander.shishkin@linux.intel.com>,
        "Jiri Olsa" <jolsa@kernel.org>,
        "Namhyung Kim" <namhyung@kernel.org>,
        "Ian Rogers" <irogers@google.com>,
        "Adrian Hunter" <adrian.hunter@intel.com>,
        =?UTF-8?q?J=C3=A9r=C3=B4me=20Glisse?= <jglisse@redhat.com>,
        "Andrey Ryabinin" <ryabinin.a.a@gmail.com>,
        "Alexander Potapenko" <glider@google.com>,
        "Andrey Konovalov" <andreyknvl@gmail.com>,
        "Dmitry Vyukov" <dvyukov@google.com>,
        "Vincenzo Frascino" <vincenzo.frascino@arm.com>,
        "Johannes Weiner" <hannes@cmpxchg.org>,
        "Michal Hocko" <mhocko@kernel.org>,
        "Roman Gushchin" <roman.gushchin@linux.dev>,
        "Shakeel Butt" <shakeelb@google.com>,
        "Naoya Horiguchi" <naoya.horiguchi@nec.com>,
        "Miaohe Lin" <linmiaohe@huawei.com>,
        "Pasha Tatashin" <pasha.tatashin@soleen.com>,
        "Uladzislau Rezki" <urezki@gmail.com>,
        "Christoph Hellwig" <hch@infradead.org>,
        "Lorenzo Stoakes" <lstoakes@gmail.com>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        damon@lists.linux.dev, kernel test robot <lkp@intel.com>
Subject: Re: [PATCH v3 3/3] mm: ptep_get() conversion
Date:   Mon, 12 Jun 2023 21:27:23 +0000
Message-Id: <20230612212723.196693-1-sj@kernel.org>
In-Reply-To: <20230612151545.3317766-4-ryan.roberts@arm.com>
References: 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Mon, 12 Jun 2023 16:15:45 +0100 Ryan Roberts <ryan.roberts@arm.com> wrote:

> Convert all instances of direct pte_t* dereferencing to instead use
> ptep_get() helper. This means that by default, the accesses change from
> a C dereference to a READ_ONCE(). This is technically the correct thing
> to do since where pgtables are modified by HW (for access/dirty) they
> are volatile and therefore we should always ensure READ_ONCE()
> semantics.
> 
> But more importantly, by always using the helper, it can be overridden
> by the architecture to fully encapsulate the contents of the pte. Arch
> code is deliberately not converted, as the arch code knows best. It is
> intended that arch code (arm64) will override the default with its own
> implementation that can (e.g.) hide certain bits from the core code, or
> determine young/dirty status by mixing in state from another source.
> 
> Conversion was done using Coccinelle:
> 
> ----
> 
> // $ make coccicheck \
> //          COCCI=ptepget.cocci \
> //          SPFLAGS="--include-headers" \
> //          MODE=patch
> 
> virtual patch
> 
> @ depends on patch @
> pte_t *v;
> @@
> 
> - *v
> + ptep_get(v)
> 
> ----
> 
> Then reviewed and hand-edited to avoid multiple unnecessary calls to
> ptep_get(), instead opting to store the result of a single call in a
> variable, where it is correct to do so. This aims to negate any cost of
> READ_ONCE() and will benefit arch-overrides that may be more complex.
> 
> Included is a fix for an issue in an earlier version of this patch that
> was pointed out by kernel test robot. The issue arose because config
> MMU=n elides definition of the ptep helper functions, including
> ptep_get(). HUGETLB_PAGE=n configs still define a simple
> huge_ptep_clear_flush() for linking purposes, which dereferences the
> ptep. So when both configs are disabled, this caused a build error
> because ptep_get() is not defined. Fix by continuing to do a direct
> dereference when MMU=n. This is safe because for this config the arch
> code cannot be trying to virtualize the ptes because none of the ptep
> helpers are defined.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Link: https://lore.kernel.org/oe-kbuild-all/202305120142.yXsNEo6H-lkp@intel.com/
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>  .../drm/i915/gem/selftests/i915_gem_mman.c    |   8 +-
>  drivers/misc/sgi-gru/grufault.c               |   2 +-
>  drivers/vfio/vfio_iommu_type1.c               |   7 +-
>  drivers/xen/privcmd.c                         |   2 +-
>  fs/proc/task_mmu.c                            |  33 +++---
>  fs/userfaultfd.c                              |   6 +-
>  include/linux/hugetlb.h                       |   4 +
>  include/linux/mm_inline.h                     |   2 +-
>  include/linux/pgtable.h                       |   6 +-
>  kernel/events/uprobes.c                       |   2 +-
>  mm/damon/ops-common.c                         |   2 +-
>  mm/damon/paddr.c                              |   2 +-
>  mm/damon/vaddr.c                              |  10 +-
[...]
> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> index d4ab81229136..e940802a15a4 100644
> --- a/mm/damon/ops-common.c
> +++ b/mm/damon/ops-common.c
> @@ -39,7 +39,7 @@ struct folio *damon_get_folio(unsigned long pfn)
>  
>  void damon_ptep_mkold(pte_t *pte, struct vm_area_struct *vma, unsigned long addr)
>  {
> -	struct folio *folio = damon_get_folio(pte_pfn(*pte));
> +	struct folio *folio = damon_get_folio(pte_pfn(ptep_get(pte)));
>  
>  	if (!folio)
>  		return;
> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 5b3a3463d078..40801e38fcf0 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -89,7 +89,7 @@ static bool __damon_pa_young(struct folio *folio, struct vm_area_struct *vma,
>  	while (page_vma_mapped_walk(&pvmw)) {
>  		addr = pvmw.address;
>  		if (pvmw.pte) {
> -			*accessed = pte_young(*pvmw.pte) ||
> +			*accessed = pte_young(ptep_get(pvmw.pte)) ||
>  				!folio_test_idle(folio) ||
>  				mmu_notifier_test_young(vma->vm_mm, addr);
>  		} else {
> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index e814f66dfc2e..2fcc9731528a 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -323,7 +323,7 @@ static int damon_mkold_pmd_entry(pmd_t *pmd, unsigned long addr,
>  		walk->action = ACTION_AGAIN;
>  		return 0;
>  	}
> -	if (!pte_present(*pte))
> +	if (!pte_present(ptep_get(pte)))
>  		goto out;
>  	damon_ptep_mkold(pte, walk->vma, addr);
>  out:
> @@ -433,6 +433,7 @@ static int damon_young_pmd_entry(pmd_t *pmd, unsigned long addr,
>  		unsigned long next, struct mm_walk *walk)
>  {
>  	pte_t *pte;
> +	pte_t ptent;
>  	spinlock_t *ptl;
>  	struct folio *folio;
>  	struct damon_young_walk_private *priv = walk->private;
> @@ -471,12 +472,13 @@ static int damon_young_pmd_entry(pmd_t *pmd, unsigned long addr,
>  		walk->action = ACTION_AGAIN;
>  		return 0;
>  	}
> -	if (!pte_present(*pte))
> +	ptent = ptep_get(pte);
> +	if (!pte_present(ptent))
>  		goto out;
> -	folio = damon_get_folio(pte_pfn(*pte));
> +	folio = damon_get_folio(pte_pfn(ptent));
>  	if (!folio)
>  		goto out;
> -	if (pte_young(*pte) || !folio_test_idle(folio) ||
> +	if (pte_young(ptent) || !folio_test_idle(folio) ||
>  			mmu_notifier_test_young(walk->mm, addr))
>  		priv->young = true;
>  	*priv->folio_sz = folio_size(folio);

For above DAMON part,

Reviewed-by: SeongJae Park <sj@kernel.org>


Thanks,
SJ

[...]