* [PATCH] mm/hugetlb: avoid unnecessary check on pud and pmd entry in huge_pte_offset
@ 2020-04-23 12:49 Li Xinhai
2020-04-23 13:12 ` Li Xinhai
2020-04-23 18:14 ` Mike Kravetz
0 siblings, 2 replies; 11+ messages in thread
From: Li Xinhai @ 2020-04-23 12:49 UTC (permalink / raw)
To: linux-mm; +Cc: Mike Kravetz, Andrew Morton
When huge_pte_offset() is called, the parameter sz can only be PUD_SIZE
or PMD_SIZE.
If sz is PUD_SIZE and code can reach pud, then *pud must be none, or
normal hugetlb entry, or non-present (migration or hwpoisoned) hugetlb
entry, and we can directly return pud.
When sz is PMD_SIZE, pud must be none or present, and if code can reach
pmd, we can directly return pmd.
So, after this patch, the code is simplified by first check on the
parameter sz, and avoid unnecessary checks in current code.
Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
mm/hugetlb.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index bcabbe0..e1424f5 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5365,8 +5365,8 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
{
pgd_t *pgd;
p4d_t *p4d;
- pud_t *pud, pud_entry;
- pmd_t *pmd, pmd_entry;
+ pud_t *pud;
+ pmd_t *pmd;
pgd = pgd_offset(mm, addr);
if (!pgd_present(*pgd))
@@ -5376,22 +5376,16 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
return NULL;
pud = pud_offset(p4d, addr);
- pud_entry = READ_ONCE(*pud);
- if (sz != PUD_SIZE && pud_none(pud_entry))
- return NULL;
- /* hugepage or swap? */
- if (pud_huge(pud_entry) || !pud_present(pud_entry))
+ if (sz == PUD_SIZE)
+ /* must be pud_huge or pud_none */
return (pte_t *)pud;
-
- pmd = pmd_offset(pud, addr);
- pmd_entry = READ_ONCE(*pmd);
- if (sz != PMD_SIZE && pmd_none(pmd_entry))
+ if (!pud_present(*pud))
return NULL;
- /* hugepage or swap? */
- if (pmd_huge(pmd_entry) || !pmd_present(pmd_entry))
- return (pte_t *)pmd;
+ /* must have a valid entry and size to go further */
- return NULL;
+ pmd = pmd_offset(pud, addr);
+ /* must be pmd_huge or pmd_none */
+ return (pte_t *)pmd;
}
#endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/hugetlb: avoid unnecessary check on pud and pmd entry in huge_pte_offset
2020-04-23 12:49 [PATCH] mm/hugetlb: avoid unnecessary check on pud and pmd entry in huge_pte_offset Li Xinhai
@ 2020-04-23 13:12 ` Li Xinhai
2020-04-23 18:14 ` Mike Kravetz
1 sibling, 0 replies; 11+ messages in thread
From: Li Xinhai @ 2020-04-23 13:12 UTC (permalink / raw)
To: linux-mm; +Cc: Mike Kravetz, akpm
On 2020-04-23 at 20:49 Li Xinhai wrote:
>When huge_pte_offset() is called, the parameter sz can only be PUD_SIZE
>or PMD_SIZE.
>If sz is PUD_SIZE and code can reach pud, then *pud must be none, or
>normal hugetlb entry, or non-present (migration or hwpoisoned) hugetlb
>entry, and we can directly return pud.
>When sz is PMD_SIZE, pud must be none or present, and if code can reach
>pmd, we can directly return pmd.
>
>So, after this patch, the code is simplified by first check on the
>parameter sz, and avoid unnecessary checks in current code.
>
>Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
>Cc: Mike Kravetz <mike.kravetz@oracle.com>
>Cc: Andrew Morton <akpm@linux-foundation.org>
Since this huge_pte_offset() is under CONFIG_ARCH_WANT_GENERAL_HUGETLB,
I only have chance to test with x86_64, but the logical should hold for all other
cases which using this general implementation.
The exsiting code path was introduced by commit 9b19df292c6 (mm/hugetlb.c:
make huge_pte_offset() consistent and document behaviour), and the sematics
is maintained after current patch applied.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/hugetlb: avoid unnecessary check on pud and pmd entry in huge_pte_offset
2020-04-23 12:49 [PATCH] mm/hugetlb: avoid unnecessary check on pud and pmd entry in huge_pte_offset Li Xinhai
2020-04-23 13:12 ` Li Xinhai
@ 2020-04-23 18:14 ` Mike Kravetz
2020-04-23 18:38 ` Jason Gunthorpe
1 sibling, 1 reply; 11+ messages in thread
From: Mike Kravetz @ 2020-04-23 18:14 UTC (permalink / raw)
To: Li Xinhai, linux-mm
Cc: Andrew Morton, Punit Agrawal, Longpeng, Jason Gunthorpe
Cc a few people who have looked at huge_pte_offset() recently.
On 4/23/20 5:49 AM, Li Xinhai wrote:
> When huge_pte_offset() is called, the parameter sz can only be PUD_SIZE
> or PMD_SIZE.
> If sz is PUD_SIZE and code can reach pud, then *pud must be none, or
> normal hugetlb entry, or non-present (migration or hwpoisoned) hugetlb
> entry, and we can directly return pud.
> When sz is PMD_SIZE, pud must be none or present, and if code can reach
> pmd, we can directly return pmd.
>
> So, after this patch, the code is simplified by first check on the
> parameter sz, and avoid unnecessary checks in current code.
>
> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
> mm/hugetlb.c | 24 +++++++++---------------
> 1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index bcabbe0..e1424f5 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5365,8 +5365,8 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
> {
> pgd_t *pgd;
> p4d_t *p4d;
> - pud_t *pud, pud_entry;
> - pmd_t *pmd, pmd_entry;
> + pud_t *pud;
> + pmd_t *pmd;
>
> pgd = pgd_offset(mm, addr);
> if (!pgd_present(*pgd))
> @@ -5376,22 +5376,16 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
> return NULL;
>
> pud = pud_offset(p4d, addr);
> - pud_entry = READ_ONCE(*pud);
> - if (sz != PUD_SIZE && pud_none(pud_entry))
> - return NULL;
> - /* hugepage or swap? */
> - if (pud_huge(pud_entry) || !pud_present(pud_entry))
> + if (sz == PUD_SIZE)
> + /* must be pud_huge or pud_none */
> return (pte_t *)pud;
> -
> - pmd = pmd_offset(pud, addr);
> - pmd_entry = READ_ONCE(*pmd);
> - if (sz != PMD_SIZE && pmd_none(pmd_entry))
> + if (!pud_present(*pud))
> return NULL;
> - /* hugepage or swap? */
> - if (pmd_huge(pmd_entry) || !pmd_present(pmd_entry))
> - return (pte_t *)pmd;
> + /* must have a valid entry and size to go further */
>
> - return NULL;
> + pmd = pmd_offset(pud, addr);
Can we get here with sz = PMD_SIZE and pud_none(*pud)? Would that be
an issue for the pmd_offset() call?
--
Mike Kravetz
> + /* must be pmd_huge or pmd_none */
> + return (pte_t *)pmd;
> }
>
> #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/hugetlb: avoid unnecessary check on pud and pmd entry in huge_pte_offset
2020-04-23 18:14 ` Mike Kravetz
@ 2020-04-23 18:38 ` Jason Gunthorpe
2020-04-24 4:07 ` Li Xinhai
0 siblings, 1 reply; 11+ messages in thread
From: Jason Gunthorpe @ 2020-04-23 18:38 UTC (permalink / raw)
To: Mike Kravetz; +Cc: Li Xinhai, linux-mm, Andrew Morton, Punit Agrawal, Longpeng
On Thu, Apr 23, 2020 at 11:14:28AM -0700, Mike Kravetz wrote:
> Cc a few people who have looked at huge_pte_offset() recently.
>
> On 4/23/20 5:49 AM, Li Xinhai wrote:
> > When huge_pte_offset() is called, the parameter sz can only be PUD_SIZE
> > or PMD_SIZE.
> > If sz is PUD_SIZE and code can reach pud, then *pud must be none, or
> > normal hugetlb entry, or non-present (migration or hwpoisoned) hugetlb
> > entry, and we can directly return pud.
> > When sz is PMD_SIZE, pud must be none or present, and if code can reach
> > pmd, we can directly return pmd.
> >
> > So, after this patch, the code is simplified by first check on the
> > parameter sz, and avoid unnecessary checks in current code.
> >
> > Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
> > Cc: Mike Kravetz <mike.kravetz@oracle.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > mm/hugetlb.c | 24 +++++++++---------------
> > 1 file changed, 9 insertions(+), 15 deletions(-)
> >
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index bcabbe0..e1424f5 100644
> > +++ b/mm/hugetlb.c
> > @@ -5365,8 +5365,8 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
> > {
> > pgd_t *pgd;
> > p4d_t *p4d;
> > - pud_t *pud, pud_entry;
> > - pmd_t *pmd, pmd_entry;
> > + pud_t *pud;
> > + pmd_t *pmd;
> >
> > pgd = pgd_offset(mm, addr);
> > if (!pgd_present(*pgd))
> > @@ -5376,22 +5376,16 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
> > return NULL;
> >
> > pud = pud_offset(p4d, addr);
> > - pud_entry = READ_ONCE(*pud);
> > - if (sz != PUD_SIZE && pud_none(pud_entry))
> > - return NULL;
> > - /* hugepage or swap? */
> > - if (pud_huge(pud_entry) || !pud_present(pud_entry))
> > + if (sz == PUD_SIZE)
> > + /* must be pud_huge or pud_none */
> > return (pte_t *)pud;
> > -
> > - pmd = pmd_offset(pud, addr);
> > - pmd_entry = READ_ONCE(*pmd);
> > - if (sz != PMD_SIZE && pmd_none(pmd_entry))
> > + if (!pud_present(*pud))
> > return NULL;
> > - /* hugepage or swap? */
> > - if (pmd_huge(pmd_entry) || !pmd_present(pmd_entry))
> > - return (pte_t *)pmd;
> > + /* must have a valid entry and size to go further */
> >
> > - return NULL;
> > + pmd = pmd_offset(pud, addr);
>
> Can we get here with sz = PMD_SIZE and pud_none(*pud)? Would that be
> an issue for the pmd_offset() call?
Certainly pmd_offset() must only be called if the PUD entry is
pointing at a pmd level.
AFAIK this means it should not be called on pud_none(), pud_huge() or
!pud_present() cases.
Jason
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/hugetlb: avoid unnecessary check on pud and pmd entry in huge_pte_offset
2020-04-23 18:38 ` Jason Gunthorpe
@ 2020-04-24 4:07 ` Li Xinhai
2020-04-24 12:57 ` Jason Gunthorpe
0 siblings, 1 reply; 11+ messages in thread
From: Li Xinhai @ 2020-04-24 4:07 UTC (permalink / raw)
To: Jason Gunthorpe, Mike Kravetz; +Cc: linux-mm, akpm, Punit Agrawal, Longpeng
On 2020-04-24 at 02:38 Jason Gunthorpe wrote:
>On Thu, Apr 23, 2020 at 11:14:28AM -0700, Mike Kravetz wrote:
>> Cc a few people who have looked at huge_pte_offset() recently.
>>
>> On 4/23/20 5:49 AM, Li Xinhai wrote:
>> > When huge_pte_offset() is called, the parameter sz can only be PUD_SIZE
>> > or PMD_SIZE.
>> > If sz is PUD_SIZE and code can reach pud, then *pud must be none, or
>> > normal hugetlb entry, or non-present (migration or hwpoisoned) hugetlb
>> > entry, and we can directly return pud.
>> > When sz is PMD_SIZE, pud must be none or present, and if code can reach
>> > pmd, we can directly return pmd.
>> >
>> > So, after this patch, the code is simplified by first check on the
>> > parameter sz, and avoid unnecessary checks in current code.
>> >
>> > Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
>> > Cc: Mike Kravetz <mike.kravetz@oracle.com>
>> > Cc: Andrew Morton <akpm@linux-foundation.org>
>> > mm/hugetlb.c | 24 +++++++++---------------
>> > 1 file changed, 9 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> > index bcabbe0..e1424f5 100644
>> > +++ b/mm/hugetlb.c
>> > @@ -5365,8 +5365,8 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>> > {
>> > pgd_t *pgd;
>> > p4d_t *p4d;
>> > - pud_t *pud, pud_entry;
>> > - pmd_t *pmd, pmd_entry;
>> > + pud_t *pud;
>> > + pmd_t *pmd;
>> >
>> > pgd = pgd_offset(mm, addr);
>> > if (!pgd_present(*pgd))
>> > @@ -5376,22 +5376,16 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>> > return NULL;
>> >
>> > pud = pud_offset(p4d, addr);
>> > - pud_entry = READ_ONCE(*pud);
>> > - if (sz != PUD_SIZE && pud_none(pud_entry))
>> > - return NULL;
>> > - /* hugepage or swap? */
>> > - if (pud_huge(pud_entry) || !pud_present(pud_entry))
>> > + if (sz == PUD_SIZE)
>> > + /* must be pud_huge or pud_none */
>> > return (pte_t *)pud;
>> > -
>> > - pmd = pmd_offset(pud, addr);
>> > - pmd_entry = READ_ONCE(*pmd);
>> > - if (sz != PMD_SIZE && pmd_none(pmd_entry))
>> > + if (!pud_present(*pud))
>> > return NULL;
>> > - /* hugepage or swap? */
>> > - if (pmd_huge(pmd_entry) || !pmd_present(pmd_entry))
>> > - return (pte_t *)pmd;
>> > + /* must have a valid entry and size to go further */
>> >
>> > - return NULL;
>> > + pmd = pmd_offset(pud, addr);
>>
>> Can we get here with sz = PMD_SIZE and pud_none(*pud)? Would that be
>> an issue for the pmd_offset() call?
>
>Certainly pmd_offset() must only be called if the PUD entry is
>pointing at a pmd level.
>
>AFAIK this means it should not be called on pud_none(), pud_huge() or
>!pud_present() cases.
The test of !pud_present(*pud) also block pud_none(*pud), so when sz ==
PMD_SIZE, pmd_offset() only called with a valid PUD entry which point to PMD
page table.
>
>Jason
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/hugetlb: avoid unnecessary check on pud and pmd entry in huge_pte_offset
2020-04-24 4:07 ` Li Xinhai
@ 2020-04-24 12:57 ` Jason Gunthorpe
2020-04-24 13:33 ` Li Xinhai
0 siblings, 1 reply; 11+ messages in thread
From: Jason Gunthorpe @ 2020-04-24 12:57 UTC (permalink / raw)
To: Li Xinhai; +Cc: Mike Kravetz, linux-mm, akpm, Punit Agrawal, Longpeng
On Fri, Apr 24, 2020 at 12:07:50PM +0800, Li Xinhai wrote:
> On 2020-04-24 at 02:38 Jason Gunthorpe wrote:
> >On Thu, Apr 23, 2020 at 11:14:28AM -0700, Mike Kravetz wrote:
> >> Cc a few people who have looked at huge_pte_offset() recently.
> >>
> >> On 4/23/20 5:49 AM, Li Xinhai wrote:
> >> > When huge_pte_offset() is called, the parameter sz can only be PUD_SIZE
> >> > or PMD_SIZE.
> >> > If sz is PUD_SIZE and code can reach pud, then *pud must be none, or
> >> > normal hugetlb entry, or non-present (migration or hwpoisoned) hugetlb
> >> > entry, and we can directly return pud.
> >> > When sz is PMD_SIZE, pud must be none or present, and if code can reach
> >> > pmd, we can directly return pmd.
> >> >
> >> > So, after this patch, the code is simplified by first check on the
> >> > parameter sz, and avoid unnecessary checks in current code.
> >> >
> >> > Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
> >> > Cc: Mike Kravetz <mike.kravetz@oracle.com>
> >> > Cc: Andrew Morton <akpm@linux-foundation.org>
> >> > mm/hugetlb.c | 24 +++++++++---------------
> >> > 1 file changed, 9 insertions(+), 15 deletions(-)
> >> >
> >> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> >> > index bcabbe0..e1424f5 100644
> >> > +++ b/mm/hugetlb.c
> >> > @@ -5365,8 +5365,8 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
> >> > {
> >> > pgd_t *pgd;
> >> > p4d_t *p4d;
> >> > - pud_t *pud, pud_entry;
> >> > - pmd_t *pmd, pmd_entry;
> >> > + pud_t *pud;
> >> > + pmd_t *pmd;
> >> >
> >> > pgd = pgd_offset(mm, addr);
> >> > if (!pgd_present(*pgd))
> >> > @@ -5376,22 +5376,16 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
> >> > return NULL;
> >> >
> >> > pud = pud_offset(p4d, addr);
> >> > - pud_entry = READ_ONCE(*pud);
> >> > - if (sz != PUD_SIZE && pud_none(pud_entry))
> >> > - return NULL;
> >> > - /* hugepage or swap? */
> >> > - if (pud_huge(pud_entry) || !pud_present(pud_entry))
> >> > + if (sz == PUD_SIZE)
> >> > + /* must be pud_huge or pud_none */
> >> > return (pte_t *)pud;
> >> > -
> >> > - pmd = pmd_offset(pud, addr);
> >> > - pmd_entry = READ_ONCE(*pmd);
> >> > - if (sz != PMD_SIZE && pmd_none(pmd_entry))
> >> > + if (!pud_present(*pud))
> >> > return NULL;
> >> > - /* hugepage or swap? */
> >> > - if (pmd_huge(pmd_entry) || !pmd_present(pmd_entry))
> >> > - return (pte_t *)pmd;
> >> > + /* must have a valid entry and size to go further */
> >> >
> >> > - return NULL;
> >> > + pmd = pmd_offset(pud, addr);
> >>
> >> Can we get here with sz = PMD_SIZE and pud_none(*pud)? Would that be
> >> an issue for the pmd_offset() call?
> >
> >Certainly pmd_offset() must only be called if the PUD entry is
> >pointing at a pmd level.
> >
> >AFAIK this means it should not be called on pud_none(), pud_huge() or
> >!pud_present() cases.
>
> The test of !pud_present(*pud) also block pud_none(*pud)
Sure
> , so when sz == PMD_SIZE, pmd_offset() only called with a valid PUD
> entry which point to PMD page table.
But what prevents pud_huge?
This API seems kind of strange to be honest.. Should it be two
functions instead of a sz parameter?
huge_pud_offset() and huge_pmd_offset() ?
Jason
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/hugetlb: avoid unnecessary check on pud and pmd entry in huge_pte_offset
2020-04-24 12:57 ` Jason Gunthorpe
@ 2020-04-24 13:33 ` Li Xinhai
2020-04-24 13:42 ` Jason Gunthorpe
0 siblings, 1 reply; 11+ messages in thread
From: Li Xinhai @ 2020-04-24 13:33 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: Mike Kravetz, linux-mm, akpm, Punit Agrawal, Longpeng
On 2020-04-24 at 20:57 Jason Gunthorpe wrote:
>On Fri, Apr 24, 2020 at 12:07:50PM +0800, Li Xinhai wrote:
>> On 2020-04-24 at 02:38 Jason Gunthorpe wrote:
>> >On Thu, Apr 23, 2020 at 11:14:28AM -0700, Mike Kravetz wrote:
>> >> Cc a few people who have looked at huge_pte_offset() recently.
>> >>
>> >> On 4/23/20 5:49 AM, Li Xinhai wrote:
>> >> > When huge_pte_offset() is called, the parameter sz can only be PUD_SIZE
>> >> > or PMD_SIZE.
>> >> > If sz is PUD_SIZE and code can reach pud, then *pud must be none, or
>> >> > normal hugetlb entry, or non-present (migration or hwpoisoned) hugetlb
>> >> > entry, and we can directly return pud.
>> >> > When sz is PMD_SIZE, pud must be none or present, and if code can reach
>> >> > pmd, we can directly return pmd.
>> >> >
>> >> > So, after this patch, the code is simplified by first check on the
>> >> > parameter sz, and avoid unnecessary checks in current code.
>> >> >
>> >> > Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
>> >> > Cc: Mike Kravetz <mike.kravetz@oracle.com>
>> >> > Cc: Andrew Morton <akpm@linux-foundation.org>
>> >> > mm/hugetlb.c | 24 +++++++++---------------
>> >> > 1 file changed, 9 insertions(+), 15 deletions(-)
>> >> >
>> >> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> >> > index bcabbe0..e1424f5 100644
>> >> > +++ b/mm/hugetlb.c
>> >> > @@ -5365,8 +5365,8 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>> >> > {
>> >> > pgd_t *pgd;
>> >> > p4d_t *p4d;
>> >> > - pud_t *pud, pud_entry;
>> >> > - pmd_t *pmd, pmd_entry;
>> >> > + pud_t *pud;
>> >> > + pmd_t *pmd;
>> >> >
>> >> > pgd = pgd_offset(mm, addr);
>> >> > if (!pgd_present(*pgd))
>> >> > @@ -5376,22 +5376,16 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>> >> > return NULL;
>> >> >
>> >> > pud = pud_offset(p4d, addr);
>> >> > - pud_entry = READ_ONCE(*pud);
>> >> > - if (sz != PUD_SIZE && pud_none(pud_entry))
>> >> > - return NULL;
>> >> > - /* hugepage or swap? */
>> >> > - if (pud_huge(pud_entry) || !pud_present(pud_entry))
>> >> > + if (sz == PUD_SIZE)
>> >> > + /* must be pud_huge or pud_none */
>> >> > return (pte_t *)pud;
>> >> > -
>> >> > - pmd = pmd_offset(pud, addr);
>> >> > - pmd_entry = READ_ONCE(*pmd);
>> >> > - if (sz != PMD_SIZE && pmd_none(pmd_entry))
>> >> > + if (!pud_present(*pud))
>> >> > return NULL;
>> >> > - /* hugepage or swap? */
>> >> > - if (pmd_huge(pmd_entry) || !pmd_present(pmd_entry))
>> >> > - return (pte_t *)pmd;
>> >> > + /* must have a valid entry and size to go further */
>> >> >
>> >> > - return NULL;
>> >> > + pmd = pmd_offset(pud, addr);
>> >>
>> >> Can we get here with sz = PMD_SIZE and pud_none(*pud)? Would that be
>> >> an issue for the pmd_offset() call?
>> >
>> >Certainly pmd_offset() must only be called if the PUD entry is
>> >pointing at a pmd level.
>> >
>> >AFAIK this means it should not be called on pud_none(), pud_huge() or
>> >!pud_present() cases.
>>
>> The test of !pud_present(*pud) also block pud_none(*pud)
>
>Sure
>
>> , so when sz == PMD_SIZE, pmd_offset() only called with a valid PUD
>> entry which point to PMD page table.
>
>But what prevents pud_huge?
>
if sz == PUD_SIZE, the 'return (pte_t*)pud' alrady end the function, which cover
pud_huge() and pud_none(), because we the mapping is for PUD_SIZE huge page.
So, there is no possibility for pmd_offset() been called with invalid pud entry.
Below is the code I used for test which has BUG_ON, that should give more
clear idea about the semantics of code path:
...
pud = pud_offset(p4d, addr);
if (sz == PUD_SIZE) {
/* must be pud_huge or pud_none */
BUG_ON(!pud_huge(*pud) && !pud_none(*pud));
return (pte_t *)pud; // note that return valid pointer for pud_none() case,
// instead of NULL, that is same semantics as existing code.
}
if (!pud_present(*pud))
return NULL; // note that only return NULL in case pud not present,
// same sematics as existing code.
/* must have a valid entry and size to go further */
BUG_ON(sz != PMD_SIZE);
pmd = pmd_offset(pud, addr);
/* must be pmd_huge or pmd_none */
BUG_ON(!pmd_huge(*pmd) && !pmd_none(*pmd));
return (pte_t *)pmd; // note that return valid pointer for pmd_none() case,
// instead of NULL, that is same semantics as existing code.
...
>This API seems kind of strange to be honest.. Should it be two
>functions instead of a sz parameter?
>
>huge_pud_offset() and huge_pmd_offset() ?
I think checking huge size then call to one of these two functions at caller
site will involve many redundant code do branch work in one function is
better.
>
>Jason
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/hugetlb: avoid unnecessary check on pud and pmd entry in huge_pte_offset
2020-04-24 13:33 ` Li Xinhai
@ 2020-04-24 13:42 ` Jason Gunthorpe
2020-04-24 14:07 ` Li Xinhai
0 siblings, 1 reply; 11+ messages in thread
From: Jason Gunthorpe @ 2020-04-24 13:42 UTC (permalink / raw)
To: Li Xinhai; +Cc: Mike Kravetz, linux-mm, akpm, Punit Agrawal, Longpeng
On Fri, Apr 24, 2020 at 09:33:40PM +0800, Li Xinhai wrote:
> >
> >> , so when sz == PMD_SIZE, pmd_offset() only called with a valid PUD
> >> entry which point to PMD page table.
> >
> >But what prevents pud_huge?
> >
> if sz == PUD_SIZE, the 'return (pte_t*)pud' alrady end the function, which cover
> pud_huge() and pud_none(), because we the mapping is for PUD_SIZE huge page.
>
> So, there is no possibility for pmd_offset() been called with invalid pud entry.
> Below is the code I used for test which has BUG_ON, that should give more
> clear idea about the semantics of code path:
>
> ...
> pud = pud_offset(p4d, addr);
> if (sz == PUD_SIZE) {
> /* must be pud_huge or pud_none */
> BUG_ON(!pud_huge(*pud) && !pud_none(*pud));
> return (pte_t *)pud; // note that return valid pointer for pud_none() case,
> // instead of NULL, that is same semantics as existing code.
> }
> if (!pud_present(*pud))
> return NULL; // note that only return NULL in case pud not present,
> // same sematics as existing code.
> /* must have a valid entry and size to go further */
> BUG_ON(sz != PMD_SIZE);
>
> pmd = pmd_offset(pud, addr);
> /* must be pmd_huge or pmd_none */
> BUG_ON(!pmd_huge(*pmd) && !pmd_none(*pmd));
But why is !pmd_huge() ? The prior code returned null here, is that
dead code? Your commit message should explain all of this..
Jason
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/hugetlb: avoid unnecessary check on pud and pmd entry in huge_pte_offset
2020-04-24 13:42 ` Jason Gunthorpe
@ 2020-04-24 14:07 ` Li Xinhai
2020-04-24 14:10 ` Jason Gunthorpe
0 siblings, 1 reply; 11+ messages in thread
From: Li Xinhai @ 2020-04-24 14:07 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: Mike Kravetz, linux-mm, akpm, Punit Agrawal, Longpeng
On 2020-04-24 at 21:42 Jason Gunthorpe wrote:
>On Fri, Apr 24, 2020 at 09:33:40PM +0800, Li Xinhai wrote:
>> >
>> >> , so when sz == PMD_SIZE, pmd_offset() only called with a valid PUD
>> >> entry which point to PMD page table.
>> >
>> >But what prevents pud_huge?
>> >
>> if sz == PUD_SIZE, the 'return (pte_t*)pud' alrady end the function, which cover
>> pud_huge() and pud_none(), because we the mapping is for PUD_SIZE huge page.
>>
>> So, there is no possibility for pmd_offset() been called with invalid pud entry.
>> Below is the code I used for test which has BUG_ON, that should give more
>> clear idea about the semantics of code path:
>>
>> ...
>> pud = pud_offset(p4d, addr);
>> if (sz == PUD_SIZE) {
>> /* must be pud_huge or pud_none */
>> BUG_ON(!pud_huge(*pud) && !pud_none(*pud));
>> return (pte_t *)pud; // note that return valid pointer for pud_none() case,
>> // instead of NULL, that is same semantics as existing code.
>> }
>> if (!pud_present(*pud))
>> return NULL; // note that only return NULL in case pud not present,
>> // same sematics as existing code.
>> /* must have a valid entry and size to go further */
>> BUG_ON(sz != PMD_SIZE);
>>
>> pmd = pmd_offset(pud, addr);
>> /* must be pmd_huge or pmd_none */
>> BUG_ON(!pmd_huge(*pmd) && !pmd_none(*pmd));
>
>But why is !pmd_huge() ? The prior code returned null here, is that
>dead code? Your commit message should explain all of this..
>
let's see exising code for pmd part, the reason are in comments:
...
pmd = pmd_offset(pud, addr);
if (sz != PMD_SIZE && pmd_none(*pmd))
return NULL; // dead code, must sz == PMD_SIZE
/* hugepage or swap? */
if (pmd_huge(*pmd) || !pmd_present(*pmd)) // !pmd_present() also cover pmd_none(),
return (pte_t *)pmd; // so, all possible and valid value in pmd entry will reach here.
return NULL; // dead code; can we have (!pmd_huge() && pmd_present()) and reach here?
// no, because this is a hugetlb mapping. otherwise, there is invalid value in pmd entry.
...
>Jason
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/hugetlb: avoid unnecessary check on pud and pmd entry in huge_pte_offset
2020-04-24 14:07 ` Li Xinhai
@ 2020-04-24 14:10 ` Jason Gunthorpe
2020-04-24 14:53 ` Li Xinhai
0 siblings, 1 reply; 11+ messages in thread
From: Jason Gunthorpe @ 2020-04-24 14:10 UTC (permalink / raw)
To: Li Xinhai; +Cc: Mike Kravetz, linux-mm, akpm, Punit Agrawal, Longpeng
On Fri, Apr 24, 2020 at 10:07:19PM +0800, Li Xinhai wrote:
> On 2020-04-24 at 21:42 Jason Gunthorpe wrote:
> >On Fri, Apr 24, 2020 at 09:33:40PM +0800, Li Xinhai wrote:
> >> >
> >> >> , so when sz == PMD_SIZE, pmd_offset() only called with a valid PUD
> >> >> entry which point to PMD page table.
> >> >
> >> >But what prevents pud_huge?
> >> >
> >> if sz == PUD_SIZE, the 'return (pte_t*)pud' alrady end the function, which cover
> >> pud_huge() and pud_none(), because we the mapping is for PUD_SIZE huge page.
> >>
> >> So, there is no possibility for pmd_offset() been called with invalid pud entry.
> >> Below is the code I used for test which has BUG_ON, that should give more
> >> clear idea about the semantics of code path:
> >>
> >> ...
> >> pud = pud_offset(p4d, addr);
> >> if (sz == PUD_SIZE) {
> >> /* must be pud_huge or pud_none */
> >> BUG_ON(!pud_huge(*pud) && !pud_none(*pud));
> >> return (pte_t *)pud; // note that return valid pointer for pud_none() case,
> >> // instead of NULL, that is same semantics as existing code.
> >> }
> >> if (!pud_present(*pud))
> >> return NULL; // note that only return NULL in case pud not present,
> >> // same sematics as existing code.
> >> /* must have a valid entry and size to go further */
> >> BUG_ON(sz != PMD_SIZE);
> >>
> >> pmd = pmd_offset(pud, addr);
> >> /* must be pmd_huge or pmd_none */
> >> BUG_ON(!pmd_huge(*pmd) && !pmd_none(*pmd));
> >
> >But why is !pmd_huge() ? The prior code returned null here, is that
> >dead code? Your commit message should explain all of this..
> >
> let's see exising code for pmd part, the reason are in comments:
> ...
> pmd = pmd_offset(pud, addr);
> if (sz != PMD_SIZE && pmd_none(*pmd))
> return NULL; // dead code, must sz == PMD_SIZE
> /* hugepage or swap? */
> if (pmd_huge(*pmd) || !pmd_present(*pmd)) // !pmd_present() also cover pmd_none(),
> return (pte_t *)pmd; // so, all possible and valid value in pmd entry will reach here.
>
> return NULL; // dead code; can we have (!pmd_huge() && pmd_present()) and reach here?
> // no, because this is a hugetlb mapping. otherwise, there is invalid value in pmd entry.
> ...
well if you are relying on the caller to not call this in wrong cases
it would make sense to have a
if (WARN_ON(!pmd_huge(*pmd)))
return NULL
To document the assertion
Jason
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mm/hugetlb: avoid unnecessary check on pud and pmd entry in huge_pte_offset
2020-04-24 14:10 ` Jason Gunthorpe
@ 2020-04-24 14:53 ` Li Xinhai
0 siblings, 0 replies; 11+ messages in thread
From: Li Xinhai @ 2020-04-24 14:53 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: Mike Kravetz, linux-mm, akpm, Punit Agrawal, Longpeng
On 2020-04-24 at 22:10 Jason Gunthorpe wrote:
>On Fri, Apr 24, 2020 at 10:07:19PM +0800, Li Xinhai wrote:
>> On 2020-04-24 at 21:42 Jason Gunthorpe wrote:
>> >On Fri, Apr 24, 2020 at 09:33:40PM +0800, Li Xinhai wrote:
>> >> >
>> >> >> , so when sz == PMD_SIZE, pmd_offset() only called with a valid PUD
>> >> >> entry which point to PMD page table.
>> >> >
>> >> >But what prevents pud_huge?
>> >> >
>> >> if sz == PUD_SIZE, the 'return (pte_t*)pud' alrady end the function, which cover
>> >> pud_huge() and pud_none(), because we the mapping is for PUD_SIZE huge page.
>> >>
>> >> So, there is no possibility for pmd_offset() been called with invalid pud entry.
>> >> Below is the code I used for test which has BUG_ON, that should give more
>> >> clear idea about the semantics of code path:
>> >>
>> >> ...
>> >> pud = pud_offset(p4d, addr);
>> >> if (sz == PUD_SIZE) {
>> >> /* must be pud_huge or pud_none */
>> >> BUG_ON(!pud_huge(*pud) && !pud_none(*pud));
>> >> return (pte_t *)pud; // note that return valid pointer for pud_none() case,
>> >> // instead of NULL, that is same semantics as existing code.
>> >> }
>> >> if (!pud_present(*pud))
>> >> return NULL; // note that only return NULL in case pud not present,
>> >> // same sematics as existing code.
>> >> /* must have a valid entry and size to go further */
>> >> BUG_ON(sz != PMD_SIZE);
>> >>
>> >> pmd = pmd_offset(pud, addr);
>> >> /* must be pmd_huge or pmd_none */
>> >> BUG_ON(!pmd_huge(*pmd) && !pmd_none(*pmd));
>> >
>> >But why is !pmd_huge() ? The prior code returned null here, is that
>> >dead code? Your commit message should explain all of this..
>> >
>> let's see exising code for pmd part, the reason are in comments:
>> ...
>> pmd = pmd_offset(pud, addr);
>> if (sz != PMD_SIZE && pmd_none(*pmd))
>> return NULL; // dead code, must sz == PMD_SIZE
>> /* hugepage or swap? */
>> if (pmd_huge(*pmd) || !pmd_present(*pmd)) // !pmd_present() also cover pmd_none(),
>> return (pte_t *)pmd; // so, all possible and valid value in pmd entry will reach here.
>>
>> return NULL; // dead code; can we have (!pmd_huge() && pmd_present()) and reach here?
>> // no, because this is a hugetlb mapping. otherwise, there is invalid value in pmd entry.
>> ...
>
>well if you are relying on the caller to not call this in wrong cases
>it would make sense to have a
>
>if (WARN_ON(!pmd_huge(*pmd)))
> return NULL
>
>To document the assertion
>
right, if this WARN_ON occurs, which means huge_pte_offset() been called
for a normal 4K mapping, that is a bug of caller. After inspected current code
of callers, no one tries to call it on 4K mapping.
>Jason
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-04-24 14:53 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-23 12:49 [PATCH] mm/hugetlb: avoid unnecessary check on pud and pmd entry in huge_pte_offset Li Xinhai
2020-04-23 13:12 ` Li Xinhai
2020-04-23 18:14 ` Mike Kravetz
2020-04-23 18:38 ` Jason Gunthorpe
2020-04-24 4:07 ` Li Xinhai
2020-04-24 12:57 ` Jason Gunthorpe
2020-04-24 13:33 ` Li Xinhai
2020-04-24 13:42 ` Jason Gunthorpe
2020-04-24 14:07 ` Li Xinhai
2020-04-24 14:10 ` Jason Gunthorpe
2020-04-24 14:53 ` Li Xinhai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).