Re: [PATCH 4/6] mm: hugetlb_vmemmap: add missing smp_wmb() before set_pte_at()

[Muchun Song <muchun.song@linux.dev>]

> On Aug 18, 2022, at 15:52, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> On 2022/8/18 10:47, Muchun Song wrote:
>> 
>> 
>>> On Aug 18, 2022, at 10:00, Yin, Fengwei <fengwei.yin@intel.com> 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