The memory barrier smp_wmb() is needed to make sure that preceding stores
to the page contents become visible before the below set_pte_at() write.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/hugetlb_vmemmap.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
index 20f414c0379f..76b2d03a0d8d 100644
--- a/mm/hugetlb_vmemmap.c
+++ b/mm/hugetlb_vmemmap.c
@@ -287,6 +287,11 @@ static void vmemmap_restore_pte(pte_t *pte, unsigned long addr,
copy_page(to, (void *)walk->reuse_addr);
reset_struct_pages(to);
+ /*
+ * Makes sure that preceding stores to the page contents become visible
+ * before the set_pte_at() write.
+ */
+ smp_wmb();
set_pte_at(&init_mm, addr, pte, mk_pte(page, pgprot));
}
--
2.23.0
> On Aug 16, 2022, at 21:05, Miaohe Lin <[email protected]> wrote:
>
> The memory barrier smp_wmb() is needed to make sure that preceding stores
> to the page contents become visible before the below set_pte_at() write.
I’m not sure if you are right. I think it is set_pte_at()’s responsibility.
Take arm64 (since it is a Relaxed Memory Order model) as an example (the
following code snippet is set_pte()), I see a barrier guarantee. So I am
curious what issues you are facing. So I want to know the basis for you to
do this change.
static inline void set_pte(pte_t *ptep, pte_t pte)
{
*ptep = pte;
/*
* Only if the new pte is valid and kernel, otherwise TLB maintenance
* or update_mmu_cache() have the necessary barriers.
*/
if (pte_valid_not_user(pte)) {
dsb(ishst);
isb();
}
}
Thanks.
>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> mm/hugetlb_vmemmap.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
> index 20f414c0379f..76b2d03a0d8d 100644
> --- a/mm/hugetlb_vmemmap.c
> +++ b/mm/hugetlb_vmemmap.c
> @@ -287,6 +287,11 @@ static void vmemmap_restore_pte(pte_t *pte, unsigned long addr,
> copy_page(to, (void *)walk->reuse_addr);
> reset_struct_pages(to);
>
> + /*
> + * Makes sure that preceding stores to the page contents become visible
> + * before the set_pte_at() write.
> + */
> + smp_wmb();
> set_pte_at(&init_mm, addr, pte, mk_pte(page, pgprot));
> }
>
> --
> 2.23.0
>
>
On 2022/8/17 10:53, Muchun Song wrote:
>
>
>> On Aug 16, 2022, at 21:05, Miaohe Lin <[email protected]> wrote:
>>
>> The memory barrier smp_wmb() is needed to make sure that preceding stores
>> to the page contents become visible before the below set_pte_at() write.
>
> I’m not sure if you are right. I think it is set_pte_at()’s responsibility.
Maybe not. There're many call sites do the similar things:
hugetlb_mcopy_atomic_pte
__do_huge_pmd_anonymous_page
collapse_huge_page
do_anonymous_page
migrate_vma_insert_page
mcopy_atomic_pte
Take do_anonymous_page as an example:
/*
* The memory barrier inside __SetPageUptodate makes sure that
* preceding stores to the page contents become visible before
* the set_pte_at() write.
*/
__SetPageUptodate(page);
So I think a memory barrier is needed before the set_pte_at() write. Or am I miss something?
Thanks,
Miaohe Lin
> Take arm64 (since it is a Relaxed Memory Order model) as an example (the
> following code snippet is set_pte()), I see a barrier guarantee. So I am
> curious what issues you are facing. So I want to know the basis for you to
> do this change.
>
> static inline void set_pte(pte_t *ptep, pte_t pte)
> {
> *ptep = pte;
>
> /*
> * Only if the new pte is valid and kernel, otherwise TLB maintenance
> * or update_mmu_cache() have the necessary barriers.
> */
> if (pte_valid_not_user(pte)) {
> dsb(ishst);
> isb();
> }
> }
>
> Thanks.
>
> On Aug 17, 2022, at 16:41, Miaohe Lin <[email protected]> wrote:
>
> On 2022/8/17 10:53, Muchun Song wrote:
>>
>>
>>> On Aug 16, 2022, at 21:05, Miaohe Lin <[email protected]> wrote:
>>>
>>> The memory barrier smp_wmb() is needed to make sure that preceding stores
>>> to the page contents become visible before the below set_pte_at() write.
>>
>> I’m not sure if you are right. I think it is set_pte_at()’s responsibility.
>
> Maybe not. There're many call sites do the similar things:
>
> hugetlb_mcopy_atomic_pte
> __do_huge_pmd_anonymous_page
> collapse_huge_page
> do_anonymous_page
> migrate_vma_insert_page
> mcopy_atomic_pte
>
> Take do_anonymous_page as an example:
>
> /*
> * The memory barrier inside __SetPageUptodate makes sure that
> * preceding stores to the page contents become visible before
> * the set_pte_at() write.
> */
> __SetPageUptodate(page);
IIUC, the case here we should make sure others (CPUs) can see new page’s
contents after they have saw PG_uptodate is set. I think commit 0ed361dec369
can tell us more details.
I also looked at commit 52f37629fd3c to see why we need a barrier before
set_pte_at(), but I didn’t find any info to explain why. I guess we want
to make sure the order between the page’s contents and subsequent memory
accesses using the corresponding virtual address, do you agree with this?
Thanks.
>
> So I think a memory barrier is needed before the set_pte_at() write. Or am I miss something?
>
> Thanks,
> Miaohe Lin
>
>> Take arm64 (since it is a Relaxed Memory Order model) as an example (the
>> following code snippet is set_pte()), I see a barrier guarantee. So I am
>> curious what issues you are facing. So I want to know the basis for you to
>> do this change.
>>
>> static inline void set_pte(pte_t *ptep, pte_t pte)
>> {
>> *ptep = pte;
>>
>> /*
>> * Only if the new pte is valid and kernel, otherwise TLB maintenance
>> * or update_mmu_cache() have the necessary barriers.
>> */
>> if (pte_valid_not_user(pte)) {
>> dsb(ishst);
>> isb();
>> }
>> }
>>
>> Thanks.
>>
On 8/16/2022 9:05 PM, Miaohe Lin wrote:
> The memory barrier smp_wmb() is needed to make sure that preceding stores
> to the page contents become visible before the below set_pte_at() write.
>
> Signed-off-by: Miaohe Lin <[email protected]>
Reviewed-by: Yin Fengwei <[email protected]>
Regards
Yin, Fengwei
> ---
> mm/hugetlb_vmemmap.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
> index 20f414c0379f..76b2d03a0d8d 100644
> --- a/mm/hugetlb_vmemmap.c
> +++ b/mm/hugetlb_vmemmap.c
> @@ -287,6 +287,11 @@ static void vmemmap_restore_pte(pte_t *pte, unsigned long addr,
> copy_page(to, (void *)walk->reuse_addr);
> reset_struct_pages(to);
>
> + /*
> + * Makes sure that preceding stores to the page contents become visible
> + * before the set_pte_at() write.
> + */
> + smp_wmb();
> set_pte_at(&init_mm, addr, pte, mk_pte(page, pgprot));
> }
>
On 8/17/2022 7:21 PM, Muchun Song wrote:
>
>
>> On Aug 17, 2022, at 16:41, Miaohe Lin <[email protected]> wrote:
>>
>> On 2022/8/17 10:53, Muchun Song wrote:
>>>
>>>
>>>> On Aug 16, 2022, at 21:05, Miaohe Lin <[email protected]> wrote:
>>>>
>>>> The memory barrier smp_wmb() is needed to make sure that preceding stores
>>>> to the page contents become visible before the below set_pte_at() write.
>>>
>>> I’m not sure if you are right. I think it is set_pte_at()’s responsibility.
>>
>> Maybe not. There're many call sites do the similar things:
>>
>> hugetlb_mcopy_atomic_pte
>> __do_huge_pmd_anonymous_page
>> collapse_huge_page
>> do_anonymous_page
>> migrate_vma_insert_page
>> mcopy_atomic_pte
>>
>> Take do_anonymous_page as an example:
>>
>> /*
>> * The memory barrier inside __SetPageUptodate makes sure that
>> * preceding stores to the page contents become visible before
>> * the set_pte_at() write.
>> */
>> __SetPageUptodate(page);
>
> IIUC, the case here we should make sure others (CPUs) can see new page’s
> contents after they have saw PG_uptodate is set. I think commit 0ed361dec369
> can tell us more details.
>
> I also looked at commit 52f37629fd3c to see why we need a barrier before
> set_pte_at(), but I didn’t find any info to explain why. I guess we want
> to make sure the order between the page’s contents and subsequent memory
> accesses using the corresponding virtual address, do you agree with this?
This is my understanding also. Thanks.
Regards
Yin, Fengwei
>
> Thanks.
>
>>
>> So I think a memory barrier is needed before the set_pte_at() write. Or am I miss something?
>>
>> Thanks,
>> Miaohe Lin
>>
>>> Take arm64 (since it is a Relaxed Memory Order model) as an example (the
>>> following code snippet is set_pte()), I see a barrier guarantee. So I am
>>> curious what issues you are facing. So I want to know the basis for you to
>>> do this change.
>>>
>>> static inline void set_pte(pte_t *ptep, pte_t pte)
>>> {
>>> *ptep = pte;
>>>
>>> /*
>>> * Only if the new pte is valid and kernel, otherwise TLB maintenance
>>> * or update_mmu_cache() have the necessary barriers.
>>> */
>>> if (pte_valid_not_user(pte)) {
>>> dsb(ishst);
>>> isb();
>>> }
>>> }
>>>
>>> Thanks.
>>>
>
>
On 8/18/2022 9:55 AM, Miaohe Lin wrote:
>>>> /*
>>>> * The memory barrier inside __SetPageUptodate makes sure that
>>>> * preceding stores to the page contents become visible before
>>>> * the set_pte_at() write.
>>>> */
>>>> __SetPageUptodate(page);
>>> IIUC, the case here we should make sure others (CPUs) can see new page’s
>>> contents after they have saw PG_uptodate is set. I think commit 0ed361dec369
>>> can tell us more details.
>>>
>>> I also looked at commit 52f37629fd3c to see why we need a barrier before
>>> set_pte_at(), but I didn’t find any info to explain why. I guess we want
>>> to make sure the order between the page’s contents and subsequent memory
>>> accesses using the corresponding virtual address, do you agree with this?
>> This is my understanding also. Thanks.
> That's also my understanding. Thanks both.
I have an unclear thing (not related with this patch directly): Who is response
for the read barrier in the read side in this case?
For SetPageUptodate, there are paring write/read memory barrier.
Regards
Yin, Fengwei
On 2022/8/18 9:14, Yin, Fengwei wrote:
>
>
> On 8/17/2022 7:21 PM, Muchun Song wrote:
>>
>>
>>> On Aug 17, 2022, at 16:41, Miaohe Lin <[email protected]> wrote:
>>>
>>> On 2022/8/17 10:53, Muchun Song wrote:
>>>>
>>>>
>>>>> On Aug 16, 2022, at 21:05, Miaohe Lin <[email protected]> wrote:
>>>>>
>>>>> The memory barrier smp_wmb() is needed to make sure that preceding stores
>>>>> to the page contents become visible before the below set_pte_at() write.
>>>>
>>>> I’m not sure if you are right. I think it is set_pte_at()’s responsibility.
>>>
>>> Maybe not. There're many call sites do the similar things:
>>>
>>> hugetlb_mcopy_atomic_pte
>>> __do_huge_pmd_anonymous_page
>>> collapse_huge_page
>>> do_anonymous_page
>>> migrate_vma_insert_page
>>> mcopy_atomic_pte
>>>
>>> Take do_anonymous_page as an example:
>>>
>>> /*
>>> * The memory barrier inside __SetPageUptodate makes sure that
>>> * preceding stores to the page contents become visible before
>>> * the set_pte_at() write.
>>> */
>>> __SetPageUptodate(page);
>>
>> IIUC, the case here we should make sure others (CPUs) can see new page’s
>> contents after they have saw PG_uptodate is set. I think commit 0ed361dec369
>> can tell us more details.
>>
>> I also looked at commit 52f37629fd3c to see why we need a barrier before
>> set_pte_at(), but I didn’t find any info to explain why. I guess we want
>> to make sure the order between the page’s contents and subsequent memory
>> accesses using the corresponding virtual address, do you agree with this?
> This is my understanding also. Thanks.
That's also my understanding. Thanks both.
Thanks,
Miaohe Lin
> On Aug 18, 2022, at 10:00, Yin, Fengwei <[email protected]> wrote:
>
>
>
> On 8/18/2022 9:55 AM, Miaohe Lin wrote:
>>>>> /*
>>>>> * The memory barrier inside __SetPageUptodate makes sure that
>>>>> * preceding stores to the page contents become visible before
>>>>> * the set_pte_at() write.
>>>>> */
>>>>> __SetPageUptodate(page);
>>>> IIUC, the case here we should make sure others (CPUs) can see new page’s
>>>> contents after they have saw PG_uptodate is set. I think commit 0ed361dec369
>>>> can tell us more details.
>>>>
>>>> I also looked at commit 52f37629fd3c to see why we need a barrier before
>>>> set_pte_at(), but I didn’t find any info to explain why. I guess we want
>>>> to make sure the order between the page’s contents and subsequent memory
>>>> accesses using the corresponding virtual address, do you agree with this?
>>> This is my understanding also. Thanks.
>> That's also my understanding. Thanks both.
> I have an unclear thing (not related with this patch directly): Who is response
> for the read barrier in the read side in this case?
>
> For SetPageUptodate, there are paring write/read memory barrier.
>
I have the same question. So I think the example proposed by Miaohe is a little
difference from the case (hugetlb_vmemmap) here.
>
> Regards
> Yin, Fengwei
>
>
On 2022/8/18 10:47, Muchun Song wrote:
>
>
>> On Aug 18, 2022, at 10:00, Yin, Fengwei <[email protected]> wrote:
>>
>>
>>
>> On 8/18/2022 9:55 AM, Miaohe Lin wrote:
>>>>>> /*
>>>>>> * The memory barrier inside __SetPageUptodate makes sure that
>>>>>> * preceding stores to the page contents become visible before
>>>>>> * the set_pte_at() write.
>>>>>> */
>>>>>> __SetPageUptodate(page);
>>>>> IIUC, the case here we should make sure others (CPUs) can see new page’s
>>>>> contents after they have saw PG_uptodate is set. I think commit 0ed361dec369
>>>>> can tell us more details.
>>>>>
>>>>> I also looked at commit 52f37629fd3c to see why we need a barrier before
>>>>> set_pte_at(), but I didn’t find any info to explain why. I guess we want
>>>>> to make sure the order between the page’s contents and subsequent memory
>>>>> accesses using the corresponding virtual address, do you agree with this?
>>>> This is my understanding also. Thanks.
>>> That's also my understanding. Thanks both.
>> I have an unclear thing (not related with this patch directly): Who is response
>> for the read barrier in the read side in this case?
>>
>> For SetPageUptodate, there are paring write/read memory barrier.
>>
>
> I have the same question. So I think the example proposed by Miaohe is a little
> difference from the case (hugetlb_vmemmap) here.
Per my understanding, memory barrier in PageUptodate() is needed because user might access the
page contents using page_address() (corresponding pagetable entry already exists) soon. But for
the above proposed case, if user wants to access the page contents, the corresponding pagetable
should be visible first or the page contents can't be accessed. So there should be a data dependency
acting as memory barrier between pagetable entry is loaded and page contents is accessed.
Or am I miss something?
Thanks,
Miaohe Lin
> On Aug 18, 2022, at 15:52, Miaohe Lin <[email protected]> wrote:
>
> On 2022/8/18 10:47, Muchun Song wrote:
>>
>>
>>> On Aug 18, 2022, at 10:00, Yin, Fengwei <[email protected]> wrote:
>>>
>>>
>>>
>>> On 8/18/2022 9:55 AM, Miaohe Lin wrote:
>>>>>>> /*
>>>>>>> * The memory barrier inside __SetPageUptodate makes sure that
>>>>>>> * preceding stores to the page contents become visible before
>>>>>>> * the set_pte_at() write.
>>>>>>> */
>>>>>>> __SetPageUptodate(page);
>>>>>> IIUC, the case here we should make sure others (CPUs) can see new page’s
>>>>>> contents after they have saw PG_uptodate is set. I think commit 0ed361dec369
>>>>>> can tell us more details.
>>>>>>
>>>>>> I also looked at commit 52f37629fd3c to see why we need a barrier before
>>>>>> set_pte_at(), but I didn’t find any info to explain why. I guess we want
>>>>>> to make sure the order between the page’s contents and subsequent memory
>>>>>> accesses using the corresponding virtual address, do you agree with this?
>>>>> This is my understanding also. Thanks.
>>>> That's also my understanding. Thanks both.
>>> I have an unclear thing (not related with this patch directly): Who is response
>>> for the read barrier in the read side in this case?
>>>
>>> For SetPageUptodate, there are paring write/read memory barrier.
>>>
>>
>> I have the same question. So I think the example proposed by Miaohe is a little
>> difference from the case (hugetlb_vmemmap) here.
>
> Per my understanding, memory barrier in PageUptodate() is needed because user might access the
> page contents using page_address() (corresponding pagetable entry already exists) soon. But for
> the above proposed case, if user wants to access the page contents, the corresponding pagetable
> should be visible first or the page contents can't be accessed. So there should be a data dependency
> acting as memory barrier between pagetable entry is loaded and page contents is accessed.
> Or am I miss something?
Yep, it is a data dependency. The difference between hugetlb_vmemmap and PageUptodate() is that
the page table (a pointer to the mapped page frame) is loaded by MMU while PageUptodate() is
loaded by CPU. Seems like the data dependency should be inserted between the MMU access and the CPU
access. Maybe it is hardware’s guarantee?
>
> Thanks,
> Miaohe Lin
On 8/18/2022 3:59 PM, Muchun Song wrote:
>
>
>> On Aug 18, 2022, at 15:52, Miaohe Lin <[email protected]> wrote:
>>
>> On 2022/8/18 10:47, Muchun Song wrote:
>>>
>>>
>>>> On Aug 18, 2022, at 10:00, Yin, Fengwei <[email protected]> wrote:
>>>>
>>>>
>>>>
>>>> On 8/18/2022 9:55 AM, Miaohe Lin wrote:
>>>>>>>> /*
>>>>>>>> * The memory barrier inside __SetPageUptodate makes sure that
>>>>>>>> * preceding stores to the page contents become visible before
>>>>>>>> * the set_pte_at() write.
>>>>>>>> */
>>>>>>>> __SetPageUptodate(page);
>>>>>>> IIUC, the case here we should make sure others (CPUs) can see new page’s
>>>>>>> contents after they have saw PG_uptodate is set. I think commit 0ed361dec369
>>>>>>> can tell us more details.
>>>>>>>
>>>>>>> I also looked at commit 52f37629fd3c to see why we need a barrier before
>>>>>>> set_pte_at(), but I didn’t find any info to explain why. I guess we want
>>>>>>> to make sure the order between the page’s contents and subsequent memory
>>>>>>> accesses using the corresponding virtual address, do you agree with this?
>>>>>> This is my understanding also. Thanks.
>>>>> That's also my understanding. Thanks both.
>>>> I have an unclear thing (not related with this patch directly): Who is response
>>>> for the read barrier in the read side in this case?
>>>>
>>>> For SetPageUptodate, there are paring write/read memory barrier.
>>>>
>>>
>>> I have the same question. So I think the example proposed by Miaohe is a little
>>> difference from the case (hugetlb_vmemmap) here.
>>
>> Per my understanding, memory barrier in PageUptodate() is needed because user might access the
>> page contents using page_address() (corresponding pagetable entry already exists) soon. But for
>> the above proposed case, if user wants to access the page contents, the corresponding pagetable
>> should be visible first or the page contents can't be accessed. So there should be a data dependency
>> acting as memory barrier between pagetable entry is loaded and page contents is accessed.
>> Or am I miss something?
>
> Yep, it is a data dependency. The difference between hugetlb_vmemmap and PageUptodate() is that
> the page table (a pointer to the mapped page frame) is loaded by MMU while PageUptodate() is
> loaded by CPU. Seems like the data dependency should be inserted between the MMU access and the CPU
> access. Maybe it is hardware’s guarantee?
I just found the comment in pmd_install() explained why most arch has no read
side memory barrier except alpha which has read side memory barrier.
Regards
Yin, Fengwei
>
>>
>> Thanks,
>> Miaohe Lin
>
> On Aug 18, 2022, at 16:32, Yin, Fengwei <[email protected]> wrote:
>
>
>
> On 8/18/2022 3:59 PM, Muchun Song wrote:
>>
>>
>>> On Aug 18, 2022, at 15:52, Miaohe Lin <[email protected]> wrote:
>>>
>>> On 2022/8/18 10:47, Muchun Song wrote:
>>>>
>>>>
>>>>> On Aug 18, 2022, at 10:00, Yin, Fengwei <[email protected]> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 8/18/2022 9:55 AM, Miaohe Lin wrote:
>>>>>>>>> /*
>>>>>>>>> * The memory barrier inside __SetPageUptodate makes sure that
>>>>>>>>> * preceding stores to the page contents become visible before
>>>>>>>>> * the set_pte_at() write.
>>>>>>>>> */
>>>>>>>>> __SetPageUptodate(page);
>>>>>>>> IIUC, the case here we should make sure others (CPUs) can see new page’s
>>>>>>>> contents after they have saw PG_uptodate is set. I think commit 0ed361dec369
>>>>>>>> can tell us more details.
>>>>>>>>
>>>>>>>> I also looked at commit 52f37629fd3c to see why we need a barrier before
>>>>>>>> set_pte_at(), but I didn’t find any info to explain why. I guess we want
>>>>>>>> to make sure the order between the page’s contents and subsequent memory
>>>>>>>> accesses using the corresponding virtual address, do you agree with this?
>>>>>>> This is my understanding also. Thanks.
>>>>>> That's also my understanding. Thanks both.
>>>>> I have an unclear thing (not related with this patch directly): Who is response
>>>>> for the read barrier in the read side in this case?
>>>>>
>>>>> For SetPageUptodate, there are paring write/read memory barrier.
>>>>>
>>>>
>>>> I have the same question. So I think the example proposed by Miaohe is a little
>>>> difference from the case (hugetlb_vmemmap) here.
>>>
>>> Per my understanding, memory barrier in PageUptodate() is needed because user might access the
>>> page contents using page_address() (corresponding pagetable entry already exists) soon. But for
>>> the above proposed case, if user wants to access the page contents, the corresponding pagetable
>>> should be visible first or the page contents can't be accessed. So there should be a data dependency
>>> acting as memory barrier between pagetable entry is loaded and page contents is accessed.
>>> Or am I miss something?
>>
>> Yep, it is a data dependency. The difference between hugetlb_vmemmap and PageUptodate() is that
>> the page table (a pointer to the mapped page frame) is loaded by MMU while PageUptodate() is
>> loaded by CPU. Seems like the data dependency should be inserted between the MMU access and the CPU
>> access. Maybe it is hardware’s guarantee?
> I just found the comment in pmd_install() explained why most arch has no read
I think pmd_install() is a little different as well. We should make sure the
page table walker (like GUP) see the correct PTE entry after they see the pmd
entry.
> side memory barrier except alpha which has read side memory barrier.
Right. Only alpha has data dependency barrier.
>
>
> Regards
> Yin, Fengwei
>
>>
>>>
>>> Thanks,
>>> Miaohe Lin
On 8/18/2022 4:40 PM, Muchun Song wrote:
>
>
>> On Aug 18, 2022, at 16:32, Yin, Fengwei <[email protected]> wrote:
>>
>>
>>
>> On 8/18/2022 3:59 PM, Muchun Song wrote:
>>>
>>>
>>>> On Aug 18, 2022, at 15:52, Miaohe Lin <[email protected]> wrote:
>>>>
>>>> On 2022/8/18 10:47, Muchun Song wrote:
>>>>>
>>>>>
>>>>>> On Aug 18, 2022, at 10:00, Yin, Fengwei <[email protected]> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 8/18/2022 9:55 AM, Miaohe Lin wrote:
>>>>>>>>>> /*
>>>>>>>>>> * The memory barrier inside __SetPageUptodate makes sure that
>>>>>>>>>> * preceding stores to the page contents become visible before
>>>>>>>>>> * the set_pte_at() write.
>>>>>>>>>> */
>>>>>>>>>> __SetPageUptodate(page);
>>>>>>>>> IIUC, the case here we should make sure others (CPUs) can see new page’s
>>>>>>>>> contents after they have saw PG_uptodate is set. I think commit 0ed361dec369
>>>>>>>>> can tell us more details.
>>>>>>>>>
>>>>>>>>> I also looked at commit 52f37629fd3c to see why we need a barrier before
>>>>>>>>> set_pte_at(), but I didn’t find any info to explain why. I guess we want
>>>>>>>>> to make sure the order between the page’s contents and subsequent memory
>>>>>>>>> accesses using the corresponding virtual address, do you agree with this?
>>>>>>>> This is my understanding also. Thanks.
>>>>>>> That's also my understanding. Thanks both.
>>>>>> I have an unclear thing (not related with this patch directly): Who is response
>>>>>> for the read barrier in the read side in this case?
>>>>>>
>>>>>> For SetPageUptodate, there are paring write/read memory barrier.
>>>>>>
>>>>>
>>>>> I have the same question. So I think the example proposed by Miaohe is a little
>>>>> difference from the case (hugetlb_vmemmap) here.
>>>>
>>>> Per my understanding, memory barrier in PageUptodate() is needed because user might access the
>>>> page contents using page_address() (corresponding pagetable entry already exists) soon. But for
>>>> the above proposed case, if user wants to access the page contents, the corresponding pagetable
>>>> should be visible first or the page contents can't be accessed. So there should be a data dependency
>>>> acting as memory barrier between pagetable entry is loaded and page contents is accessed.
>>>> Or am I miss something?
>>>
>>> Yep, it is a data dependency. The difference between hugetlb_vmemmap and PageUptodate() is that
>>> the page table (a pointer to the mapped page frame) is loaded by MMU while PageUptodate() is
>>> loaded by CPU. Seems like the data dependency should be inserted between the MMU access and the CPU
>>> access. Maybe it is hardware’s guarantee?
>> I just found the comment in pmd_install() explained why most arch has no read
>
> I think pmd_install() is a little different as well. We should make sure the
> page table walker (like GUP) see the correct PTE entry after they see the pmd
> entry.
The difference I can see is that pmd/pte thing has both hardware page walker and
software page walker (like GUP) as read side. While the case here only has hardware
page walker as read side. But I suppose the memory barrier requirement still apply
here.
Maybe we could do a test: add large delay between reset_struct_page() and set_pte_at?
Regards
Yin, Fengwei
>
>> side memory barrier except alpha which has read side memory barrier.
>
> Right. Only alpha has data dependency barrier.
>
>>
>>
>> Regards
>> Yin, Fengwei
>>
>>>
>>>>
>>>> Thanks,
>>>> Miaohe Lin
>
> On Aug 18, 2022, at 16:54, Yin, Fengwei <[email protected]> wrote:
>
>
>
> On 8/18/2022 4:40 PM, Muchun Song wrote:
>>
>>
>>> On Aug 18, 2022, at 16:32, Yin, Fengwei <[email protected]> wrote:
>>>
>>>
>>>
>>> On 8/18/2022 3:59 PM, Muchun Song wrote:
>>>>
>>>>
>>>>> On Aug 18, 2022, at 15:52, Miaohe Lin <[email protected]> wrote:
>>>>>
>>>>> On 2022/8/18 10:47, Muchun Song wrote:
>>>>>>
>>>>>>
>>>>>>> On Aug 18, 2022, at 10:00, Yin, Fengwei <[email protected]> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 8/18/2022 9:55 AM, Miaohe Lin wrote:
>>>>>>>>>>> /*
>>>>>>>>>>> * The memory barrier inside __SetPageUptodate makes sure that
>>>>>>>>>>> * preceding stores to the page contents become visible before
>>>>>>>>>>> * the set_pte_at() write.
>>>>>>>>>>> */
>>>>>>>>>>> __SetPageUptodate(page);
>>>>>>>>>> IIUC, the case here we should make sure others (CPUs) can see new page’s
>>>>>>>>>> contents after they have saw PG_uptodate is set. I think commit 0ed361dec369
>>>>>>>>>> can tell us more details.
>>>>>>>>>>
>>>>>>>>>> I also looked at commit 52f37629fd3c to see why we need a barrier before
>>>>>>>>>> set_pte_at(), but I didn’t find any info to explain why. I guess we want
>>>>>>>>>> to make sure the order between the page’s contents and subsequent memory
>>>>>>>>>> accesses using the corresponding virtual address, do you agree with this?
>>>>>>>>> This is my understanding also. Thanks.
>>>>>>>> That's also my understanding. Thanks both.
>>>>>>> I have an unclear thing (not related with this patch directly): Who is response
>>>>>>> for the read barrier in the read side in this case?
>>>>>>>
>>>>>>> For SetPageUptodate, there are paring write/read memory barrier.
>>>>>>>
>>>>>>
>>>>>> I have the same question. So I think the example proposed by Miaohe is a little
>>>>>> difference from the case (hugetlb_vmemmap) here.
>>>>>
>>>>> Per my understanding, memory barrier in PageUptodate() is needed because user might access the
>>>>> page contents using page_address() (corresponding pagetable entry already exists) soon. But for
>>>>> the above proposed case, if user wants to access the page contents, the corresponding pagetable
>>>>> should be visible first or the page contents can't be accessed. So there should be a data dependency
>>>>> acting as memory barrier between pagetable entry is loaded and page contents is accessed.
>>>>> Or am I miss something?
>>>>
>>>> Yep, it is a data dependency. The difference between hugetlb_vmemmap and PageUptodate() is that
>>>> the page table (a pointer to the mapped page frame) is loaded by MMU while PageUptodate() is
>>>> loaded by CPU. Seems like the data dependency should be inserted between the MMU access and the CPU
>>>> access. Maybe it is hardware’s guarantee?
>>> I just found the comment in pmd_install() explained why most arch has no read
>>
>> I think pmd_install() is a little different as well. We should make sure the
>> page table walker (like GUP) see the correct PTE entry after they see the pmd
>> entry.
>
> The difference I can see is that pmd/pte thing has both hardware page walker and
> software page walker (like GUP) as read side. While the case here only has hardware
> page walker as read side. But I suppose the memory barrier requirement still apply
> here.
I am not against this change. Just in order to make me get a better understanding of
hardware behavior.
>
> Maybe we could do a test: add large delay between reset_struct_page() and set_pte_at?
Hi Miaohe,
Would you mind doing this test? One thread do vmemmap_restore_pte(), another thread
detect if it can see a tail page with PG_head after the previous thread has executed
set_pte_at().
Thanks.
>
> Regards
> Yin, Fengwei
>
>>
>>> side memory barrier except alpha which has read side memory barrier.
>>
>> Right. Only alpha has data dependency barrier.
>>
>>>
>>>
>>> Regards
>>> Yin, Fengwei
>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>> Miaohe Lin
On 2022/8/18 17:18, Muchun Song wrote:
>
>
>> On Aug 18, 2022, at 16:54, Yin, Fengwei <[email protected]> wrote:
>>
>>
>>
>> On 8/18/2022 4:40 PM, Muchun Song wrote:
>>>
>>>
>>>> On Aug 18, 2022, at 16:32, Yin, Fengwei <[email protected]> wrote:
>>>>
>>>>
>>>>
>>>> On 8/18/2022 3:59 PM, Muchun Song wrote:
>>>>>
>>>>>
>>>>>> On Aug 18, 2022, at 15:52, Miaohe Lin <[email protected]> wrote:
>>>>>>
>>>>>> On 2022/8/18 10:47, Muchun Song wrote:
>>>>>>>
>>>>>>>
>>>>>>>> On Aug 18, 2022, at 10:00, Yin, Fengwei <[email protected]> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 8/18/2022 9:55 AM, Miaohe Lin wrote:
>>>>>>>>>>>> /*
>>>>>>>>>>>> * The memory barrier inside __SetPageUptodate makes sure that
>>>>>>>>>>>> * preceding stores to the page contents become visible before
>>>>>>>>>>>> * the set_pte_at() write.
>>>>>>>>>>>> */
>>>>>>>>>>>> __SetPageUptodate(page);
>>>>>>>>>>> IIUC, the case here we should make sure others (CPUs) can see new page’s
>>>>>>>>>>> contents after they have saw PG_uptodate is set. I think commit 0ed361dec369
>>>>>>>>>>> can tell us more details.
>>>>>>>>>>>
>>>>>>>>>>> I also looked at commit 52f37629fd3c to see why we need a barrier before
>>>>>>>>>>> set_pte_at(), but I didn’t find any info to explain why. I guess we want
>>>>>>>>>>> to make sure the order between the page’s contents and subsequent memory
>>>>>>>>>>> accesses using the corresponding virtual address, do you agree with this?
>>>>>>>>>> This is my understanding also. Thanks.
>>>>>>>>> That's also my understanding. Thanks both.
>>>>>>>> I have an unclear thing (not related with this patch directly): Who is response
>>>>>>>> for the read barrier in the read side in this case?
>>>>>>>>
>>>>>>>> For SetPageUptodate, there are paring write/read memory barrier.
>>>>>>>>
>>>>>>>
>>>>>>> I have the same question. So I think the example proposed by Miaohe is a little
>>>>>>> difference from the case (hugetlb_vmemmap) here.
>>>>>>
>>>>>> Per my understanding, memory barrier in PageUptodate() is needed because user might access the
>>>>>> page contents using page_address() (corresponding pagetable entry already exists) soon. But for
>>>>>> the above proposed case, if user wants to access the page contents, the corresponding pagetable
>>>>>> should be visible first or the page contents can't be accessed. So there should be a data dependency
>>>>>> acting as memory barrier between pagetable entry is loaded and page contents is accessed.
>>>>>> Or am I miss something?
>>>>>
>>>>> Yep, it is a data dependency. The difference between hugetlb_vmemmap and PageUptodate() is that
>>>>> the page table (a pointer to the mapped page frame) is loaded by MMU while PageUptodate() is
>>>>> loaded by CPU. Seems like the data dependency should be inserted between the MMU access and the CPU
>>>>> access. Maybe it is hardware’s guarantee?
>>>> I just found the comment in pmd_install() explained why most arch has no read
>>>
>>> I think pmd_install() is a little different as well. We should make sure the
>>> page table walker (like GUP) see the correct PTE entry after they see the pmd
>>> entry.
>>
>> The difference I can see is that pmd/pte thing has both hardware page walker and
>> software page walker (like GUP) as read side. While the case here only has hardware
>> page walker as read side. But I suppose the memory barrier requirement still apply
>> here.
>
> I am not against this change. Just in order to make me get a better understanding of
> hardware behavior.
>
>>
>> Maybe we could do a test: add large delay between reset_struct_page() and set_pte_at?
>
> Hi Miaohe,
>
> Would you mind doing this test? One thread do vmemmap_restore_pte(), another thread
> detect if it can see a tail page with PG_head after the previous thread has executed
> set_pte_at().
Will it be easier to construct the memory reorder manually like below?
vmemmap_restore_pte()
...
set_pte_at(&init_mm, addr, pte, mk_pte(page, pgprot));
/* might a delay. */
copy_page(to, (void *)walk->reuse_addr);
reset_struct_pages(to);
And another thread detects whether it can see a tail page with some invalid fields? If so,
it seems the problem will always trigger? If not, we depend on the observed meory reorder
and set_pte_at doesn't contain a memory barrier?
Thanks,
Miaohe Lin
On 8/18/2022 8:58 PM, Miaohe Lin wrote:
> On 2022/8/18 17:18, Muchun Song wrote:
>>
>>
>>> On Aug 18, 2022, at 16:54, Yin, Fengwei <[email protected]> wrote:
>>>
>>>
>>>
>>> On 8/18/2022 4:40 PM, Muchun Song wrote:
>>>>
>>>>
>>>>> On Aug 18, 2022, at 16:32, Yin, Fengwei <[email protected]> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 8/18/2022 3:59 PM, Muchun Song wrote:
>>>>>>
>>>>>>
>>>>>>> On Aug 18, 2022, at 15:52, Miaohe Lin <[email protected]> wrote:
>>>>>>>
>>>>>>> On 2022/8/18 10:47, Muchun Song wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> On Aug 18, 2022, at 10:00, Yin, Fengwei <[email protected]> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 8/18/2022 9:55 AM, Miaohe Lin wrote:
>>>>>>>>>>>>> /*
>>>>>>>>>>>>> * The memory barrier inside __SetPageUptodate makes sure that
>>>>>>>>>>>>> * preceding stores to the page contents become visible before
>>>>>>>>>>>>> * the set_pte_at() write.
>>>>>>>>>>>>> */
>>>>>>>>>>>>> __SetPageUptodate(page);
>>>>>>>>>>>> IIUC, the case here we should make sure others (CPUs) can see new page’s
>>>>>>>>>>>> contents after they have saw PG_uptodate is set. I think commit 0ed361dec369
>>>>>>>>>>>> can tell us more details.
>>>>>>>>>>>>
>>>>>>>>>>>> I also looked at commit 52f37629fd3c to see why we need a barrier before
>>>>>>>>>>>> set_pte_at(), but I didn’t find any info to explain why. I guess we want
>>>>>>>>>>>> to make sure the order between the page’s contents and subsequent memory
>>>>>>>>>>>> accesses using the corresponding virtual address, do you agree with this?
>>>>>>>>>>> This is my understanding also. Thanks.
>>>>>>>>>> That's also my understanding. Thanks both.
>>>>>>>>> I have an unclear thing (not related with this patch directly): Who is response
>>>>>>>>> for the read barrier in the read side in this case?
>>>>>>>>>
>>>>>>>>> For SetPageUptodate, there are paring write/read memory barrier.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I have the same question. So I think the example proposed by Miaohe is a little
>>>>>>>> difference from the case (hugetlb_vmemmap) here.
>>>>>>>
>>>>>>> Per my understanding, memory barrier in PageUptodate() is needed because user might access the
>>>>>>> page contents using page_address() (corresponding pagetable entry already exists) soon. But for
>>>>>>> the above proposed case, if user wants to access the page contents, the corresponding pagetable
>>>>>>> should be visible first or the page contents can't be accessed. So there should be a data dependency
>>>>>>> acting as memory barrier between pagetable entry is loaded and page contents is accessed.
>>>>>>> Or am I miss something?
>>>>>>
>>>>>> Yep, it is a data dependency. The difference between hugetlb_vmemmap and PageUptodate() is that
>>>>>> the page table (a pointer to the mapped page frame) is loaded by MMU while PageUptodate() is
>>>>>> loaded by CPU. Seems like the data dependency should be inserted between the MMU access and the CPU
>>>>>> access. Maybe it is hardware’s guarantee?
>>>>> I just found the comment in pmd_install() explained why most arch has no read
>>>>
>>>> I think pmd_install() is a little different as well. We should make sure the
>>>> page table walker (like GUP) see the correct PTE entry after they see the pmd
>>>> entry.
>>>
>>> The difference I can see is that pmd/pte thing has both hardware page walker and
>>> software page walker (like GUP) as read side. While the case here only has hardware
>>> page walker as read side. But I suppose the memory barrier requirement still apply
>>> here.
>>
>> I am not against this change. Just in order to make me get a better understanding of
>> hardware behavior.
>>
>>>
>>> Maybe we could do a test: add large delay between reset_struct_page() and set_pte_at?
>>
>> Hi Miaohe,
>>
>> Would you mind doing this test? One thread do vmemmap_restore_pte(), another thread
>> detect if it can see a tail page with PG_head after the previous thread has executed
>> set_pte_at().
>
> Will it be easier to construct the memory reorder manually like below?
>
> vmemmap_restore_pte()
> ...
> set_pte_at(&init_mm, addr, pte, mk_pte(page, pgprot));
> /* might a delay. */
> copy_page(to, (void *)walk->reuse_addr);
> reset_struct_pages(to);
This should be correct change for the testing. :).
Regards
Yin, Fengwei
>
> And another thread detects whether it can see a tail page with some invalid fields? If so,
> it seems the problem will always trigger? If not, we depend on the observed meory reorder
> and set_pte_at doesn't contain a memory barrier?
>
> Thanks,
> Miaohe Lin
>
> On Aug 18, 2022, at 20:58, Miaohe Lin <[email protected]> wrote:
>
> On 2022/8/18 17:18, Muchun Song wrote:
>>
>>
>>> On Aug 18, 2022, at 16:54, Yin, Fengwei <[email protected]> wrote:
>>>
>>>
>>>
>>> On 8/18/2022 4:40 PM, Muchun Song wrote:
>>>>
>>>>
>>>>> On Aug 18, 2022, at 16:32, Yin, Fengwei <[email protected]> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 8/18/2022 3:59 PM, Muchun Song wrote:
>>>>>>
>>>>>>
>>>>>>> On Aug 18, 2022, at 15:52, Miaohe Lin <[email protected]> wrote:
>>>>>>>
>>>>>>> On 2022/8/18 10:47, Muchun Song wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> On Aug 18, 2022, at 10:00, Yin, Fengwei <[email protected]> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 8/18/2022 9:55 AM, Miaohe Lin wrote:
>>>>>>>>>>>>> /*
>>>>>>>>>>>>> * The memory barrier inside __SetPageUptodate makes sure that
>>>>>>>>>>>>> * preceding stores to the page contents become visible before
>>>>>>>>>>>>> * the set_pte_at() write.
>>>>>>>>>>>>> */
>>>>>>>>>>>>> __SetPageUptodate(page);
>>>>>>>>>>>> IIUC, the case here we should make sure others (CPUs) can see new page’s
>>>>>>>>>>>> contents after they have saw PG_uptodate is set. I think commit 0ed361dec369
>>>>>>>>>>>> can tell us more details.
>>>>>>>>>>>>
>>>>>>>>>>>> I also looked at commit 52f37629fd3c to see why we need a barrier before
>>>>>>>>>>>> set_pte_at(), but I didn’t find any info to explain why. I guess we want
>>>>>>>>>>>> to make sure the order between the page’s contents and subsequent memory
>>>>>>>>>>>> accesses using the corresponding virtual address, do you agree with this?
>>>>>>>>>>> This is my understanding also. Thanks.
>>>>>>>>>> That's also my understanding. Thanks both.
>>>>>>>>> I have an unclear thing (not related with this patch directly): Who is response
>>>>>>>>> for the read barrier in the read side in this case?
>>>>>>>>>
>>>>>>>>> For SetPageUptodate, there are paring write/read memory barrier.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I have the same question. So I think the example proposed by Miaohe is a little
>>>>>>>> difference from the case (hugetlb_vmemmap) here.
>>>>>>>
>>>>>>> Per my understanding, memory barrier in PageUptodate() is needed because user might access the
>>>>>>> page contents using page_address() (corresponding pagetable entry already exists) soon. But for
>>>>>>> the above proposed case, if user wants to access the page contents, the corresponding pagetable
>>>>>>> should be visible first or the page contents can't be accessed. So there should be a data dependency
>>>>>>> acting as memory barrier between pagetable entry is loaded and page contents is accessed.
>>>>>>> Or am I miss something?
>>>>>>
>>>>>> Yep, it is a data dependency. The difference between hugetlb_vmemmap and PageUptodate() is that
>>>>>> the page table (a pointer to the mapped page frame) is loaded by MMU while PageUptodate() is
>>>>>> loaded by CPU. Seems like the data dependency should be inserted between the MMU access and the CPU
>>>>>> access. Maybe it is hardware’s guarantee?
>>>>> I just found the comment in pmd_install() explained why most arch has no read
>>>>
>>>> I think pmd_install() is a little different as well. We should make sure the
>>>> page table walker (like GUP) see the correct PTE entry after they see the pmd
>>>> entry.
>>>
>>> The difference I can see is that pmd/pte thing has both hardware page walker and
>>> software page walker (like GUP) as read side. While the case here only has hardware
>>> page walker as read side. But I suppose the memory barrier requirement still apply
>>> here.
>>
>> I am not against this change. Just in order to make me get a better understanding of
>> hardware behavior.
>>
>>>
>>> Maybe we could do a test: add large delay between reset_struct_page() and set_pte_at?
>>
>> Hi Miaohe,
>>
>> Would you mind doing this test? One thread do vmemmap_restore_pte(), another thread
>> detect if it can see a tail page with PG_head after the previous thread has executed
>> set_pte_at().
>
> Will it be easier to construct the memory reorder manually like below?
>
> vmemmap_restore_pte()
> ...
> set_pte_at(&init_mm, addr, pte, mk_pte(page, pgprot));
> /* might a delay. */
> copy_page(to, (void *)walk->reuse_addr);
> reset_struct_pages(to);
Well, you have changed the code ordering. I thought we don’t change the code
ordering. Just let the hardware do reordering. The ideal scenario would be
as follows.
CPU0: CPU1:
vmemmap_restore_pte()
copy_page(to, (void *)walk->reuse_addr);
reset_struct_pages(to); // clear the tail page’s PG_head
set_pte_at(&init_mm, addr, pte, mk_pte(page, pgprot));
// Detect if it can see a tail page with PG_head.
I should admit it is a little difficult to construct the scenario. After more
thought, I think here should be inserted a barrier. So:
Reviewed-by: Muchun Song <[email protected]>
Thanks.
>
> And another thread detects whether it can see a tail page with some invalid fields? If so,
> it seems the problem will always trigger? If not, we depend on the observed meory reorder
> and set_pte_at doesn't contain a memory barrier?
>
> Thanks,
> Miaohe Lin
On 2022/8/19 11:19, Muchun Song wrote:
>
>
>> On Aug 18, 2022, at 20:58, Miaohe Lin <[email protected]> wrote:
>>
>> On 2022/8/18 17:18, Muchun Song wrote:
>>>
>>>
>>>> On Aug 18, 2022, at 16:54, Yin, Fengwei <[email protected]> wrote:
>>>>
>>>>
>>>>
>>>> On 8/18/2022 4:40 PM, Muchun Song wrote:
>>>>>
>>>>>
>>>>>> On Aug 18, 2022, at 16:32, Yin, Fengwei <[email protected]> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 8/18/2022 3:59 PM, Muchun Song wrote:
>>>>>>>
>>>>>>>
>>>>>>>> On Aug 18, 2022, at 15:52, Miaohe Lin <[email protected]> wrote:
>>>>>>>>
>>>>>>>> On 2022/8/18 10:47, Muchun Song wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> On Aug 18, 2022, at 10:00, Yin, Fengwei <[email protected]> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 8/18/2022 9:55 AM, Miaohe Lin wrote:
>>>>>>>>>>>>>> /*
>>>>>>>>>>>>>> * The memory barrier inside __SetPageUptodate makes sure that
>>>>>>>>>>>>>> * preceding stores to the page contents become visible before
>>>>>>>>>>>>>> * the set_pte_at() write.
>>>>>>>>>>>>>> */
>>>>>>>>>>>>>> __SetPageUptodate(page);
>>>>>>>>>>>>> IIUC, the case here we should make sure others (CPUs) can see new page’s
>>>>>>>>>>>>> contents after they have saw PG_uptodate is set. I think commit 0ed361dec369
>>>>>>>>>>>>> can tell us more details.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I also looked at commit 52f37629fd3c to see why we need a barrier before
>>>>>>>>>>>>> set_pte_at(), but I didn’t find any info to explain why. I guess we want
>>>>>>>>>>>>> to make sure the order between the page’s contents and subsequent memory
>>>>>>>>>>>>> accesses using the corresponding virtual address, do you agree with this?
>>>>>>>>>>>> This is my understanding also. Thanks.
>>>>>>>>>>> That's also my understanding. Thanks both.
>>>>>>>>>> I have an unclear thing (not related with this patch directly): Who is response
>>>>>>>>>> for the read barrier in the read side in this case?
>>>>>>>>>>
>>>>>>>>>> For SetPageUptodate, there are paring write/read memory barrier.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I have the same question. So I think the example proposed by Miaohe is a little
>>>>>>>>> difference from the case (hugetlb_vmemmap) here.
>>>>>>>>
>>>>>>>> Per my understanding, memory barrier in PageUptodate() is needed because user might access the
>>>>>>>> page contents using page_address() (corresponding pagetable entry already exists) soon. But for
>>>>>>>> the above proposed case, if user wants to access the page contents, the corresponding pagetable
>>>>>>>> should be visible first or the page contents can't be accessed. So there should be a data dependency
>>>>>>>> acting as memory barrier between pagetable entry is loaded and page contents is accessed.
>>>>>>>> Or am I miss something?
>>>>>>>
>>>>>>> Yep, it is a data dependency. The difference between hugetlb_vmemmap and PageUptodate() is that
>>>>>>> the page table (a pointer to the mapped page frame) is loaded by MMU while PageUptodate() is
>>>>>>> loaded by CPU. Seems like the data dependency should be inserted between the MMU access and the CPU
>>>>>>> access. Maybe it is hardware’s guarantee?
>>>>>> I just found the comment in pmd_install() explained why most arch has no read
>>>>>
>>>>> I think pmd_install() is a little different as well. We should make sure the
>>>>> page table walker (like GUP) see the correct PTE entry after they see the pmd
>>>>> entry.
>>>>
>>>> The difference I can see is that pmd/pte thing has both hardware page walker and
>>>> software page walker (like GUP) as read side. While the case here only has hardware
>>>> page walker as read side. But I suppose the memory barrier requirement still apply
>>>> here.
>>>
>>> I am not against this change. Just in order to make me get a better understanding of
>>> hardware behavior.
>>>
>>>>
>>>> Maybe we could do a test: add large delay between reset_struct_page() and set_pte_at?
>>>
>>> Hi Miaohe,
>>>
>>> Would you mind doing this test? One thread do vmemmap_restore_pte(), another thread
>>> detect if it can see a tail page with PG_head after the previous thread has executed
>>> set_pte_at().
>>
>> Will it be easier to construct the memory reorder manually like below?
>>
>> vmemmap_restore_pte()
>> ...
>> set_pte_at(&init_mm, addr, pte, mk_pte(page, pgprot));
>> /* might a delay. */
>> copy_page(to, (void *)walk->reuse_addr);
>> reset_struct_pages(to);
>
>
> Well, you have changed the code ordering. I thought we don’t change the code
> ordering. Just let the hardware do reordering. The ideal scenario would be
> as follows.
>
>
> CPU0: CPU1:
>
> vmemmap_restore_pte()
> copy_page(to, (void *)walk->reuse_addr);
> reset_struct_pages(to); // clear the tail page’s PG_head
> set_pte_at(&init_mm, addr, pte, mk_pte(page, pgprot));
> // Detect if it can see a tail page with PG_head.
>
> I should admit it is a little difficult to construct the scenario. After more
> thought, I think here should be inserted a barrier. So:
>
> Reviewed-by: Muchun Song <[email protected]>
Many thanks both for review and discussion. :)
Thanks,
Miaohe Lin
> On Aug 16, 2022, at 21:05, Miaohe Lin <[email protected]> wrote:
>
> The memory barrier smp_wmb() is needed to make sure that preceding stores
> to the page contents become visible before the below set_pte_at() write.
I found another place where is a similar case. See kasan_populate_vmalloc_pte() in
mm/kasan/shadow.c.
Should we fix it as well?
>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> mm/hugetlb_vmemmap.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
> index 20f414c0379f..76b2d03a0d8d 100644
> --- a/mm/hugetlb_vmemmap.c
> +++ b/mm/hugetlb_vmemmap.c
> @@ -287,6 +287,11 @@ static void vmemmap_restore_pte(pte_t *pte, unsigned long addr,
> copy_page(to, (void *)walk->reuse_addr);
> reset_struct_pages(to);
>
> + /*
> + * Makes sure that preceding stores to the page contents become visible
> + * before the set_pte_at() write.
> + */
> + smp_wmb();
> set_pte_at(&init_mm, addr, pte, mk_pte(page, pgprot));
> }
>
> --
> 2.23.0
>
>
On 2022/8/20 16:12, Muchun Song wrote:
>
>
>> On Aug 16, 2022, at 21:05, Miaohe Lin <[email protected]> wrote:
>>
>> The memory barrier smp_wmb() is needed to make sure that preceding stores
>> to the page contents become visible before the below set_pte_at() write.
>
> I found another place where is a similar case. See kasan_populate_vmalloc_pte() in
> mm/kasan/shadow.c.
Thanks for your report.
>
> Should we fix it as well?
I'm not familiar with kasan yet, but I think memory barrier is needed here or memory corrupt
can't be detected until the contents are visible. smp_mb__after_atomic before set_pte_at should
be enough? What's your opinion?
Thanks,
Miaohe Lin
> On Aug 22, 2022, at 16:45, Miaohe Lin <[email protected]> wrote:
>
> On 2022/8/20 16:12, Muchun Song wrote:
>>
>>
>>> On Aug 16, 2022, at 21:05, Miaohe Lin <[email protected]> wrote:
>>>
>>> The memory barrier smp_wmb() is needed to make sure that preceding stores
>>> to the page contents become visible before the below set_pte_at() write.
>>
>> I found another place where is a similar case. See kasan_populate_vmalloc_pte() in
>> mm/kasan/shadow.c.
>
> Thanks for your report.
>
>>
>> Should we fix it as well?
>
> I'm not familiar with kasan yet, but I think memory barrier is needed here or memory corrupt
> can't be detected until the contents are visible. smp_mb__after_atomic before set_pte_at should
> be enough? What's your opinion?
I didn’t see any atomic operation between set_pte_at() and memset(), I don’t think
smp_mb__after_atomic() is feasible if we really need to insert a barrier. I suggest
you to send a RFC patch to KASAN maintainers, they are more familiar with this than
us.
Thanks.
>
> Thanks,
> Miaohe Lin
>
>
On 2022/8/22 18:23, Muchun Song wrote:
>
>
>> On Aug 22, 2022, at 16:45, Miaohe Lin <[email protected]> wrote:
>>
>> On 2022/8/20 16:12, Muchun Song wrote:
>>>
>>>
>>>> On Aug 16, 2022, at 21:05, Miaohe Lin <[email protected]> wrote:
>>>>
>>>> The memory barrier smp_wmb() is needed to make sure that preceding stores
>>>> to the page contents become visible before the below set_pte_at() write.
>>>
>>> I found another place where is a similar case. See kasan_populate_vmalloc_pte() in
>>> mm/kasan/shadow.c.
>>
>> Thanks for your report.
>>
>>>
>>> Should we fix it as well?
>>
>> I'm not familiar with kasan yet, but I think memory barrier is needed here or memory corrupt
>> can't be detected until the contents are visible. smp_mb__after_atomic before set_pte_at should
>> be enough? What's your opinion?
>
> I didn’t see any atomic operation between set_pte_at() and memset(), I don’t think
> smp_mb__after_atomic() is feasible if we really need to insert a barrier. I suggest
Oh, it should be smp_mb__after_spinlock(), i.e. something like below:
diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c
index 0e3648b603a6..38e503c89740 100644
--- a/mm/kasan/shadow.c
+++ b/mm/kasan/shadow.c
@@ -277,6 +277,7 @@ static int kasan_populate_vmalloc_pte(pte_t *ptep, unsigned long addr,
spin_lock(&init_mm.page_table_lock);
if (likely(pte_none(*ptep))) {
+ smp_mb__after_spinlock();
set_pte_at(&init_mm, addr, ptep, pte);
page = 0;
}
Does this make sense for you?
> you to send a RFC patch to KASAN maintainers, they are more familiar with this than
> us.
Sounds like a good idea. Will do it.
Thanks,
Miaohe Lin