From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5814BECAAD5 for ; Fri, 2 Sep 2022 15:37:50 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 838C68D0026; Fri, 2 Sep 2022 11:37:49 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7E9448D0014; Fri, 2 Sep 2022 11:37:49 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 68AC28D0026; Fri, 2 Sep 2022 11:37:49 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 5B71D8D0014 for ; Fri, 2 Sep 2022 11:37:49 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 35F5DAB906 for ; Fri, 2 Sep 2022 15:37:49 +0000 (UTC) X-FDA: 79867550658.12.36D5A17 Received: from mail-pf1-f172.google.com (mail-pf1-f172.google.com [209.85.210.172]) by imf21.hostedemail.com (Postfix) with ESMTP id D3B6E1C0067 for ; Fri, 2 Sep 2022 15:37:48 +0000 (UTC) Received: by mail-pf1-f172.google.com with SMTP id w139so2219450pfc.13 for ; Fri, 02 Sep 2022 08:37:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=wxuJKVL4TdrpBfaqB67ZFhkoV3FR0V/WApM6+UkhSDY=; b=logBeReqwhiMPJ+9Et4jQQRif3S4/EFNghg66d1akxtF4CgpHkr2/2eH0P+FOUZ8YW ni5I1humC6Jpi8wTov8hh67azGrQl7qqeyqH/+M4EJWbGfzc8+ixDdOTRBAxyvkRCUQ+ vha6jR73f3AKIaAMoa+mrNwkUhL5RkCVw/Yc0JOwd4hGzJJB5iQ+t1a9J0J9YIxmpVwQ VCIO9ZaG/moxpcrb+UXJIVwFUKVVGER3GTv/w2xgIyG+EyPLqqWzogh80kg+zp6MQEy6 JWAFV7lWdn75iIyxFMgGpp5alFnb+X6oQeP4z7wgCiEHVQN2D76SgznaxasS5criYUwV 7/yw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=wxuJKVL4TdrpBfaqB67ZFhkoV3FR0V/WApM6+UkhSDY=; b=Dq9mztfwfzTC8LNhDLyFHqJe1kfKDKY/1eI0PQV28bd04l/3uflIRWHaKmO/llHTIj UDGk+RJgNC2TMi2r+MS2y4nsaC4DvLQthrAplutxiCDMA4sijGLpO50FbUQqXSZAgvkv O1Xyvzx3QWtMdAcVkS7AJ8xo0a5mRd+NlJE7tQo2s+rMTzzE43ON314PJ1sQIeydho8F gDoM8HbOVTVVxVKc88M6Ijp2kDQ2I9QCWa4eOkeuv3lc4vIk+QFDkeLNxtwvlGx2vHjF pSssZ/PJyclt4bNPZLCffLVAhgKyT25SLGafZGIoApz5bBzXOLNIiReSB5ktSzhOBao+ Yc3A== X-Gm-Message-State: ACgBeo2pI8wSBZn18j8foJVQ/FNDA9l14mUygD+aRlZOuqw+kFGPj+h+ cz6VQEcCA4EEE0OXtTl53/77QC69dj8wbEOctEk= X-Google-Smtp-Source: AA6agR73Ctcqg5Eu+vup07FNPO1+IPQ12OI47E1xnwJpeMPLt6l9qFgI30lW0WWHlbFLAtBMFoVHJEjBLxucssvaGc0= X-Received: by 2002:a63:de01:0:b0:42b:31f7:b6ea with SMTP id f1-20020a63de01000000b0042b31f7b6eamr30333941pgg.587.1662133067862; Fri, 02 Sep 2022 08:37:47 -0700 (PDT) MIME-Version: 1.0 References: <20220901072119.37588-1-david@redhat.com> <2368d91f-8442-076f-f33a-64b51b44825c@redhat.com> In-Reply-To: <2368d91f-8442-076f-f33a-64b51b44825c@redhat.com> From: Yang Shi Date: Fri, 2 Sep 2022 08:37:35 -0700 Message-ID: Subject: Re: [PATCH v1] mm/gup: adjust stale comment for RCU GUP-fast To: David Hildenbrand Cc: Peter Xu , linux-kernel@vger.kernel.org, linux-mm@kvack.org, "Kirill A . Shutemov" , "Aneesh Kumar K . V" , Vlastimil Babka , Jerome Marchand , Andrea Arcangeli , Hugh Dickins , Jason Gunthorpe , John Hubbard Content-Type: text/plain; charset="UTF-8" ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1662133068; a=rsa-sha256; cv=none; b=eN1wWvlMKEq9P1HmYSeRbqu3BE9DJVE0YVvOubXwdMXydLn1zuyjvKDDWj/hPRalpVRROY /ZB6HVpCx1hARPeEIGRvxYb/GVFSkApZYl7bvnLc9KJHFd3aeeVsQmZx66slXlk7FDKi0n E7BHcn9MQkXteBza9p+gtzhZ8P+76jE= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=logBeReq; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf21.hostedemail.com: domain of shy828301@gmail.com designates 209.85.210.172 as permitted sender) smtp.mailfrom=shy828301@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1662133068; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=wxuJKVL4TdrpBfaqB67ZFhkoV3FR0V/WApM6+UkhSDY=; b=v3xcWVmJ2VXDpwE6ujE87T6uH36IlTQoLK+CFCpDlUDLJd2w1X3M+kyH0azTVkL96of8GF EDS0rocsthDn0FHxZun54BbfAfzxkzB5I53a8gEe783mNeKG1EUg16qXwCHNzFzJ1wuJPQ FJzW4tabZ3WayJ0WgspDijoU4qXvHC8= X-Stat-Signature: 75chzrnrdtw56occtmex6yfbh1hreqf4 X-Rspamd-Queue-Id: D3B6E1C0067 Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=logBeReq; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf21.hostedemail.com: domain of shy828301@gmail.com designates 209.85.210.172 as permitted sender) smtp.mailfrom=shy828301@gmail.com X-Rspam-User: X-Rspamd-Server: rspam02 X-HE-Tag: 1662133068-438544 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Sep 1, 2022 at 11:32 PM David Hildenbrand wrote: > > On 01.09.22 20:35, Yang Shi wrote: > > On Thu, Sep 1, 2022 at 11:07 AM Peter Xu wrote: > >> > >> On Thu, Sep 01, 2022 at 10:50:48AM -0700, Yang Shi wrote: > >>> Yeah, because THP collapse does copy the data before clearing pte. If > >>> we want to remove pmdp_collapse_flush() by just clearing pmd, we > >>> should clear *AND* flush pte before copying the data IIRC. > >> > >> Yes tlb flush is still needed. IIUC the generic pmdp_collapse_flush() will > >> still be working (with the pte level flushing there) but it should just > >> start to work for all archs, so potentially we could drop the arch-specific > >> pmdp_collapse_flush()s, mostly the ppc impl. > > > > I'm don't know why powperpc needs to have its specific > > pmdp_collapse_flush() in the first place, not only the mandatory IPI > > broadcast, but also the specific implementation of pmd tlb flush. But > > anyway the IPI broadcast could be removed at least IMO. > > > > pmdp_collapse_flush() is overwritten on book3s only. It either translates > to radix__pmdp_collapse_flush() or hash__pmdp_collapse_flush(). > > > radix__pmdp_collapse_flush() has a comment explaining the situation: > > > + /* > + * pmdp collapse_flush need to ensure that there are no parallel gup > + * walk after this call. This is needed so that we can have stable > + * page ref count when collapsing a page. We don't allow a collapse page > + * if we have gup taken on the page. We can ensure that by sending IPI > + * because gup walk happens with IRQ disabled. > + */ > > > The comment for hash__pmdp_collapse_flush() is a bit more involved: > > /* > * Wait for all pending hash_page to finish. This is needed > * in case of subpage collapse. When we collapse normal pages > * to hugepage, we first clear the pmd, then invalidate all > * the PTE entries. The assumption here is that any low level > * page fault will see a none pmd and take the slow path that > * will wait on mmap_lock. But we could very well be in a > * hash_page with local ptep pointer value. Such a hash page > * can result in adding new HPTE entries for normal subpages. > * That means we could be modifying the page content as we > * copy them to a huge page. So wait for parallel hash_page > * to finish before invalidating HPTE entries. We can do this > * by sending an IPI to all the cpus and executing a dummy > * function there. > */ > > I'm not sure if that implies that the IPI is needed for some other hash-magic. They do issue IPI broadcast to call a dummy function in order to serialize against fast GUP, please see serialize_against_pte_lookup(), and it does full memory barrier too. I think the IPI broadcast could be removed once my fix is merged and the common pmd clear and memory barrier could be consolidated, now it is duplicated in both radix and hash. > > Maybe Aneesh can clarify. > > >> > >> This also reminded me that the s390 version of pmdp_collapse_flush() is a > >> bit weird, since it doesn't even have the tlb flush there. I feel like > >> it's broken but I can't really tell whether something I've overlooked. > >> Worth an eye on. > > > > I don't know why. But if s390 doesn't flush tlb in > > pmdp_collapse_flush(), then there may be data integrity problem since > > the page is still writable when copying the data because pte is > > cleared after data copying. Or s390 hardware does flush tlb > > automatically? > > s390x does a pmdp_huge_get_and_clear(). > > pmdp_huge_get_and_clear() does an pmdp_xchg_direct(). > > pmdp_xchg_direct() does an pmdp_flush_direct(). > > pmdp_flush_direct() issues an IDTE, which is a TLB flush. Aha, thanks, I didn't look that deep... I stopped looking once I saw pmdp_huge_get_and_clear(), I thought it just does clear... > > > Note that this matches ptep_get_and_clear() behavior on s390x. Quoting the comment in there: > > > /* > * This is hard to understand. ptep_get_and_clear and ptep_clear_flush > * both clear the TLB for the unmapped pte. The reason is that > * ptep_get_and_clear is used in common code (e.g. change_pte_range) > * to modify an active pte. The sequence is > * 1) ptep_get_and_clear > * 2) set_pte_at > * 3) flush_tlb_range > * On s390 the tlb needs to get flushed with the modification of the pte > * if the pte is active. The only way how this can be implemented is to > * have ptep_get_and_clear do the tlb flush. In exchange flush_tlb_range > * is a nop. > */ > > -- > Thanks, > > David / dhildenb >