On Feb 21, 2020, at 10:34 PM, Longpeng(Mike) longpeng2@huawei.com wrote:
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index dd8737a..90daf37 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -4910,28 +4910,30 @@ pte_t *huge_pte_offset(struct mm_struct *mm, { pgd_t *pgd; p4d_t *p4d;
- pud_t *pud;
- pmd_t *pmd;
pud_t *pud, pud_entry;
pmd_t *pmd, pmd_entry;
pgd = pgd_offset(mm, addr);
- if (!pgd_present(*pgd))
- if (!pgd_present(READ_ONCE(*pgd))) return NULL; p4d = p4d_offset(pgd, addr);
- if (!p4d_present(*p4d))
- if (!p4d_present(READ_ONCE(*p4d))) return NULL;
What’s the point of READ_ONCE() on those two places?
在 2020/2/22 13:23, Qian Cai 写道:
On Feb 21, 2020, at 10:34 PM, Longpeng(Mike) longpeng2@huawei.com wrote:
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index dd8737a..90daf37 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -4910,28 +4910,30 @@ pte_t *huge_pte_offset(struct mm_struct *mm, { pgd_t *pgd; p4d_t *p4d;
- pud_t *pud;
- pmd_t *pmd;
pud_t *pud, pud_entry;
pmd_t *pmd, pmd_entry;
pgd = pgd_offset(mm, addr);
- if (!pgd_present(*pgd))
- if (!pgd_present(READ_ONCE(*pgd))) return NULL; p4d = p4d_offset(pgd, addr);
- if (!p4d_present(*p4d))
- if (!p4d_present(READ_ONCE(*p4d))) return NULL;
What’s the point of READ_ONCE() on those two places?
As explained in the commit messages, it's for safe(e.g. avoid the compilier mischief). You can also find the same usage in the ARM64's huge_pte_offset() in arch/arm64/mm/hugetlbpage.c
On Feb 22, 2020, at 1:33 AM, Longpeng (Mike) longpeng2@huawei.com wrote:
As explained in the commit messages, it's for safe(e.g. avoid the compilier mischief). You can also find the same usage in the ARM64's huge_pte_offset() in arch/arm64/mm/hugetlbpage.c
Rather than blindly copy over there, are those correct here? What kind of bad compiler optimizations exactly do they try to prevent? Until we understand those details, blindly adding READ_ONCE() will only hide other problems.
On Sat, Feb 22, 2020 at 02:33:10PM +0800, Longpeng (Mike) wrote:
在 2020/2/22 13:23, Qian Cai 写道:
On Feb 21, 2020, at 10:34 PM, Longpeng(Mike) longpeng2@huawei.com wrote:
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index dd8737a..90daf37 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -4910,28 +4910,30 @@ pte_t *huge_pte_offset(struct mm_struct *mm, { pgd_t *pgd; p4d_t *p4d;
- pud_t *pud;
- pmd_t *pmd;
pud_t *pud, pud_entry;
pmd_t *pmd, pmd_entry;
pgd = pgd_offset(mm, addr);
- if (!pgd_present(*pgd))
- if (!pgd_present(READ_ONCE(*pgd))) return NULL; p4d = p4d_offset(pgd, addr);
- if (!p4d_present(*p4d))
- if (!p4d_present(READ_ONCE(*p4d))) return NULL;
What’s the point of READ_ONCE() on those two places?
As explained in the commit messages, it's for safe(e.g. avoid the compilier mischief). You can also find the same usage in the ARM64's huge_pte_offset() in arch/arm64/mm/hugetlbpage.c
I rather agree with Qian; if we need something like READ_ONCE() here, why don't we always need it as part of pgd_present()? It seems like an unnecessary burden for every user.
在 2020/2/23 1:02, Matthew Wilcox 写道:
On Sat, Feb 22, 2020 at 02:33:10PM +0800, Longpeng (Mike) wrote:
在 2020/2/22 13:23, Qian Cai 写道:
On Feb 21, 2020, at 10:34 PM, Longpeng(Mike) longpeng2@huawei.com wrote:
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index dd8737a..90daf37 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -4910,28 +4910,30 @@ pte_t *huge_pte_offset(struct mm_struct *mm, { pgd_t *pgd; p4d_t *p4d;
- pud_t *pud;
- pmd_t *pmd;
pud_t *pud, pud_entry;
pmd_t *pmd, pmd_entry;
pgd = pgd_offset(mm, addr);
- if (!pgd_present(*pgd))
- if (!pgd_present(READ_ONCE(*pgd))) return NULL; p4d = p4d_offset(pgd, addr);
- if (!p4d_present(*p4d))
- if (!p4d_present(READ_ONCE(*p4d))) return NULL;
What’s the point of READ_ONCE() on those two places?
As explained in the commit messages, it's for safe(e.g. avoid the compilier mischief). You can also find the same usage in the ARM64's huge_pte_offset() in arch/arm64/mm/hugetlbpage.c
I rather agree with Qian; if we need something like READ_ONCE() here, why don't we always need it as part of pgd_present()? It seems like an unnecessary burden for every user.
Hi Matthew & Qian,
Firstly, this is NOT a 'blindly copy', it's an unwise words. I don't know whether you read the commit message (commit 20a004e7) of ARM64's huge_pte_offset ? If you read, I think worry about the safe is necessary.
Secondly, huge_pte_offset in mm/hugetlb.c is for ARCH_WANT_GENERAL_HUGETLB, many architectures use it, can you make sure there is no issue on all the architectures using it with all the version of gcc ?
Thirdly, there are several places use READ_ONCE to access the page table in mm/* (e.g. gup_pmd_range), they're also generical for all architectures, and they're much more like unnecessary than here, so why there can use but not here? What's more, you can read this commit 688272809.
On 2/22/20 5:24 PM, Longpeng (Mike) wrote:
在 2020/2/23 1:02, Matthew Wilcox 写道:
On Sat, Feb 22, 2020 at 02:33:10PM +0800, Longpeng (Mike) wrote:
在 2020/2/22 13:23, Qian Cai 写道:
On Feb 21, 2020, at 10:34 PM, Longpeng(Mike) longpeng2@huawei.com wrote:
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index dd8737a..90daf37 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -4910,28 +4910,30 @@ pte_t *huge_pte_offset(struct mm_struct *mm, { pgd_t *pgd; p4d_t *p4d;
- pud_t *pud;
- pmd_t *pmd;
pud_t *pud, pud_entry;
pmd_t *pmd, pmd_entry;
pgd = pgd_offset(mm, addr);
- if (!pgd_present(*pgd))
- if (!pgd_present(READ_ONCE(*pgd))) return NULL; p4d = p4d_offset(pgd, addr);
- if (!p4d_present(*p4d))
- if (!p4d_present(READ_ONCE(*p4d))) return NULL;
What’s the point of READ_ONCE() on those two places?
As explained in the commit messages, it's for safe(e.g. avoid the compilier mischief). You can also find the same usage in the ARM64's huge_pte_offset() in arch/arm64/mm/hugetlbpage.c
I rather agree with Qian; if we need something like READ_ONCE() here, why don't we always need it as part of pgd_present()? It seems like an unnecessary burden for every user.
Hi Matthew & Qian,
Firstly, this is NOT a 'blindly copy', it's an unwise words. I don't know whether you read the commit message (commit 20a004e7) of ARM64's huge_pte_offset ? If you read, I think worry about the safe is necessary.
Secondly, huge_pte_offset in mm/hugetlb.c is for ARCH_WANT_GENERAL_HUGETLB, many architectures use it, can you make sure there is no issue on all the architectures using it with all the version of gcc ?
Thirdly, there are several places use READ_ONCE to access the page table in mm/* (e.g. gup_pmd_range), they're also generical for all architectures, and they're much more like unnecessary than here, so why there can use but not here? What's more, you can read this commit 688272809.
Apologies for the late reply.
In commit 20a004e7 the message says that "Whilst there are some scenarios where this cannot happen ... the overhead of using READ_ONCE/WRITE_ONCE everywhere is minimal and makes the code an awful lot easier to reason about." Therefore, a decision was made to ALWAYS use READ_ONCE in the arm64 code whether or not it was absolutely necessary. Therefore, I do not think we can assume all the READ_ONCE additions made in 20a004e7 are necessary. Then the question remains, it it necessary in two statements above? I do not believe it is necessary. Why? In the statements, if (!pgd_present(*pgd)) and if (!p4d_present(*p4d)) the variables are only accessed and dereferenced once. I can not imagine any way in which the compiler could perform multiple accesses of the variable.
I do believe the READ_ONCE in code accessing the pud and pmd is necessary. This is because the variables (pud_entry or pmd_entry) are accessed more than once. And, I could imagine some strange compiler optimization where it would dereference the pud or pmd pointer more than once. For this same reason (multiple accesses), I believe the READ_ONCE was added in commit 688272809.
I am no expert in this area, so corrections/comments appreciated.
BTW, I still think there may be races present in lookup_address_in_pgd(). Multiple dereferences of a p4d, pud and pmd are done.
On Thu, 27 Feb 2020 13:41:46 -0800 Mike Kravetz mike.kravetz@oracle.com wrote:
Secondly, huge_pte_offset in mm/hugetlb.c is for ARCH_WANT_GENERAL_HUGETLB, many architectures use it, can you make sure there is no issue on all the architectures using it with all the version of gcc ?
Thirdly, there are several places use READ_ONCE to access the page table in mm/* (e.g. gup_pmd_range), they're also generical for all architectures, and they're much more like unnecessary than here, so why there can use but not here? What's more, you can read this commit 688272809.
Apologies for the late reply.
In commit 20a004e7 the message says that "Whilst there are some scenarios where this cannot happen ... the overhead of using READ_ONCE/WRITE_ONCE everywhere is minimal and makes the code an awful lot easier to reason about." Therefore, a decision was made to ALWAYS use READ_ONCE in the arm64 code whether or not it was absolutely necessary. Therefore, I do not think we can assume all the READ_ONCE additions made in 20a004e7 are necessary. Then the question remains, it it necessary in two statements above? I do not believe it is necessary. Why? In the statements, if (!pgd_present(*pgd)) and if (!p4d_present(*p4d)) the variables are only accessed and dereferenced once. I can not imagine any way in which the compiler could perform multiple accesses of the variable.
I do believe the READ_ONCE in code accessing the pud and pmd is necessary. This is because the variables (pud_entry or pmd_entry) are accessed more than once. And, I could imagine some strange compiler optimization where it would dereference the pud or pmd pointer more than once. For this same reason (multiple accesses), I believe the READ_ONCE was added in commit 688272809.
I am no expert in this area, so corrections/comments appreciated.
BTW, I still think there may be races present in lookup_address_in_pgd(). Multiple dereferences of a p4d, pud and pmd are done.
Based on Mike's observations I shall drop this patch. If we still believe it is needed, please enhance the changelog, resend and let's take another look.
linux-stable-mirror@lists.linaro.org
-
Andrew Morton -
Longpeng (Mike) -
Matthew Wilcox -
Mike Kravetz -
Qian Cai