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: <b51769db-0344-5817-7daf-a2e39c5d2644@nvidia.com>
Date:   Sun, 4 Sep 2022 15:21:57 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.0
Subject: Re: [PATCH] mm: gup: fix the fast GUP race against THP collapse
Content-Language: en-US
To:     Peter Xu <peterx@redhat.com>, Yang Shi <shy828301@gmail.com>
Cc:     david@redhat.com, kirill.shutemov@linux.intel.com, jgg@nvidia.com,
        hughd@google.com, akpm@linux-foundation.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
References: <20220901222707.477402-1-shy828301@gmail.com>
 <YxE/vuQlWJCJMuG2@xz-m1.local>
 <CAHbLzkqjZ_UhUbJ_f9Br7WCAgQvjrm5bMPRsKYvaFc2bzSuzrw@mail.gmail.com>
 <YxIofPiI8jvGzcjC@xz-m1.local>
From:   John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <YxIofPiI8jvGzcjC@xz-m1.local>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Yjl5MnRPa3psdUM5N1Rva2dMMXhRR2ZhMnR4RCtwUTZwL0RzMkdrbno5K0xq?=
 =?utf-8?B?aUZzVTNVRlNHOEZFRGJOVG9RaDJEMzROWTZsblVFcm1wbWtUb1dhQlVIVUVQ?=
 =?utf-8?B?RHFXSWZkTit3VU9jWEpJZkY5b2JKZi96UHVweFNodHBZRGxyaDZkdHB4QnVJ?=
 =?utf-8?B?b040TjgwazVGV21JbXB6SDZPbHVlRFNSc011WW10MGNvZ3h6QVhYZXNxblBs?=
 =?utf-8?B?NVBCeTFNS1JNcktBNGRqbm5lcmhONVpGazlEbFpFc0pCVTdSRjh0c1E0OEdt?=
 =?utf-8?B?eHdrNENML2xOOXhTbmszdW1QMnpXM2FIaUFPelg4V2JVeHczbC90UGtkNzQz?=
 =?utf-8?B?eHNLdGxXN1dDa2tjd1lMVmN1QmRPWkdYM3FtTkZjV2Y4aWYwOWxWSElzcmUx?=
 =?utf-8?B?VEhZMWRmcmZ5Q2p4SmcvK1ZJQ2g2Z1VPYU5ZT2NNczNneUJzbVFsaSt0M2py?=
 =?utf-8?B?NUdXSGRRTlZXa0VyZFY2MlRnMDNmQTlibU54RWlCKzl2UHYwVzNrM0FSRVI2?=
 =?utf-8?B?V0tCV1B5b0o2YmZpS1B0aGJ0SWc4ZEQ4SUtFVVRSMFdXZlJlTk81eDd1aXFU?=
 =?utf-8?B?ckpKeFU5N0VLblhtOU93dGszRWJJaGt4b3JId1ZCaUQvNlVQc3FGaU02MlBp?=
 =?utf-8?B?VllOY2dLVnJxT1ZxdkdiY0Y1Tm01SjBwWi8vT3UxTHhPVFhxcENESUJUTVJS?=
 =?utf-8?B?Zzc2QVBvVERwY2FxZEI1UEgvdCtsU1N2Mis4SUpCaCtvckNmaXFDMERyQ2JC?=
 =?utf-8?B?dm9kRml4ekcxTEs2THRMaFg0cmduVktIa1JnWVVvOGlBcndzZHZDOXB0TVZM?=
 =?utf-8?B?UTAyZktLMGg1d0IxbkFJa29yeHB5UDlPOUdibyttREJMelJ0ZnRRQ3ZvemNR?=
 =?utf-8?B?R210bzA3TW1PMnNaRTNabERqMHpuQUJwaG81UEV0LzhXM3FVckRKZFBqZWtx?=
 =?utf-8?B?QnZmNVBSV1NWK1JaVGt6eS9hb0dObHhVb0pDMHlULzVSOE9ZQm91YWdodlgv?=
 =?utf-8?B?bW5NTVVYSG9XRkIxeGFMUDdlNDhubU9FSkRNQnJPeEdWQVZSZ09EN2xvanRC?=
 =?utf-8?B?R0hBaERpaVBZMERXME1DeGxsZWxRc1FJamlyUUNZcTc0a21rc3UzdFAvQ0NE?=
 =?utf-8?B?b1ZWL1YvVHJkYWlsdUJtd0RKM0pVcXpDSk1TQXN4NVk0OUtjRVN6MnNiWmdS?=
 =?utf-8?B?VXVYaHRrNlh2TVY2eW90UTFmK1RTdURtYUNWV2l2TlZSRU9CcnlPNExha0hD?=
 =?utf-8?B?VER5WTlobkt0VmM2WGhOaUpaWXlRV0d0M081SmZobXd1d3dvTnRDOTFrUTJ6?=
 =?utf-8?B?S1hPYVdQL20wZmtqWlF6MTFSYzhJK3JOT09HcUdVdHBZaWFLdWdqWHB2aGxR?=
 =?utf-8?B?NGtuNEZvUVhhT2o1eGZDazVNNnorQUVtWEI2azJvN2ZWcXcrNlBCZWZ0TGh3?=
 =?utf-8?B?NTVHcnJ3NWg2bjV4NlNhcDlwNHc3VHdYdVFqTi92dTB2dnl2SWRld2x6V3pu?=
 =?utf-8?B?M3IxRy94Ni9UVk5FVjN2ZXhmNmxxYmZITzVCdk1hTmNsOWlBbDNld3JlSDJU?=
 =?utf-8?B?YUZHc0tQOUZZMW9XNkNIbGRTTTA2b1NUczNWZnRnR1ZVS2NXdGordTRlZ05T?=
 =?utf-8?B?VGszZnJ6Y1AvSnhmTU1IYXhUWjBJelZUZkx5OWVIbVR0cWdEaFgzL3hrWFQ0?=
 =?utf-8?B?bS9nblhvSzNKSStOL1NFVGtKSnRaTTlkV1NVc3EvTnZMV3VPN3BueXBKY2Zr?=
 =?utf-8?B?SS83WS91WW5nVnVwdWNzMm1uc3k1MHprTHFBSnZNWGJOUUdTTVoyYTNCWkI2?=
 =?utf-8?B?ejhrK1h2L2lDZlE4eHBFWnE2WkpBY1V3SzFiTmtxS09DMHpsSDVZL1pWYW9R?=
 =?utf-8?B?b0hJcnB1c2FWSTJCZmRybTlBZXpTakViTjg4aERTZzlxZngzbnZuZ045VDcy?=
 =?utf-8?B?ZldWODFubUZQc1d1c0VMT0hpSUR4M2I5L0RjaHZzVC9Dby9Gem4rQ1orU0l0?=
 =?utf-8?B?TldZVVpvNTd6REo2a1lsRE9kbDFzU1ZVZzJka3QwZHlxSmRDeWI1cVY5Sklj?=
 =?utf-8?B?SnJ5dVJyeHkvTWZqNzlvZ0hRdllwaC85eGphZjdiNTBHVFlJaVh2TTJhQ1cw?=
 =?utf-8?Q?fycyXFbACjzLo8gIwyR3TH6WD?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bf5f8ffa-9f9f-46c7-1d30-08da8ec3e280
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4130.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Sep 2022 22:21:59.8191
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: E311E2OjmgUJwPGUxi7ecDifw/rKw3qS6JPK4UDPc7jLX3cVzOgqXL91tO0m6dHsXRPV29Pa6uhe9iClC6Z3rA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR12MB7632
X-Spam-Status: No, score=-2.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE,T_SCC_BODY_TEXT_LINE 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 9/2/22 08:59, Peter Xu wrote:
...
> Would you mind rewording this comment a bit?  I feel it a bit weird to
> suddenly mention about thp collapse especially its details.
> 
> Maybe some statement on the whole history of why check pte, and in what
> case pmd check is needed (where the thp collapse example can be moved to,
> imho)?
> 
> One of my attempt for reference..
> 
> 		/*
> 		 * Fast-gup relies on pte change detection to avoid
> 		 * concurrent pgtable operations.
> 		 *
> 		 * To pin the page, fast-gup needs to do below in order:
> 		 * (1) pin the page (by prefetching pte), then (2) check
> 		 * pte not changed.
> 		 *
> 		 * For the rest of pgtable operations where pgtable updates
> 		 * can be racy with fast-gup, we need to do (1) clear pte,
> 		 * then (2) check whether page is pinned.
> 		 *
> 		 * Above will work for all pte-level operations, including
> 		 * thp split.
> 		 *
> 		 * For thp collapse, it's a bit more complicated because
> 		 * with RCU pgtable free fast-gup can be walking a pgtable
> 		 * page that is being freed (so pte is still valid but pmd
> 		 * can be cleared already).  To avoid race in such
> 		 * condition, we need to also check pmd here to make sure
> 		 * pmd doesn't change (corresponds to pmdp_collapse_flush()
> 		 * in the thp collide code path).
> 		 */
> 
> If you agree with the comment change, feel free to add:
> 
> Acked-by: Peter Xu <peterx@redhat.com>
> 

This seems fine, but I'd like to additionally request that we move it
to function-level documentation. Because it expands the length of the
function, which previously fit neatly on my screen. So I think it's
time to move it up a level.

thanks,

-- 
John Hubbard
NVIDIA