* [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).