| Message ID | 20210308112809.26107-5-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some cleanups for hugetlb | expand |
On 3/8/21 3:28 AM, Miaohe Lin wrote: > Rework the error handling code when alloc_huge_page() failed to remove some > duplicated code and simplify the code slightly. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/hugetlb.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 695603071f2c..69b8de866a24 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -4337,13 +4337,10 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, > * sure there really is no pte entry. > */ > ptl = huge_pte_lock(h, mm, ptep); > - if (!huge_pte_none(huge_ptep_get(ptep))) { > - ret = 0; > - spin_unlock(ptl); > - goto out; > - } > + ret = 0; > + if (huge_pte_none(huge_ptep_get(ptep))) > + ret = vmf_error(PTR_ERR(page)); This new code is simpler. The !huge_pte_none() catches an unlikely race. IMO, the existing code made that very clear. Would have been even more clear with an unlikely modifier. In any case, the lengthy comment above this code makes it clear why the check is there. Code changes are fine. Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
On 2021/3/13 3:58, Mike Kravetz wrote: > On 3/8/21 3:28 AM, Miaohe Lin wrote: >> Rework the error handling code when alloc_huge_page() failed to remove some >> duplicated code and simplify the code slightly. >> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/hugetlb.c | 9 +++------ >> 1 file changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >> index 695603071f2c..69b8de866a24 100644 >> --- a/mm/hugetlb.c >> +++ b/mm/hugetlb.c >> @@ -4337,13 +4337,10 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, >> * sure there really is no pte entry. >> */ >> ptl = huge_pte_lock(h, mm, ptep); >> - if (!huge_pte_none(huge_ptep_get(ptep))) { >> - ret = 0; >> - spin_unlock(ptl); >> - goto out; >> - } >> + ret = 0; >> + if (huge_pte_none(huge_ptep_get(ptep))) >> + ret = vmf_error(PTR_ERR(page)); > > This new code is simpler. > > The !huge_pte_none() catches an unlikely race. IMO, the existing code > made that very clear. Would have been even more clear with an unlikely > modifier. In any case, the lengthy comment above this code makes it > clear why the check is there. Code changes are fine. > Yep, the lengthy comment above this code makes it much clear why we need the check. Thanks for carefully review! :) > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> >
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 695603071f2c..69b8de866a24 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -4337,13 +4337,10 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm, * sure there really is no pte entry. */ ptl = huge_pte_lock(h, mm, ptep); - if (!huge_pte_none(huge_ptep_get(ptep))) { - ret = 0; - spin_unlock(ptl); - goto out; - } + ret = 0; + if (huge_pte_none(huge_ptep_get(ptep))) + ret = vmf_error(PTR_ERR(page)); spin_unlock(ptl); - ret = vmf_error(PTR_ERR(page)); goto out; } clear_huge_page(page, address, pages_per_huge_page(h));
Rework the error handling code when alloc_huge_page() failed to remove some duplicated code and simplify the code slightly. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/hugetlb.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)