mm: move huge_pmd_set_accessed out of huge_memory.c

huge_pmd_set_accessed is only called by __handle_mm_fault from memory.c,
move the definition to memory.c and make it static like create_huge_pmd and
wp_huge_pmd.

Signed-off-by: Yang Shi <yang.shi@linaro.org>

---
 include/linux/huge_mm.h |  4 ----
 mm/huge_memory.c        | 23 -----------------------
 mm/memory.c             | 23 +++++++++++++++++++++++
 3 files changed, 23 insertions(+), 27 deletions(-)

-- 
2.0.2

Hi folks,

I didn't realize pmd_* functions are protected by 
CONFIG_TRANSPARENT_HUGEPAGE on the most architectures before I made this 
change.

Before I fix all the affected architectures code, I want to check if you 
guys think this change is worth or not?

Thanks,
Yang

On 4/20/2016 11:24 AM, Yang Shi wrote:
> huge_pmd_set_accessed is only called by __handle_mm_fault from memory.c,

> move the definition to memory.c and make it static like create_huge_pmd and

> wp_huge_pmd.

>

> Signed-off-by: Yang Shi <yang.shi@linaro.org>

> ---

>   include/linux/huge_mm.h |  4 ----

>   mm/huge_memory.c        | 23 -----------------------

>   mm/memory.c             | 23 +++++++++++++++++++++++

>   3 files changed, 23 insertions(+), 27 deletions(-)

>

> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h

> index 7008623..c218ab7b 100644

> --- a/include/linux/huge_mm.h

> +++ b/include/linux/huge_mm.h

> @@ -8,10 +8,6 @@ extern int do_huge_pmd_anonymous_page(struct mm_struct *mm,

>   extern int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,

>   			 pmd_t *dst_pmd, pmd_t *src_pmd, unsigned long addr,

>   			 struct vm_area_struct *vma);

> -extern void huge_pmd_set_accessed(struct mm_struct *mm,

> -				  struct vm_area_struct *vma,

> -				  unsigned long address, pmd_t *pmd,

> -				  pmd_t orig_pmd, int dirty);

>   extern int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,

>   			       unsigned long address, pmd_t *pmd,

>   			       pmd_t orig_pmd);

> diff --git a/mm/huge_memory.c b/mm/huge_memory.c

> index fecbbc5..6c14cb6 100644

> --- a/mm/huge_memory.c

> +++ b/mm/huge_memory.c

> @@ -1137,29 +1137,6 @@ out:

>   	return ret;

>   }

>

> -void huge_pmd_set_accessed(struct mm_struct *mm,

> -			   struct vm_area_struct *vma,

> -			   unsigned long address,

> -			   pmd_t *pmd, pmd_t orig_pmd,

> -			   int dirty)

> -{

> -	spinlock_t *ptl;

> -	pmd_t entry;

> -	unsigned long haddr;

> -

> -	ptl = pmd_lock(mm, pmd);

> -	if (unlikely(!pmd_same(*pmd, orig_pmd)))

> -		goto unlock;

> -

> -	entry = pmd_mkyoung(orig_pmd);

> -	haddr = address & HPAGE_PMD_MASK;

> -	if (pmdp_set_access_flags(vma, haddr, pmd, entry, dirty))

> -		update_mmu_cache_pmd(vma, address, pmd);

> -

> -unlock:

> -	spin_unlock(ptl);

> -}

> -

>   static int do_huge_pmd_wp_page_fallback(struct mm_struct *mm,

>   					struct vm_area_struct *vma,

>   					unsigned long address,

> diff --git a/mm/memory.c b/mm/memory.c

> index 93897f2..6ced4eb 100644

> --- a/mm/memory.c

> +++ b/mm/memory.c

> @@ -3287,6 +3287,29 @@ static int wp_huge_pmd(struct mm_struct *mm, struct vm_area_struct *vma,

>   	return VM_FAULT_FALLBACK;

>   }

>

> +static void huge_pmd_set_accessed(struct mm_struct *mm,

> +				  struct vm_area_struct *vma,

> +				  unsigned long address,

> +				  pmd_t *pmd, pmd_t orig_pmd,

> +				  int dirty)

> +{

> +	spinlock_t *ptl;

> +	pmd_t entry;

> +	unsigned long haddr;

> +

> +	ptl = pmd_lock(mm, pmd);

> +	if (unlikely(!pmd_same(*pmd, orig_pmd)))

> +		goto unlock;

> +

> +	entry = pmd_mkyoung(orig_pmd);

> +	haddr = address & HPAGE_PMD_MASK;

> +	if (pmdp_set_access_flags(vma, haddr, pmd, entry, dirty))

> +		update_mmu_cache_pmd(vma, address, pmd);

> +

> +unlock:

> +	spin_unlock(ptl);

> +}

> +

>   /*

>    * These routines also need to handle stuff like marking pages dirty

>    * and/or accessed for architectures that don't do it in hardware (most

>

On 4/21/2016 12:30 AM, Kirill A. Shutemov wrote:
> On Wed, Apr 20, 2016 at 02:00:11PM -0700, Shi, Yang wrote:

>> Hi folks,

>>

>> I didn't realize pmd_* functions are protected by

>> CONFIG_TRANSPARENT_HUGEPAGE on the most architectures before I made this

>> change.

>>

>> Before I fix all the affected architectures code, I want to check if you

>> guys think this change is worth or not?

>>

>> Thanks,

>> Yang

>>

>> On 4/20/2016 11:24 AM, Yang Shi wrote:

>>> huge_pmd_set_accessed is only called by __handle_mm_fault from memory.c,

>>> move the definition to memory.c and make it static like create_huge_pmd and

>>> wp_huge_pmd.

>>>

>>> Signed-off-by: Yang Shi <yang.shi@linaro.org>

>

> On pte side we have the same functionality open-coded. Should we do the

> same for pmd? Or change pte side the same way?


Sorry, I don't quite understand you. Do you mean pte_* functions?

Thanks,
Yang

>

On 4/21/2016 2:15 AM, Hugh Dickins wrote:
> On Wed, 20 Apr 2016, Shi, Yang wrote:

>

>> Hi folks,

>>

>> I didn't realize pmd_* functions are protected by CONFIG_TRANSPARENT_HUGEPAGE

>> on the most architectures before I made this change.

>>

>> Before I fix all the affected architectures code, I want to check if you guys

>> think this change is worth or not?

>

> Thanks for asking: no, it is not worthwhile.

>

> I would much prefer not to have to consider these trivial cleanups

> in the huge memory area at this time.  Kirill and I have urgent work

> to do in this area, and coping with patch conflicts between different

> versions of the source will not help any of us.


Thanks for your suggestion. I would consider to put such cleanup work on 
the back burner.

Yang

>

> Thanks,

> Hugh

>

>>

>> Thanks,

>> Yang

>>

>> On 4/20/2016 11:24 AM, Yang Shi wrote:

>>> huge_pmd_set_accessed is only called by __handle_mm_fault from memory.c,

>>> move the definition to memory.c and make it static like create_huge_pmd and

>>> wp_huge_pmd.

>>>

>>> Signed-off-by: Yang Shi <yang.shi@linaro.org>

>>> ---

>>>    include/linux/huge_mm.h |  4 ----

>>>    mm/huge_memory.c        | 23 -----------------------

>>>    mm/memory.c             | 23 +++++++++++++++++++++++

>>>    3 files changed, 23 insertions(+), 27 deletions(-)

>>>

>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h

>>> index 7008623..c218ab7b 100644

>>> --- a/include/linux/huge_mm.h

>>> +++ b/include/linux/huge_mm.h

>>> @@ -8,10 +8,6 @@ extern int do_huge_pmd_anonymous_page(struct mm_struct

>>> *mm,

>>>    extern int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct

>>> *src_mm,

>>>    			 pmd_t *dst_pmd, pmd_t *src_pmd, unsigned long addr,

>>>    			 struct vm_area_struct *vma);

>>> -extern void huge_pmd_set_accessed(struct mm_struct *mm,

>>> -				  struct vm_area_struct *vma,

>>> -				  unsigned long address, pmd_t *pmd,

>>> -				  pmd_t orig_pmd, int dirty);

>>>    extern int do_huge_pmd_wp_page(struct mm_struct *mm, struct

>>> vm_area_struct *vma,

>>>    			       unsigned long address, pmd_t *pmd,

>>>    			       pmd_t orig_pmd);

>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c

>>> index fecbbc5..6c14cb6 100644

>>> --- a/mm/huge_memory.c

>>> +++ b/mm/huge_memory.c

>>> @@ -1137,29 +1137,6 @@ out:

>>>    	return ret;

>>>    }

>>>

>>> -void huge_pmd_set_accessed(struct mm_struct *mm,

>>> -			   struct vm_area_struct *vma,

>>> -			   unsigned long address,

>>> -			   pmd_t *pmd, pmd_t orig_pmd,

>>> -			   int dirty)

>>> -{

>>> -	spinlock_t *ptl;

>>> -	pmd_t entry;

>>> -	unsigned long haddr;

>>> -

>>> -	ptl = pmd_lock(mm, pmd);

>>> -	if (unlikely(!pmd_same(*pmd, orig_pmd)))

>>> -		goto unlock;

>>> -

>>> -	entry = pmd_mkyoung(orig_pmd);

>>> -	haddr = address & HPAGE_PMD_MASK;

>>> -	if (pmdp_set_access_flags(vma, haddr, pmd, entry, dirty))

>>> -		update_mmu_cache_pmd(vma, address, pmd);

>>> -

>>> -unlock:

>>> -	spin_unlock(ptl);

>>> -}

>>> -

>>>    static int do_huge_pmd_wp_page_fallback(struct mm_struct *mm,

>>>    					struct vm_area_struct *vma,

>>>    					unsigned long address,

>>> diff --git a/mm/memory.c b/mm/memory.c

>>> index 93897f2..6ced4eb 100644

>>> --- a/mm/memory.c

>>> +++ b/mm/memory.c

>>> @@ -3287,6 +3287,29 @@ static int wp_huge_pmd(struct mm_struct *mm, struct

>>> vm_area_struct *vma,

>>>    	return VM_FAULT_FALLBACK;

>>>    }

>>>

>>> +static void huge_pmd_set_accessed(struct mm_struct *mm,

>>> +				  struct vm_area_struct *vma,

>>> +				  unsigned long address,

>>> +				  pmd_t *pmd, pmd_t orig_pmd,

>>> +				  int dirty)

>>> +{

>>> +	spinlock_t *ptl;

>>> +	pmd_t entry;

>>> +	unsigned long haddr;

>>> +

>>> +	ptl = pmd_lock(mm, pmd);

>>> +	if (unlikely(!pmd_same(*pmd, orig_pmd)))

>>> +		goto unlock;

>>> +

>>> +	entry = pmd_mkyoung(orig_pmd);

>>> +	haddr = address & HPAGE_PMD_MASK;

>>> +	if (pmdp_set_access_flags(vma, haddr, pmd, entry, dirty))

>>> +		update_mmu_cache_pmd(vma, address, pmd);

>>> +

>>> +unlock:

>>> +	spin_unlock(ptl);

>>> +}

>>> +

>>>    /*

>>>     * These routines also need to handle stuff like marking pages dirty

>>>     * and/or accessed for architectures that don't do it in hardware (most

On 4/22/2016 2:48 AM, Kirill A. Shutemov wrote:
> On Thu, Apr 21, 2016 at 03:56:07PM -0700, Shi, Yang wrote:

>> On 4/21/2016 12:30 AM, Kirill A. Shutemov wrote:

>>> On Wed, Apr 20, 2016 at 02:00:11PM -0700, Shi, Yang wrote:

>>>> Hi folks,

>>>>

>>>> I didn't realize pmd_* functions are protected by

>>>> CONFIG_TRANSPARENT_HUGEPAGE on the most architectures before I made this

>>>> change.

>>>>

>>>> Before I fix all the affected architectures code, I want to check if you

>>>> guys think this change is worth or not?

>>>>

>>>> Thanks,

>>>> Yang

>>>>

>>>> On 4/20/2016 11:24 AM, Yang Shi wrote:

>>>>> huge_pmd_set_accessed is only called by __handle_mm_fault from memory.c,

>>>>> move the definition to memory.c and make it static like create_huge_pmd and

>>>>> wp_huge_pmd.

>>>>>

>>>>> Signed-off-by: Yang Shi <yang.shi@linaro.org>

>>>

>>> On pte side we have the same functionality open-coded. Should we do the

>>> same for pmd? Or change pte side the same way?

>>

>> Sorry, I don't quite understand you. Do you mean pte_* functions?

>

> See handle_pte_fault(), we do the same for pte there what

> huge_pmd_set_accessed() does for pmd.


Thanks for directing to this code.

>

> I think we should be consistent here: either both are abstructed into

> functions or both open-coded.


I'm supposed functions sound better. However, do_wp_page has to be 
called with pte lock acquired. So, the abstracted function has to call it.

Thanks,
Yang


>