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

Groups > linux.kernel > #1695864 > unrolled thread

[PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and document behaviour

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

  [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and document behaviour Punit Agrawal <punit.agrawal@arm.com> - 2017-07-25 17:50 +0200
    Re: [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and  document behaviour Catalin Marinas <catalin.marinas@arm.com> - 2017-07-26 10:40 +0200
    Re: [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and  document behaviour Michal Hocko <mhocko@kernel.org> - 2017-07-26 11:00 +0200
      Re: [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and document behaviour Punit Agrawal <punit.agrawal@arm.com> - 2017-07-26 14:20 +0200
        Re: [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and  document behaviour Michal Hocko <mhocko@kernel.org> - 2017-07-26 14:40 +0200
          Re: [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and  document behaviour Michal Hocko <mhocko@kernel.org> - 2017-07-26 14:50 +0200
            Re: [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and document behaviour Punit Agrawal <punit.agrawal@arm.com> - 2017-07-26 15:40 +0200
              Re: [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and  document behaviour Mike Kravetz <mike.kravetz@oracle.com> - 2017-07-27 05:20 +0200
                Re: [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and document behaviour Punit Agrawal <punit.agrawal@arm.com> - 2017-07-27 15:00 +0200
    Re: [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and  document behaviour Michal Hocko <mhocko@kernel.org> - 2017-07-26 11:00 +0200

#1695864 — [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and document behaviour

When walking the page tables to resolve an address that points to
!p*d_present() entry, huge_pte_offset() returns inconsistent values
depending on the level of page table (PUD or PMD).

It returns NULL in the case of a PUD entry while in the case of a PMD
entry, it returns a pointer to the page table entry.

A similar inconsitency exists when handling swap entries - returns NULL
for a PUD entry while a pointer to the pte_t is retured for the PMD
entry.

Update huge_pte_offset() to make the behaviour consistent - return NULL
in the case of p*d_none() and a pointer to the pte_t for hugepage or
swap entries.

Document the behaviour to clarify the expected behaviour of this
function. This is to set clear semantics for architecture specific
implementations of huge_pte_offset().

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
---
 mm/hugetlb.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index bc48ee783dd9..72dd1139a8e4 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4603,6 +4603,13 @@ pte_t *huge_pte_alloc(struct mm_struct *mm,
 	return pte;
 }
 
+/*
+ * huge_pte_offset() - Walk the page table to resolve the hugepage
+ * entry at address @addr
+ *
+ * Return: Pointer to page table or swap entry (PUD or PMD) for address @addr
+ * or NULL if the entry is p*d_none().
+ */
 pte_t *huge_pte_offset(struct mm_struct *mm,
 		       unsigned long addr, unsigned long sz)
 {
@@ -4617,13 +4624,22 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
 	p4d = p4d_offset(pgd, addr);
 	if (!p4d_present(*p4d))
 		return NULL;
+
 	pud = pud_offset(p4d, addr);
-	if (!pud_present(*pud))
+	if (pud_none(*pud))
 		return NULL;
-	if (pud_huge(*pud))
+	/* hugepage or swap? */
+	if (pud_huge(*pud) || !pud_present(*pud))
 		return (pte_t *)pud;
+
 	pmd = pmd_offset(pud, addr);
-	return (pte_t *) pmd;
+	if (pmd_none(*pmd))
+		return NULL;
+	/* hugepage or swap? */
+	if (pmd_huge(*pmd) || !pmd_present(*pmd))
+		return (pte_t *) pmd;
+
+	return NULL;
 }
 
 #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
-- 
2.11.0

[toc] | [next] | [standalone]

#1696882 — Re: [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and document behaviour

On Tue, Jul 25, 2017 at 04:41:14PM +0100, Punit Agrawal wrote:
> When walking the page tables to resolve an address that points to
> !p*d_present() entry, huge_pte_offset() returns inconsistent values
> depending on the level of page table (PUD or PMD).
> 
> It returns NULL in the case of a PUD entry while in the case of a PMD
> entry, it returns a pointer to the page table entry.
> 
> A similar inconsitency exists when handling swap entries - returns NULL
> for a PUD entry while a pointer to the pte_t is retured for the PMD
> entry.
> 
> Update huge_pte_offset() to make the behaviour consistent - return NULL
> in the case of p*d_none() and a pointer to the pte_t for hugepage or
> swap entries.
> 
> Document the behaviour to clarify the expected behaviour of this
> function. This is to set clear semantics for architecture specific
> implementations of huge_pte_offset().
> 
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

[toc] | [prev] | [next] | [standalone]

#1696900 — Re: [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and document behaviour

On Wed 26-07-17 10:50:38, Michal Hocko wrote:
> On Tue 25-07-17 16:41:14, Punit Agrawal wrote:
> > When walking the page tables to resolve an address that points to
> > !p*d_present() entry, huge_pte_offset() returns inconsistent values
> > depending on the level of page table (PUD or PMD).
> > 
> > It returns NULL in the case of a PUD entry while in the case of a PMD
> > entry, it returns a pointer to the page table entry.
> > 
> > A similar inconsitency exists when handling swap entries - returns NULL
> > for a PUD entry while a pointer to the pte_t is retured for the PMD
> > entry.
> > 
> > Update huge_pte_offset() to make the behaviour consistent - return NULL
> > in the case of p*d_none() and a pointer to the pte_t for hugepage or
> > swap entries.
> > 
> > Document the behaviour to clarify the expected behaviour of this
> > function. This is to set clear semantics for architecture specific
> > implementations of huge_pte_offset().
> 
> hugetlb pte semantic is a disaster and I agree it could see some
> cleanup/clarifications but I am quite nervous to see a patchi like this.
> How do we check that nothing will get silently broken by this change?

Forgot to add. Hugetlb have been special because of the pte sharing. I
haven't looked into that code for quite some time but there might be a
good reason why pud behave differently.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1697038

Hi Michal,

Michal Hocko <mhocko@kernel.org> writes:

> On Wed 26-07-17 10:50:38, Michal Hocko wrote:
>> On Tue 25-07-17 16:41:14, Punit Agrawal wrote:
>> > When walking the page tables to resolve an address that points to
>> > !p*d_present() entry, huge_pte_offset() returns inconsistent values
>> > depending on the level of page table (PUD or PMD).
>> > 
>> > It returns NULL in the case of a PUD entry while in the case of a PMD
>> > entry, it returns a pointer to the page table entry.
>> > 
>> > A similar inconsitency exists when handling swap entries - returns NULL
>> > for a PUD entry while a pointer to the pte_t is retured for the PMD
>> > entry.
>> > 
>> > Update huge_pte_offset() to make the behaviour consistent - return NULL
>> > in the case of p*d_none() and a pointer to the pte_t for hugepage or
>> > swap entries.
>> > 
>> > Document the behaviour to clarify the expected behaviour of this
>> > function. This is to set clear semantics for architecture specific
>> > implementations of huge_pte_offset().
>> 
>> hugetlb pte semantic is a disaster and I agree it could see some
>> cleanup/clarifications but I am quite nervous to see a patchi like this.
>> How do we check that nothing will get silently broken by this change?

Glad I'm not the only one who finds the hugetlb semantics somewhat
confusing. :)

I've been running tests from mce-test suite and libhugetlbfs for similar
changes we did on arm64. There could be assumptions that were not
exercised but I'm not sure how to check for all the possible usages.

Do you have any other suggestions that can help improve confidence in
the patch?

>
> Forgot to add. Hugetlb have been special because of the pte sharing. I
> haven't looked into that code for quite some time but there might be a
> good reason why pud behave differently.

I checked the code and don't see anything that would explain (or
require) the difference in behaviour.

[toc] | [prev] | [next] | [standalone]

#1697050 — Re: [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and document behaviour

On Wed 26-07-17 13:11:46, Punit Agrawal wrote:
> Hi Michal,
> 
> Michal Hocko <mhocko@kernel.org> writes:
> 
> > On Wed 26-07-17 10:50:38, Michal Hocko wrote:
> >> On Tue 25-07-17 16:41:14, Punit Agrawal wrote:
> >> > When walking the page tables to resolve an address that points to
> >> > !p*d_present() entry, huge_pte_offset() returns inconsistent values
> >> > depending on the level of page table (PUD or PMD).
> >> > 
> >> > It returns NULL in the case of a PUD entry while in the case of a PMD
> >> > entry, it returns a pointer to the page table entry.
> >> > 
> >> > A similar inconsitency exists when handling swap entries - returns NULL
> >> > for a PUD entry while a pointer to the pte_t is retured for the PMD
> >> > entry.
> >> > 
> >> > Update huge_pte_offset() to make the behaviour consistent - return NULL
> >> > in the case of p*d_none() and a pointer to the pte_t for hugepage or
> >> > swap entries.
> >> > 
> >> > Document the behaviour to clarify the expected behaviour of this
> >> > function. This is to set clear semantics for architecture specific
> >> > implementations of huge_pte_offset().
> >> 
> >> hugetlb pte semantic is a disaster and I agree it could see some
> >> cleanup/clarifications but I am quite nervous to see a patchi like this.
> >> How do we check that nothing will get silently broken by this change?
> 
> Glad I'm not the only one who finds the hugetlb semantics somewhat
> confusing. :)

This is a huge understatement. It is a source of nightmares.

> I've been running tests from mce-test suite and libhugetlbfs for similar
> changes we did on arm64. There could be assumptions that were not
> exercised but I'm not sure how to check for all the possible usages.
> 
> Do you have any other suggestions that can help improve confidence in
> the patch?

Unfortunatelly I don't. I just know there were many subtle assumptions
all over the place so I am rather careful to not touch the code unless
really necessary.

That being said, I am not opposing your patch.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1697056 — Re: [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and document behaviour

On Wed 26-07-17 14:33:57, Michal Hocko wrote:
> On Wed 26-07-17 13:11:46, Punit Agrawal wrote:
[...]
> > I've been running tests from mce-test suite and libhugetlbfs for similar
> > changes we did on arm64. There could be assumptions that were not
> > exercised but I'm not sure how to check for all the possible usages.
> > 
> > Do you have any other suggestions that can help improve confidence in
> > the patch?
> 
> Unfortunatelly I don't. I just know there were many subtle assumptions
> all over the place so I am rather careful to not touch the code unless
> really necessary.
> 
> That being said, I am not opposing your patch.

Let me be more specific. I am not opposing your patch but we should
definitely need more reviewers to have a look. I am not seeing any
immediate problems with it but I do not see a large improvements either
(slightly less nightmare doesn't make me sleep all that well ;)). So I
will leave the decisions to others.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1697103

Michal Hocko <mhocko@kernel.org> writes:

> On Wed 26-07-17 14:33:57, Michal Hocko wrote:
>> On Wed 26-07-17 13:11:46, Punit Agrawal wrote:
> [...]
>> > I've been running tests from mce-test suite and libhugetlbfs for similar
>> > changes we did on arm64. There could be assumptions that were not
>> > exercised but I'm not sure how to check for all the possible usages.
>> > 
>> > Do you have any other suggestions that can help improve confidence in
>> > the patch?
>> 
>> Unfortunatelly I don't. I just know there were many subtle assumptions
>> all over the place so I am rather careful to not touch the code unless
>> really necessary.
>> 
>> That being said, I am not opposing your patch.
>
> Let me be more specific. I am not opposing your patch but we should
> definitely need more reviewers to have a look. I am not seeing any
> immediate problems with it but I do not see a large improvements either
> (slightly less nightmare doesn't make me sleep all that well ;)). So I
> will leave the decisions to others.

I hear you - I'd definitely appreciate more eyes on the code change and
description.

Thanks for taking a look.

[toc] | [prev] | [next] | [standalone]

#1697688 — Re: [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and document behaviour

On 07/26/2017 06:34 AM, Punit Agrawal wrote:
> Michal Hocko <mhocko@kernel.org> writes:
> 
>> On Wed 26-07-17 14:33:57, Michal Hocko wrote:
>>> On Wed 26-07-17 13:11:46, Punit Agrawal wrote:
>> [...]
>>>> I've been running tests from mce-test suite and libhugetlbfs for similar
>>>> changes we did on arm64. There could be assumptions that were not
>>>> exercised but I'm not sure how to check for all the possible usages.
>>>>
>>>> Do you have any other suggestions that can help improve confidence in
>>>> the patch?
>>>
>>> Unfortunatelly I don't. I just know there were many subtle assumptions
>>> all over the place so I am rather careful to not touch the code unless
>>> really necessary.
>>>
>>> That being said, I am not opposing your patch.
>>
>> Let me be more specific. I am not opposing your patch but we should
>> definitely need more reviewers to have a look. I am not seeing any
>> immediate problems with it but I do not see a large improvements either
>> (slightly less nightmare doesn't make me sleep all that well ;)). So I
>> will leave the decisions to others.
> 
> I hear you - I'd definitely appreciate more eyes on the code change and
> description.

I like the change in semantics for the routine.  Like you, I examined all
callers of huge_pte_offset() and it appears that they will not be impacted
by your change.

My only concern is that arch specific versions of huge_pte_offset, may
not (yet) follow the new semantic.  Someone could potentially introduce
a new huge_pte_offset call and depend on the new 'documented' semantics.
Yet, an unmodified arch specific version of huge_pte_offset might have
different semantics.  I have not reviewed all the arch specific instances
of the routine to know if this is even possible.  Just curious if you
examined these, or perhaps you think this is not an issue?

-- 
Mike Kravetz

[toc] | [prev] | [next] | [standalone]

#1697962

Mike Kravetz <mike.kravetz@oracle.com> writes:

> On 07/26/2017 06:34 AM, Punit Agrawal wrote:
>> Michal Hocko <mhocko@kernel.org> writes:
>> 
>>> On Wed 26-07-17 14:33:57, Michal Hocko wrote:
>>>> On Wed 26-07-17 13:11:46, Punit Agrawal wrote:
>>> [...]
>>>>> I've been running tests from mce-test suite and libhugetlbfs for similar
>>>>> changes we did on arm64. There could be assumptions that were not
>>>>> exercised but I'm not sure how to check for all the possible usages.
>>>>>
>>>>> Do you have any other suggestions that can help improve confidence in
>>>>> the patch?
>>>>
>>>> Unfortunatelly I don't. I just know there were many subtle assumptions
>>>> all over the place so I am rather careful to not touch the code unless
>>>> really necessary.
>>>>
>>>> That being said, I am not opposing your patch.
>>>
>>> Let me be more specific. I am not opposing your patch but we should
>>> definitely need more reviewers to have a look. I am not seeing any
>>> immediate problems with it but I do not see a large improvements either
>>> (slightly less nightmare doesn't make me sleep all that well ;)). So I
>>> will leave the decisions to others.
>> 
>> I hear you - I'd definitely appreciate more eyes on the code change and
>> description.
>
> I like the change in semantics for the routine.  Like you, I examined all
> callers of huge_pte_offset() and it appears that they will not be impacted
> by your change.
>
> My only concern is that arch specific versions of huge_pte_offset, may
> not (yet) follow the new semantic.  Someone could potentially introduce
> a new huge_pte_offset call and depend on the new 'documented' semantics.
> Yet, an unmodified arch specific version of huge_pte_offset might have
> different semantics.  I have not reviewed all the arch specific instances
> of the routine to know if this is even possible.  Just curious if you
> examined these, or perhaps you think this is not an issue?

From checking through the implementations of huge_pte_offset()
architectures, the change shouldn't break anything. (I also cc'd the
posting to linux-arch for architecture maintainers to take more notice).

This is because existing users actively deal with the different returned
values (NULL, huge pte_t*, swap pte_t*) and are not checking explicitly
for pmd or pud.

Guarding against future users is more tricky - it would definitely help
to align all the implementations.

[toc] | [prev] | [next] | [standalone]

#1696902 — Re: [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and document behaviour

On Tue 25-07-17 16:41:14, Punit Agrawal wrote:
> When walking the page tables to resolve an address that points to
> !p*d_present() entry, huge_pte_offset() returns inconsistent values
> depending on the level of page table (PUD or PMD).
> 
> It returns NULL in the case of a PUD entry while in the case of a PMD
> entry, it returns a pointer to the page table entry.
> 
> A similar inconsitency exists when handling swap entries - returns NULL
> for a PUD entry while a pointer to the pte_t is retured for the PMD
> entry.
> 
> Update huge_pte_offset() to make the behaviour consistent - return NULL
> in the case of p*d_none() and a pointer to the pte_t for hugepage or
> swap entries.
> 
> Document the behaviour to clarify the expected behaviour of this
> function. This is to set clear semantics for architecture specific
> implementations of huge_pte_offset().

hugetlb pte semantic is a disaster and I agree it could see some
cleanup/clarifications but I am quite nervous to see a patchi like this.
How do we check that nothing will get silently broken by this change?

> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> ---
>  mm/hugetlb.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index bc48ee783dd9..72dd1139a8e4 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4603,6 +4603,13 @@ pte_t *huge_pte_alloc(struct mm_struct *mm,
>  	return pte;
>  }
>  
> +/*
> + * huge_pte_offset() - Walk the page table to resolve the hugepage
> + * entry at address @addr
> + *
> + * Return: Pointer to page table or swap entry (PUD or PMD) for address @addr
> + * or NULL if the entry is p*d_none().
> + */
>  pte_t *huge_pte_offset(struct mm_struct *mm,
>  		       unsigned long addr, unsigned long sz)
>  {
> @@ -4617,13 +4624,22 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>  	p4d = p4d_offset(pgd, addr);
>  	if (!p4d_present(*p4d))
>  		return NULL;
> +
>  	pud = pud_offset(p4d, addr);
> -	if (!pud_present(*pud))
> +	if (pud_none(*pud))
>  		return NULL;
> -	if (pud_huge(*pud))
> +	/* hugepage or swap? */
> +	if (pud_huge(*pud) || !pud_present(*pud))
>  		return (pte_t *)pud;
> +
>  	pmd = pmd_offset(pud, addr);
> -	return (pte_t *) pmd;
> +	if (pmd_none(*pmd))
> +		return NULL;
> +	/* hugepage or swap? */
> +	if (pmd_huge(*pmd) || !pmd_present(*pmd))
> +		return (pte_t *) pmd;
> +
> +	return NULL;
>  }
>  
>  #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
> -- 
> 2.11.0
> 

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web