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 F01A7C07E97 for ; Tue, 28 Nov 2023 19:00:31 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6F2F06B034A; Tue, 28 Nov 2023 14:00:31 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 6A3576B034B; Tue, 28 Nov 2023 14:00:31 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5454C8D0001; Tue, 28 Nov 2023 14:00:31 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 435D26B034A for ; Tue, 28 Nov 2023 14:00:31 -0500 (EST) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 04FEE14032E for ; Tue, 28 Nov 2023 19:00:30 +0000 (UTC) X-FDA: 81508279062.16.067BB7E Received: from mail-ua1-f45.google.com (mail-ua1-f45.google.com [209.85.222.45]) by imf28.hostedemail.com (Postfix) with ESMTP id 64DB5C003A for ; Tue, 28 Nov 2023 19:00:27 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=SQx9ZuIv; spf=pass (imf28.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.222.45 as permitted sender) smtp.mailfrom=21cnbao@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1701198027; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=anm1dOFExfLy75gHJZ/W03bfpmv/nns8TAXn2nHoHQI=; b=Z0SaG7Rs7WA4Qx0fnczq5pvMnI7FCFHDpk29qyPrPKHhtTQqFwb0ScHu9zQEjPzXz4EaCE /a/x11Y52bXLijXYmiJwFf8ydaGn5h0SOcDjJg7iTCfJDIzLqZJakJg0sXN0qMdFCgA0Ot RW/J0G2l614vp+u63cEuIVPrErkRbiM= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1701198027; a=rsa-sha256; cv=none; b=M6q3hjIw0Si+zHdYM9iut1Dfw92vZ/++wxFrgaDpx1oEqyP9rjEVBy8nCrs/EhtxmPM1qi OCqeQ8LF/ryd5SZ1NebG4HOP2SnaGOYlIw+t5UylKf1kPBlII24RhIYu0kZZfqS9YdP/BI KWuURPtXlbesbyFb/d9JCU4rLwq0/SE= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=SQx9ZuIv; spf=pass (imf28.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.222.45 as permitted sender) smtp.mailfrom=21cnbao@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-ua1-f45.google.com with SMTP id a1e0cc1a2514c-7c4a41372d3so915503241.1 for ; Tue, 28 Nov 2023 11:00:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1701198026; x=1701802826; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=anm1dOFExfLy75gHJZ/W03bfpmv/nns8TAXn2nHoHQI=; b=SQx9ZuIvXn3N3XzXi0CTeOP9B9PR3bCFb5N5YmwzKoGgUkyQEAb6rQGJ7iBsQTNV7f N7Yqj3Fl1TKqDMuMCpU3svtRQpp4E/5hetsdXHgO/Iw1kHN77CCgYFdq9Wo30aNwozAV AIcrNfOq8NA3Ylshhk+F5+oL3CYSDhfcgqUiEU5WcLA7O067gOWpeHVoh10R9+KgMJuL F2miicXs3umaC2+GPnazl4YwK+SBvjdhCZnR3MSQ0An/9fhyacMeq8gFOYDBkBBkCmuT l0WCcftVwx60SYOPiikxVaEZJxovG9CcleH+obLn9W1yjFCEougLNy8u4Yh+6fCD9Blo THcQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701198026; x=1701802826; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=anm1dOFExfLy75gHJZ/W03bfpmv/nns8TAXn2nHoHQI=; b=kIOXqc1pPISEhbF1EZM0PdVGVO2n08yj9YLfdtdhZ+TGpxyq4GOcDyZ1UYvf61J1B9 49ts7/TRHuw9P9LyVqUWSyhI8jlZnBxmffa6wlGCObl86fRp4iHEW2rqWPk2dYdo71YO 3W8toO5C+ekwkoXlsCjvAgRxJveUKnk9lR9BxoaE/RwD95ctIAYs6LWehGVx05L3JvNi ubPBSRApqK6YLoOWV0ddON9+NParZHhFErostZe+VS7iGgEJdS9RMlYYIsHJeP6uqArh AKOdxBQtoV3tXn72/45Qu6WjxOOhySj6HHZoDOH1BSjzeOxi5R4w6kdIgiFSuqwiuK50 fsBg== X-Gm-Message-State: AOJu0YxOH0dzM42uP2H7e2uGqo4cdravKLHGl6HcTulAPxYcDGRr+Dt9 OW/rKBwmtLrudIy2+lGD+r5S/D7ktOX16uTTb95b9WA+5plDaQBH X-Google-Smtp-Source: AGHT+IHLoiGeuCVOh3TlGs+3ECmEk64qJ+sV0ApYTRDtWpUDW3Xs3BePMa+BoYyAvROWgh4JlqPIl4RzPSt9PjU4dNU= X-Received: by 2002:a05:6102:3bfa:b0:463:d3:513e with SMTP id be26-20020a0561023bfa00b0046300d3513emr6221823vsb.27.1701198024786; Tue, 28 Nov 2023 11:00:24 -0800 (PST) MIME-Version: 1.0 References: <20231115163018.1303287-2-ryan.roberts@arm.com> <20231127055414.9015-1-v-songbaohua@oppo.com> <755343a1-ce94-4d38-8317-0925e2dae3bc@arm.com> In-Reply-To: From: Barry Song <21cnbao@gmail.com> Date: Wed, 29 Nov 2023 08:00:13 +1300 Message-ID: Subject: Re: [PATCH v2 01/14] mm: Batch-copy PTE ranges during fork() To: Ryan Roberts Cc: akpm@linux-foundation.org, andreyknvl@gmail.com, anshuman.khandual@arm.com, ardb@kernel.org, catalin.marinas@arm.com, david@redhat.com, dvyukov@google.com, glider@google.com, james.morse@arm.com, jhubbard@nvidia.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, mark.rutland@arm.com, maz@kernel.org, oliver.upton@linux.dev, ryabinin.a.a@gmail.com, suzuki.poulose@arm.com, vincenzo.frascino@arm.com, wangkefeng.wang@huawei.com, will@kernel.org, willy@infradead.org, yuzenghui@huawei.com, yuzhao@google.com, ziy@nvidia.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 64DB5C003A X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: 4uryz3px3riqwr4o1577qbp8kyy78uwp X-HE-Tag: 1701198027-595455 X-HE-Meta: U2FsdGVkX1/0JAErjP0nqBQTkDmm0tAfsG5soUEebAUC6sULgFt4fA8kHO6EgWeK9sw3x9fp2fzp1xyIMjWI6fVIsGjSb9jojdY7xlr9SrH1O0ZtAHSwmdY4OxTdrlNfkz2umMXvktG/WZ5aP8iW0A8t4YjLuhmo7wMAqg50Pa0Z3AvFmCViLNOQ/ug7OzW1IzdAjmhR2QraQ5B2Ggj0G3HZd2BHVWPSaveE3BSljUBLHU3+YMdjlBSObjvXbhC0i784U6qXVWr11C/yXwcfbO6w/ygu6WRl+SGrK/Zl4mSXLzVOsh3Ay6TtG3t90yajkaKbu6d6eCQcMsNpFwXsz3Krj+GT6elvFxr8BNCbS/xY3EwNpXsUqJanI34uiqV9MdjEdQ9gCJrdhSbKFbTulAaL2za2aoK62nY1EPzDrcpU6A3iKUxdCFoPRolYkor0AxXSo3h3u0jvdIEURObbXsaRCkllPSsNnVHXmRKhA3eBM1cUZ5Z1semPBab8qTU+zXXcUZvRiTLPagyLTmuM/nZfVhOqMfZzu5D0hVtw4Sr5PvL0TNcigRT7t3ctHCiIdX57pk4alPwrNvH1Ay2/SLlHUVG+1J/6/lmiDlXDzjs6t1uPgWaGBXyHvnN3ZRm7payTM9u5sI23cCWhhZuKjx+WpO/2iDIHNzDPrlUXTfkKz6lurfcQ/PzPC/NHutuMondwSWulWKUOyyjIp2hEOfV/Bet8BYxojlWc+9FYClzpOKG7lbx2Yk7VTSnVcA0kYEReegZedEHZXgaBvvgpnI735QBSAtuxnXUzfdQeGV+BkqKoAkX57uPwZ9pXmQ9K6hQCvkbSO9bHsrMzM3hKx+toroWENOkBuyGEO1QreYYqACVQ3t3bxEBPxXERsefLBlTYMcw9/UU2FMDUksD3m9NMTypJKLTrTmDemacHiqDp9W4CZZakX4n2rrQaeXTm/TyfTK5PBb4ikuU/Biz 6noTsMgV rzgfaqg++dCnFwuhgkUzMmXf9VtMPMBwsBH5nERK4r2nFu5cOQGWZrwPfhChgnQHjyKjOMhwx1SzRFc4REexxbihd+nXQ2wrMgODB8h0DXeMwfgUWR22yyvqB6mZG6Y17Q53Dssv6CM8CEJmiayaJcFrv4xlsUJV4MGSiTummLcUVoa3B5DlX6sw/fuoVXckB1MFabE/MLnqX1gRGZ6er4hhm5mjnBmLIN7MY87GyPF5dsfbla68l5xlh/mULjXrpVXcAFj0jSEAjWpqwTElJXy+ZTzMK4f5utKgF424/RIiESaIOUdlXxlqMkD3Psogx+RyCogoWyHo9vSCyluwiSvUamTvNDIGlGqcvrnIaeFhdiUklK3zHkXA1yczaHBhBCZADMByeHEpL9rMWblx29zjAp8KaAFb+PAcVkTaxopMUgY0RvjTed/hWsWopJrjhn/ZKTYYNKOFIwNEOPnl5VHZA00ZYeDBXVq6fn4QJUYAQCQyQm/bpLpFQbEA38Tbwhr9G 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: List-Subscribe: List-Unsubscribe: On Wed, Nov 29, 2023 at 12:00=E2=80=AFAM Ryan Roberts wrote: > > On 28/11/2023 00:11, Barry Song wrote: > > On Mon, Nov 27, 2023 at 10:24=E2=80=AFPM Ryan Roberts wrote: > >> > >> On 27/11/2023 05:54, Barry Song wrote: > >>>> +copy_present_ptes(struct vm_area_struct *dst_vma, struct vm_area_st= ruct *src_vma, > >>>> + pte_t *dst_pte, pte_t *src_pte, > >>>> + unsigned long addr, unsigned long end, > >>>> + int *rss, struct folio **prealloc) > >>>> { > >>>> struct mm_struct *src_mm =3D src_vma->vm_mm; > >>>> unsigned long vm_flags =3D src_vma->vm_flags; > >>>> pte_t pte =3D ptep_get(src_pte); > >>>> struct page *page; > >>>> struct folio *folio; > >>>> + int nr =3D 1; > >>>> + bool anon; > >>>> + bool any_dirty =3D pte_dirty(pte); > >>>> + int i; > >>>> > >>>> page =3D vm_normal_page(src_vma, addr, pte); > >>>> - if (page) > >>>> + if (page) { > >>>> folio =3D page_folio(page); > >>>> - if (page && folio_test_anon(folio)) { > >>>> - /* > >>>> - * If this page may have been pinned by the parent proc= ess, > >>>> - * copy the page immediately for the child so that we'l= l always > >>>> - * guarantee the pinned page won't be randomly replaced= in the > >>>> - * future. > >>>> - */ > >>>> - folio_get(folio); > >>>> - if (unlikely(page_try_dup_anon_rmap(page, false, src_vm= a))) { > >>>> - /* Page may be pinned, we have to copy. */ > >>>> - folio_put(folio); > >>>> - return copy_present_page(dst_vma, src_vma, dst_= pte, src_pte, > >>>> - addr, rss, prealloc, p= age); > >>>> + anon =3D folio_test_anon(folio); > >>>> + nr =3D folio_nr_pages_cont_mapped(folio, page, src_pte,= addr, > >>>> + end, pte, &any_dirty); > >>> > >>> in case we have a large folio with 16 CONTPTE basepages, and userspac= e > >>> do madvise(addr + 4KB * 5, DONTNEED); > >> > >> nit: if you are offsetting by 5 pages from addr, then below I think yo= u mean > >> page0~page4 and page6~15? > >> > >>> > >>> thus, the 4th basepage of PTE becomes PTE_NONE and folio_nr_pages_con= t_mapped() > >>> will return 15. in this case, we should copy page0~page3 and page5~pa= ge15. > >> > >> No I don't think folio_nr_pages_cont_mapped() will return 15; that's c= ertainly > >> not how its intended to work. The function is scanning forwards from t= he current > >> pte until it finds the first pte that does not fit in the batch - eith= er because > >> it maps a PFN that is not contiguous, or because the permissions are d= ifferent > >> (although this is being relaxed a bit; see conversation with DavidH ag= ainst this > >> same patch). > >> > >> So the first time through this loop, folio_nr_pages_cont_mapped() will= return 5, > >> (page0~page4) then the next time through the loop we will go through t= he > >> !present path and process the single swap marker. Then the 3rd time th= rough the > >> loop folio_nr_pages_cont_mapped() will return 10. > > > > one case we have met by running hundreds of real phones is as below, > > > > > > static int > > copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *s= rc_vma, > > pmd_t *dst_pmd, pmd_t *src_pmd, unsigned long addr, > > unsigned long end) > > { > > ... > > dst_pte =3D pte_alloc_map_lock(dst_mm, dst_pmd, addr, &dst_ptl)= ; > > if (!dst_pte) { > > ret =3D -ENOMEM; > > goto out; > > } > > src_pte =3D pte_offset_map_nolock(src_mm, src_pmd, addr, &src_p= tl); > > if (!src_pte) { > > pte_unmap_unlock(dst_pte, dst_ptl); > > /* ret =3D=3D 0 */ > > goto out; > > } > > spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING); > > orig_src_pte =3D src_pte; > > orig_dst_pte =3D dst_pte; > > arch_enter_lazy_mmu_mode(); > > > > do { > > /* > > * We are holding two locks at this point - either of t= hem > > * could generate latencies in another task on another = CPU. > > */ > > if (progress >=3D 32) { > > progress =3D 0; > > if (need_resched() || > > spin_needbreak(src_ptl) || spin_needbreak(d= st_ptl)) > > break; > > } > > ptent =3D ptep_get(src_pte); > > if (pte_none(ptent)) { > > progress++; > > continue; > > } > > > > the above iteration can break when progress > =3D32. for example, at th= e > > beginning, > > if all PTEs are none, we break when progress >=3D32, and we break when = we > > are in the 8th pte of 16PTEs which might become CONTPTE after we releas= e > > PTL. > > > > since we are releasing PTLs, next time when we get PTL, those pte_none(= ) might > > become pte_cont(), then are you going to copy CONTPTE from 8th pte, > > thus, immediately > > break the consistent CONPTEs rule of hardware? > > > > pte0 - pte_none > > pte1 - pte_none > > ... > > pte7 - pte_none > > > > pte8 - pte_cont > > ... > > pte15 - pte_cont > > > > so we did some modification to avoid a break in the middle of PTEs > > which can potentially > > become CONTPE. > > do { > > /* > > * We are holding two locks at this point - either of th= em > > * could generate latencies in another task on another C= PU. > > */ > > if (progress >=3D 32) { > > progress =3D 0; > > #ifdef CONFIG_CONT_PTE_HUGEPAGE > > /* > > * XXX: don't release ptl at an unligned address as > > cont_pte might form while > > * ptl is released, this causes double-map > > */ > > if (!vma_is_chp_anonymous(src_vma) || > > (vma_is_chp_anonymous(src_vma) && IS_ALIGNED(addr, > > HPAGE_CONT_PTE_SIZE))) > > #endif > > if (need_resched() || > > spin_needbreak(src_ptl) || spin_needbreak(dst_ptl)) > > break; > > } > > > > We could only reproduce the above issue by running thousands of phones. > > > > Does your code survive from this problem? > > Yes I'm confident my code is safe against this; as I said before, the CON= T_PTE > bit is not blindly "copied" from parent to child pte. As far as the core-= mm is > concerned, there is no CONT_PTE bit; they are just regular PTEs. So the c= ode > will see some pte_none() entries followed by some pte_present() entries. = And > when calling set_ptes() on the child, the arch code will evaluate the cur= rent > state of the pgtable along with the new set_ptes() request and determine = where > it should insert the CONT_PTE bit. yep, i have read very carefully and think your code is safe here. The only problem is that the code can randomly unfold parent processes' CONPTE while setting wrprotect in the middle of a large folio while it actually should keep CONT bit as all PTEs can be still consistent if we set protect from the 1st PTE. while A forks B, progress >=3D 32 might interrupt in the middle of a new CONTPTE folio which is forming, as we have to set wrprotect to parent A= , this parent immediately loses CONT bit. this is sad. but i can't find a good way to resolve it unless CONT is exposed to mm-core. any idea on this? Our code[1] resolves this by only breaking at the aligned address if (progress >=3D 32) { progress =3D 0; #ifdef CONFIG_CONT_PTE_HUGEPAGE /* * XXX: don't release ptl at an unligned address as cont_pte might form while * ptl is released, this causes double-map */ if (!vma_is_chp_anonymous(src_vma) || (vma_is_chp_anonymous(src_vma) && IS_ALIGNED(addr, HPAGE_CONT_PTE_SIZE))) #endif if (need_resched() || spin_needbreak(src_ptl) || spin_needbreak(dst_ptl)) break; } [1] https://github.com/OnePlusOSS/android_kernel_oneplus_sm8550/blob/oneplu= s/sm8550_u_14.0.0_oneplus11/mm/memory.c#L1180 Thanks Barry