Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <019c1272-9d01-9d51-91a0-2d656b25c318@intel.com>
Date:   Thu, 18 Aug 2022 09:14:41 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.12.0
Subject: Re: [PATCH 4/6] mm: hugetlb_vmemmap: add missing smp_wmb() before
 set_pte_at()
Content-Language: en-US
To:     Muchun Song <muchun.song@linux.dev>,
        Miaohe Lin <linmiaohe@huawei.com>
CC:     Andrew Morton <akpm@linux-foundation.org>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Muchun Song <songmuchun@bytedance.com>,
        Linux MM <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>
References: <20220816130553.31406-1-linmiaohe@huawei.com>
 <20220816130553.31406-5-linmiaohe@huawei.com>
 <D8C00BDA-160D-40CE-AFBD-9488F85E76CE@linux.dev>
 <ea67ab10-667e-f361-b80f-dafb13da4808@huawei.com>
 <0EAF1279-6A1C-41FA-9A32-414C36B3792A@linux.dev>
From:   "Yin, Fengwei" <fengwei.yin@intel.com>
In-Reply-To: <0EAF1279-6A1C-41FA-9A32-414C36B3792A@linux.dev>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cVBoSWNzdjMwMnMySmQzcG90blpkeEtLSThndlFiVFY4SENQNEpPb05wQVBx?=
 =?utf-8?B?VEhPamxTZDNwbEdOVnd0QzFUd1JZdDByVzNMNVJheDhTMllCN0R0c2JLdXlX?=
 =?utf-8?B?STI4a2RaeWplcWNGOXBNWGt0MmJWdlVVMG16cXJoZkVZcS9mT2lQUGVwcW00?=
 =?utf-8?B?VGdMa3F2RjVJc2dKbUFuaVlDb2tLb2oyR3R0dDczaHNNb2NRTytsbEk3cEZl?=
 =?utf-8?B?WkpweEt6S1hxZUhkbm5xeXNDcXN3eTMxejhjMjl0RGRYK3l5bGtCSTZCM0s2?=
 =?utf-8?B?a3ZKYXRWN1FNU2JFQVY1VWttaTlMZEJHQTFsUVdjaDg0STNDbnlpSHZRcWpV?=
 =?utf-8?B?YWtJM1V3TmtSUXZyK1VBa2dyZzFteTBBZzMrYWZJMXlVSVg3bUg1eDlkZ3E4?=
 =?utf-8?B?THBSK3A4d2w4dFdhamVOa0R4VFBQeGs5MG9zZFZVcnM3ejViamNrVEt3TzVw?=
 =?utf-8?B?TFc2czhFVFN0TThBQXllLzU3TVUvTG5NUy9helU5TVVSVUJhQ3FsUVdLcVdS?=
 =?utf-8?B?c1dRZGZDRlpIR3JMUDhidkQ2Z0tpS01EemFUMUw2SXhwQzdSYmNOcVpNVGNJ?=
 =?utf-8?B?V1JVODJTV1FYSnlMTUtkZllxRlRMck4xWmhBMy9oNjV6M1ROTEZWRW5HTjl0?=
 =?utf-8?B?WUsxWXZ4TTE5dW1od1puUVk1RG5TNzlOWHBsRlk3TllmaCtwZHRKcnA0S0hk?=
 =?utf-8?B?WmJEVnN1aEljQ0p2dUtzeFJqK3BVREx2VDV5b1REL09ESlVnWGo4VFhYOUlU?=
 =?utf-8?B?VmV3L1o1Tjh3aThJWE1hcVhvS0FXQnVNZk5udkJVbWhkWHMzS0tIK00vRUZ6?=
 =?utf-8?B?R01GY0pUSHU5UDVOalJTak52aW1NMXVwRlNhM1IvVnNCd2lVMStwNDFHc245?=
 =?utf-8?B?eHZGaUNiRGx4UDFTaTluNDB0cVhMK3dac0tZVHFyUzFYb001NnArVS9oRXJz?=
 =?utf-8?B?b2xWcTBEOXlUeXlORDZxbmtJUUN2TjNObEtzQ3czS2NiYVVqOHE1WWtnNHpN?=
 =?utf-8?B?dDBFelcxdDhub2lSd1lYdFZ2Wlk4UHRReGVsSHNUVlZyaXc5TWxXQmNFOEtM?=
 =?utf-8?B?Zjk3MmRRWm5zK2R0eXRlcmV5V1Q1ZjM4RzlJMVVOZVVCYkhXUWQvN1RIY1BL?=
 =?utf-8?B?bjdFd3UzelBMZ0ZLVGVSUjVCZ3M5My9XdndjbHNpQ3hUWkNhMU9UbHA3bHhB?=
 =?utf-8?B?M0EwQjA3d29seENlem5uRjJSU2V1dkh2WDVFaU1VTVZIRUthOTZ2aTJkYXVn?=
 =?utf-8?B?Z1hRMGU5L25pdE9IMXFLcEJtVnluQ2lObG9laklxS0FGR3hPdDIrVjFVUmNw?=
 =?utf-8?B?ckd6VUtFUkw5Nmh0MVVVcElwSzVMRWtaWURmT3pKZWhxc1Y1Wi9sUW9WZlVT?=
 =?utf-8?B?M3llU1lzdWZHdFlTNndXVEozaG5HeVVEcHNSL0tBcmFibmNubWhMZXpOajc5?=
 =?utf-8?B?T0Mya0JqZm1tM0JCUXlKVUo1bThOQXVKWE1pUjEweUN1bHAyS2VYblBxWTNM?=
 =?utf-8?B?UDVmeWszSzBwRnhIcGZaTVdxK20xUFRxUmlVMGtMTU9SVjR2bSt4aFBwbFdE?=
 =?utf-8?B?Skk3TjZ6Rkd1RitVZmo3Sm5xUllTUXMzVEZaTExlRzlxcVI1eTBaN081YUlH?=
 =?utf-8?B?dmNobk9kekFrMjByRk5SQjN6Vi9aaVJ3Uy9oK2ozNHZhNHBvenVYWUFSREo1?=
 =?utf-8?B?elMwTldCVWxQWDVENXFMbldpcHROOFVoeFVSNCtUMEFBOXVkOW9FSXhTUjg4?=
 =?utf-8?B?cUl6ZDc1bVhzSG5pTVZqZXJIVEtCMm5pMzJCNGdaMmJiZ3RLQ21oaDlMZGJ1?=
 =?utf-8?B?UmdTREI4VTY2K0pKemo1N3poVnFrZXQvUEVlMUFlalNGVjgxQ2tJcUlIZk1C?=
 =?utf-8?B?bXhiSW1XTkd4TGdaQWp1R0QyUHZ5UzNYTDEyUmkzZFJETktYUWVWekhva040?=
 =?utf-8?B?L3cxQlZxcDVlNngwMm1DTjdYcm0rd0lYWDIxeExMdFNwRFZxRkFSdjZ0ZjJS?=
 =?utf-8?B?YVVrdEpsUkxxdUpxWUdDbFZrMnFLMzBGMC83L1VaNVFJTGNRL2NJQ3dDZU8x?=
 =?utf-8?B?OFNocFdQbk1Maldsc0ZRdGE4WXdoVElIamV4YmhWb3FabVJYcXo3ekQ4OWlj?=
 =?utf-8?Q?BXHYQxTdXQort4KiZyWdSvYCS?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9efa2c4a-ad55-4c03-3c79-08da80b70bb8
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4820.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Aug 2022 01:14:49.3309
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: aW5syODNNLQalUcFdOn9lE0lF0vKfBW0yQdtiIiYf4cHHDutv4LuhYc5ndYKMP4r21xkmM0gTucsTC67Vs1rBQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB3573
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 8/17/2022 7:21 PM, Muchun Song wrote:
> 
> 
>> On Aug 17, 2022, at 16:41, Miaohe Lin <linmiaohe@huawei.com> wrote:
>>
>> On 2022/8/17 10:53, Muchun Song wrote:
>>>
>>>
>>>> On Aug 16, 2022, at 21:05, Miaohe Lin <linmiaohe@huawei.com> 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.
>>>
> 
>