[PATCH] damon: Use pmdp_get instead of drect dereferencing pmd.

[PATCH] damon: Use pmdp_get instead of drect dereferencing pmd.

[Levi Yun]

As ptep_get, Use the pmdp_get wrapper when we accessing pmdval
instead of direct dereferencing pmd.

Signed-off-by: Levi Yun <ppbuk5246@gmail.com>
---
 mm/damon/ops-common.c |  2 +-
 mm/damon/paddr.c      |  2 +-
 mm/damon/vaddr.c      | 22 ++++++++++++++--------
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
index e940802a15a4..ac1c3fa80f98 100644
--- a/mm/damon/ops-common.c
+++ b/mm/damon/ops-common.c
@@ -54,7 +54,7 @@ void damon_ptep_mkold(pte_t *pte, struct vm_area_struct *vma, unsigned long addr
 void damon_pmdp_mkold(pmd_t *pmd, struct vm_area_struct *vma, unsigned long addr)
 {
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
-	struct folio *folio = damon_get_folio(pmd_pfn(*pmd));
+	struct folio *folio = damon_get_folio(pmd_pfn(pmdp_get(pmd)));
 
 	if (!folio)
 		return;
diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index 40801e38fcf0..909db25efb35 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -94,7 +94,7 @@ static bool __damon_pa_young(struct folio *folio, struct vm_area_struct *vma,
 				mmu_notifier_test_young(vma->vm_mm, addr);
 		} else {
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
-			*accessed = pmd_young(*pvmw.pmd) ||
+			*accessed = pmd_young(pmdp_get(pvmw.pmd)) ||
 				!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 2fcc9731528a..82f7865719fe 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -301,16 +301,19 @@ static int damon_mkold_pmd_entry(pmd_t *pmd, unsigned long addr,
 		unsigned long next, struct mm_walk *walk)
 {
 	pte_t *pte;
+	pmd_t pmde;
 	spinlock_t *ptl;
 
-	if (pmd_trans_huge(*pmd)) {
+	if (pmd_trans_huge(pmdp_get_lockless(pmd))) {
 		ptl = pmd_lock(walk->mm, pmd);
-		if (!pmd_present(*pmd)) {
+		pmde = pmdp_get(pmd);
+
+		if (!pmd_present(pmde)) {
 			spin_unlock(ptl);
 			return 0;
 		}
 
-		if (pmd_trans_huge(*pmd)) {
+		if (pmd_trans_huge(pmde)) {
 			damon_pmdp_mkold(pmd, walk->vma, addr);
 			spin_unlock(ptl);
 			return 0;
@@ -434,26 +437,29 @@ static int damon_young_pmd_entry(pmd_t *pmd, unsigned long addr,
 {
 	pte_t *pte;
 	pte_t ptent;
+	pmd_t pmde;
 	spinlock_t *ptl;
 	struct folio *folio;
 	struct damon_young_walk_private *priv = walk->private;
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
-	if (pmd_trans_huge(*pmd)) {
+	if (pmd_trans_huge(pmdp_get_lockless(pmd))) {
 		ptl = pmd_lock(walk->mm, pmd);
-		if (!pmd_present(*pmd)) {
+		pmde = pmdp_get(pmd);
+
+		if (!pmd_present(pmde)) {
 			spin_unlock(ptl);
 			return 0;
 		}
 
-		if (!pmd_trans_huge(*pmd)) {
+		if (!pmd_trans_huge(pmde)) {
 			spin_unlock(ptl);
 			goto regular_page;
 		}
-		folio = damon_get_folio(pmd_pfn(*pmd));
+		folio = damon_get_folio(pmd_pfn(pmde));
 		if (!folio)
 			goto huge_out;
-		if (pmd_young(*pmd) || !folio_test_idle(folio) ||
+		if (pmd_young(pmde) || !folio_test_idle(folio) ||
 					mmu_notifier_test_young(walk->mm,
 						addr))
 			priv->young = true;
-- 
2.37.2

Re: [PATCH] damon: Use pmdp_get instead of drect dereferencing pmd.

[SeongJae Park]

Hi Levi,


Thank you for this patch.

Nit for the subject, what about s/drect/directly/?  Also, let's remove the
period at the end.

On Fri, 28 Jul 2023 03:37:45 +0900 Levi Yun <ppbuk5246@gmail.com> wrote:

> As ptep_get, Use the pmdp_get wrapper when we accessing pmdval
> instead of direct dereferencing pmd.

Nit: s/Use/use/ and s/direct/directly

> 
> Signed-off-by: Levi Yun <ppbuk5246@gmail.com>
> ---
>  mm/damon/ops-common.c |  2 +-
>  mm/damon/paddr.c      |  2 +-
>  mm/damon/vaddr.c      | 22 ++++++++++++++--------
>  3 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> index e940802a15a4..ac1c3fa80f98 100644
> --- a/mm/damon/ops-common.c
> +++ b/mm/damon/ops-common.c
> @@ -54,7 +54,7 @@ void damon_ptep_mkold(pte_t *pte, struct vm_area_struct *vma, unsigned long addr
>  void damon_pmdp_mkold(pmd_t *pmd, struct vm_area_struct *vma, unsigned long addr)
>  {
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> -	struct folio *folio = damon_get_folio(pmd_pfn(*pmd));
> +	struct folio *folio = damon_get_folio(pmd_pfn(pmdp_get(pmd)));
>  
>  	if (!folio)
>  		return;
> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 40801e38fcf0..909db25efb35 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -94,7 +94,7 @@ static bool __damon_pa_young(struct folio *folio, struct vm_area_struct *vma,
>  				mmu_notifier_test_young(vma->vm_mm, addr);
>  		} else {
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> -			*accessed = pmd_young(*pvmw.pmd) ||
> +			*accessed = pmd_young(pmdp_get(pvmw.pmd)) ||
>  				!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 2fcc9731528a..82f7865719fe 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -301,16 +301,19 @@ static int damon_mkold_pmd_entry(pmd_t *pmd, unsigned long addr,
>  		unsigned long next, struct mm_walk *walk)
>  {
>  	pte_t *pte;
> +	pmd_t pmde;
>  	spinlock_t *ptl;
>  
> -	if (pmd_trans_huge(*pmd)) {
> +	if (pmd_trans_huge(pmdp_get_lockless(pmd))) {

I don't think we really need to use pmdp_get_lockless() here, since we will do
this check again with the lock, and we have the fallback for the intermediate
changes.

>  		ptl = pmd_lock(walk->mm, pmd);
> -		if (!pmd_present(*pmd)) {
> +		pmde = pmdp_get(pmd);
> +
> +		if (!pmd_present(pmde)) {
>  			spin_unlock(ptl);
>  			return 0;
>  		}
>  
> -		if (pmd_trans_huge(*pmd)) {
> +		if (pmd_trans_huge(pmde)) {
>  			damon_pmdp_mkold(pmd, walk->vma, addr);
>  			spin_unlock(ptl);
>  			return 0;
> @@ -434,26 +437,29 @@ static int damon_young_pmd_entry(pmd_t *pmd, unsigned long addr,
>  {
>  	pte_t *pte;
>  	pte_t ptent;
> +	pmd_t pmde;

This would cause below build warning if CONFIG_TRANSPARENT_HUGEPAGE is not
defined.

.../mm/damon/vaddr.c:440:8: warning: unused variable 'pmde' [-Wunused-variable]
  440 |  pmd_t pmde;
      |        ^~~~


>  	spinlock_t *ptl;
>  	struct folio *folio;
>  	struct damon_young_walk_private *priv = walk->private;
>  
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> -	if (pmd_trans_huge(*pmd)) {
> +	if (pmd_trans_huge(pmdp_get_lockless(pmd))) {

Again, pmdp_get() might be enough?

>  		ptl = pmd_lock(walk->mm, pmd);
> -		if (!pmd_present(*pmd)) {
> +		pmde = pmdp_get(pmd);
> +
> +		if (!pmd_present(pmde)) {
>  			spin_unlock(ptl);
>  			return 0;
>  		}
>  
> -		if (!pmd_trans_huge(*pmd)) {
> +		if (!pmd_trans_huge(pmde)) {
>  			spin_unlock(ptl);
>  			goto regular_page;
>  		}
> -		folio = damon_get_folio(pmd_pfn(*pmd));
> +		folio = damon_get_folio(pmd_pfn(pmde));
>  		if (!folio)
>  			goto huge_out;
> -		if (pmd_young(*pmd) || !folio_test_idle(folio) ||
> +		if (pmd_young(pmde) || !folio_test_idle(folio) ||
>  					mmu_notifier_test_young(walk->mm,
>  						addr))
>  			priv->young = true;
> -- 
> 2.37.2
> 

Thanks,
SJ

Re: [PATCH] damon: Use pmdp_get instead of drect dereferencing pmd.

[Yun Levi]

Hi SJ.

Thanks for looking into this.

> Nit for the subject, what about s/drect/directly/?  Also, let's remove the
> period at the end.
>
> On Fri, 28 Jul 2023 03:37:45 +0900 Levi Yun <ppbuk5246@gmail.com> wrote:

Thanks. But I don't know why period is added thou I sent via git send-email ..

> > -     if (pmd_trans_huge(*pmd)) {
> > +     if (pmd_trans_huge(pmdp_get_lockless(pmd))) {
>
> I don't think we really need to use pmdp_get_lockless() here, since we will do
> this check again with the lock, and we have the fallback for the intermediate
> changes.

Agree. I'll change. Thanks :)

> > @@ -434,26 +437,29 @@ static int damon_young_pmd_entry(pmd_t *pmd, unsigned long addr,
> >  {
> >       pte_t *pte;
> >       pte_t ptent;
> > +     pmd_t pmde;
>
> This would cause below build warning if CONFIG_TRANSPARENT_HUGEPAGE is not
> defined.
>
> .../mm/damon/vaddr.c:440:8: warning: unused variable 'pmde' [-Wunused-variable]
>   440 |  pmd_t pmde;
>       |        ^~~~
>
>
> >       spinlock_t *ptl;
> >       struct folio *folio;
> >       struct damon_young_walk_private *priv = walk->private;
> >
> >  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> > -     if (pmd_trans_huge(*pmd)) {
> > +     if (pmd_trans_huge(pmdp_get_lockless(pmd))) {

Oh.. I miss this. Many thanks..!

> Thanks,
> SJ

Many Thanks..!

---------
Sincerely,
Levi.

Re: [PATCH] damon: Use pmdp_get instead of drect dereferencing pmd.

[kernel test robot]

Hi Levi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on akpm-mm/mm-everything]
[also build test WARNING on linus/master v6.5-rc3 next-20230728]
[cannot apply to sj/damon/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Levi-Yun/damon-Use-pmdp_get-instead-of-drect-dereferencing-pmd/20230728-024044
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20230727183745.682880-1-ppbuk5246%40gmail.com
patch subject: [PATCH] damon: Use pmdp_get instead of drect dereferencing pmd.
config: i386-randconfig-i016-20230727 (https://download.01.org/0day-ci/archive/20230728/202307281532.pVsrTsL2-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230728/202307281532.pVsrTsL2-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307281532.pVsrTsL2-lkp@intel.com/

All warnings (new ones prefixed by >>):

   mm/damon/vaddr.c: In function 'damon_young_pmd_entry':
>> mm/damon/vaddr.c:440:15: warning: unused variable 'pmde' [-Wunused-variable]
     440 |         pmd_t pmde;
         |               ^~~~


vim +/pmde +440 mm/damon/vaddr.c

   434	
   435	static int damon_young_pmd_entry(pmd_t *pmd, unsigned long addr,
   436			unsigned long next, struct mm_walk *walk)
   437	{
   438		pte_t *pte;
   439		pte_t ptent;
 > 440		pmd_t pmde;
   441		spinlock_t *ptl;
   442		struct folio *folio;
   443		struct damon_young_walk_private *priv = walk->private;
   444	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] damon: Use pmdp_get instead of drect dereferencing pmd.

[Yun Levi]

This build warning is fixed in v2:
    https://lore.kernel.org/all/20230727212157.2985025-1-ppbuk5246@gmail.com/

On Fri, Jul 28, 2023 at 9:09 AM kernel test robot <lkp@intel.com> wrote:
>
> Hi Levi,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on akpm-mm/mm-everything]
> [also build test WARNING on linus/master v6.5-rc3 next-20230728]
> [cannot apply to sj/damon/next]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Levi-Yun/damon-Use-pmdp_get-instead-of-drect-dereferencing-pmd/20230728-024044
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
> patch link:    https://lore.kernel.org/r/20230727183745.682880-1-ppbuk5246%40gmail.com
> patch subject: [PATCH] damon: Use pmdp_get instead of drect dereferencing pmd.
> config: i386-randconfig-i016-20230727 (https://download.01.org/0day-ci/archive/20230728/202307281532.pVsrTsL2-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20230728/202307281532.pVsrTsL2-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202307281532.pVsrTsL2-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
>    mm/damon/vaddr.c: In function 'damon_young_pmd_entry':
> >> mm/damon/vaddr.c:440:15: warning: unused variable 'pmde' [-Wunused-variable]
>      440 |         pmd_t pmde;
>          |               ^~~~
>
>
> vim +/pmde +440 mm/damon/vaddr.c
>
>    434
>    435  static int damon_young_pmd_entry(pmd_t *pmd, unsigned long addr,
>    436                  unsigned long next, struct mm_walk *walk)
>    437  {
>    438          pte_t *pte;
>    439          pte_t ptent;
>  > 440          pmd_t pmde;
>    441          spinlock_t *ptl;
>    442          struct folio *folio;
>    443          struct damon_young_walk_private *priv = walk->private;
>    444
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>