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 8CC2BC001DC for ; Mon, 17 Jul 2023 20:36:29 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8C1456B0072; Mon, 17 Jul 2023 16:36:28 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8713E8D0001; Mon, 17 Jul 2023 16:36:28 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7388F6B0075; Mon, 17 Jul 2023 16:36:28 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 5ECD76B0072 for ; Mon, 17 Jul 2023 16:36:28 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 235A1120501 for ; Mon, 17 Jul 2023 20:36:28 +0000 (UTC) X-FDA: 81022261656.29.BF9D0F5 Received: from mail-qt1-f178.google.com (mail-qt1-f178.google.com [209.85.160.178]) by imf18.hostedemail.com (Postfix) with ESMTP id 310C71C0008 for ; Mon, 17 Jul 2023 20:36:24 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=iRjdQMzC; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf18.hostedemail.com: domain of yuzhao@google.com designates 209.85.160.178 as permitted sender) smtp.mailfrom=yuzhao@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1689626185; 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=XG3UloC6pPSvkIYL8rMmu+UHUcjJ8ZWmhMAzCruBxHE=; b=00bNJcH15k/z2RD5utt7TVkqMj2J9Wq8cJEiyznaInJC7Pbn8h76aorjF8PjoG/vyf5xia wb0GcALDpa/vxecyJ69gfKAAeywId6oH/2zRV6QcRIY3sS5UQN8SnDRINPxbonTmg/4j2w +VboqFuqhuUW1OygyIKWXS3x+M3xdV0= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=iRjdQMzC; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf18.hostedemail.com: domain of yuzhao@google.com designates 209.85.160.178 as permitted sender) smtp.mailfrom=yuzhao@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1689626185; a=rsa-sha256; cv=none; b=5U8WhZ0g8+Vv8YDx/tSYx1PV+7IBOaEh5Gb31EEmDuK8ix5lAshZVa4pVv8BwLcir7KnvT A8Iqpq4Jn+uaq1/LSxJQC0KdO0IUhQbARPILGapt+/1DHsoYkRuHNJXtNLqs8i1k0dGyO1 Guynf82luc4jDeEMvfN6fBMkBHKQ3JI= Received: by mail-qt1-f178.google.com with SMTP id d75a77b69052e-40371070eb7so16391cf.1 for ; Mon, 17 Jul 2023 13:36:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1689626184; x=1692218184; 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=XG3UloC6pPSvkIYL8rMmu+UHUcjJ8ZWmhMAzCruBxHE=; b=iRjdQMzCwzc8zxaE0VcML8ZQquEPicxVIyConz1X5vBJjg/j+HWQsY18+JfB3kDuL+ t5UnYN0JVwbPAEvQxmPu6bCCDcavRFvllYcEEo0PzEAHIASq0B0UeU89StmjD6IeCi5y 7Nuz3q8fM+mNxNHbHPppbhX5z++/7eUsCrGsoiHtj05zuA1Vray53MUyG0p9Ef78eAPO LRMAwvVIrop1vzNigqNhcoTl2u3/h9DnrzFxoImhhK45lbHz95EMbHb8c5UvqEOBKsn6 3wUaRsEC2VwSo/gq6N0DUwr3X6zxqXIBfsg99qfpbsJiKdSzsVzxsbmWKPm9+S27okgo Db4A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689626184; x=1692218184; 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=XG3UloC6pPSvkIYL8rMmu+UHUcjJ8ZWmhMAzCruBxHE=; b=TuOXptD5dHy/L6PW+NU/DBvN1KoZyP/OxqeA8rXtu3qfYc8dQwQvpynsNVKHO2cnOt 307aFBoYjN9BJEPfsou0HSGYaRhQzjyGSKCKZbyVJmsd51f/z7KC3WXtLuWSjl5mare2 mjA1KCNznFxsfyrSTT6737ZNcdD9pEucejqGKsamhGtK8YqWAIXE0LEpJwqtgFcngUF5 hswggHPOXIdy7ABe2F1YRMWO1grJrK0cMf2VJm0j6b8A+BoifNFUjEDXujZSOOYGYUGu pJ42aHdQfWc6vouepU0r7N73xqTBTmCUlGcuP7ZfX7WB49PSpiQBW22GwdUSOFFWX6Dk sKyg== X-Gm-Message-State: ABy/qLbDaQjaJenZjqplKwjLSTGAeiVygc98yY0oeXh4bPKSXWHp9cfp TY2WA5vQnhDiiByTye9/WMp47z/TeoeedvC1dIu/EA== X-Google-Smtp-Source: APBJJlHoQ1hCZX4jmZcqjxUm4/c4z+1Rlze98VqSxDFJSoPTZ6il6ivy3Eok+/OrcTOc6/5Njnh1y/+9N/I2a0i07hg= X-Received: by 2002:a05:622a:34c:b0:403:ac17:c18a with SMTP id r12-20020a05622a034c00b00403ac17c18amr74135qtw.14.1689626184158; Mon, 17 Jul 2023 13:36:24 -0700 (PDT) MIME-Version: 1.0 References: <20230714160407.4142030-1-ryan.roberts@arm.com> <20230714161733.4144503-3-ryan.roberts@arm.com> <432490d1-8d1e-1742-295a-d6e60a054ab6@arm.com> <5df787a0-8e69-2472-cdd6-f96a3f7dfaaf@arm.com> In-Reply-To: From: Yu Zhao Date: Mon, 17 Jul 2023 14:35:47 -0600 Message-ID: Subject: Re: [PATCH v3 3/4] mm: FLEXIBLE_THP for improved performance To: Ryan Roberts Cc: Hugh Dickins , Matthew Wilcox , Andrew Morton , "Kirill A. Shutemov" , Yin Fengwei , David Hildenbrand , Catalin Marinas , Will Deacon , Anshuman Khandual , Yang Shi , "Huang, Ying" , Zi Yan , Luis Chamberlain , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 310C71C0008 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: u8ffng79ddfbrbmqnam17aftfk17apcf X-HE-Tag: 1689626184-759338 X-HE-Meta: U2FsdGVkX1+TobMDZhHvGxh2uIgt0ouim43SlbIhSI8zPzwqdmSsM5pyYlekPsKu5qxexnBwS5HBGJSNcnjltimYGUMk18BU97SVYUu/K9w1hecH18GG2gewsN6bw9341/0KEGb08BevSjx/+xsdJa6XOuh3xllR5EmYZcLUXNG279npfhsaBvIT8DV2qeBdp427hrQ6Xl/PjoJIj6ZCw0ERa2j8b1ggHgkF7EOxwM00L4IVljCTqnra30ZpbqozL4CIWqKoxNnGZqOmqY+q6wEX4r2QCix4bGV5pLU9pev3k0fatLwgg86wHxREA1SEqQ0Plr06Ua3dsp9gf7JzJxX0x3zyA8Ie0bL8TNAjngHPlGqdIuOxc3aiToFw7rhZ4h7XDpUrYAHanlT+E4XuzdWDEnzlrotYiq5eaPw8WF9vTc+ezES1D239m5mwQbSplbTIgo94up2C1DiRMvBpiMTu7h+95cXLJWjq48xSHYuMmZQz5aAF1bqJxGAaTSPZJYA+bbnh4Zaq5i62jqeZO9jFHaGppFZLtW3ZhoyDetvJDzkwg2k0zNOXw8fJVsc1vdg/d98z8FfMUGYq0c4Qpo66M4jCAOIYNlWHk+dLFx89zzKH+QuC4YpPdwNPsqv/kA1Id54fwQOhD805uVAacfo7v7i8/TBBb3PJ4SkbVzqDLktVihI0YHlyeatLqusx5N7hpG1t+Hl3e5IFfMfzwTkhwP9gjQEgLU0SlIcmvj1BbDdi1el/NNxktixSqqXbfgGHiPev1aPQc5UwFzz1cgEKa/lF0KRQp7Yi2L085ek91ShsGOrwLqAmrABDujJ8DEGNc/k10zo74lJqfIOSighKsrdt0k2aR0uJDZn2qsst2S2zg158/Wknc9jViZeQnoVrZKJpDii3KO2gLiU9gXhGcqN65hW9gR+Z+Ahjc2orKRlUCmMCjdQjmTN1zD8N7gv6YA3e81hmHATS96t DXIMT2CY ASbUqyXMjSicvASXHcze9QHV0ecENJOcBia0g5cc3Zi4h650uO6vahpSxlgCZ+9janUfnTuQJUlCe8LvlGDBRXA4I44XLOGVpgeUGx7oATGRn9EavKZgnXRy/g7nLZZbmLCHklrZ7o/OAHHpXQgAevSd2KFjTNKeEqdnWrUD7ODLuD2kFUQ7eojVOPaMD5qrmdJLUrwPMaBnhg/BHqdBFl+M9ZYOWbdCw0K3akMjHSiAvd81glhmueWWYcorw+r9qADTDxfI6/jYHTFmC0WhZQGiBxncIPdXB+nqMqXcc39JWFOoR1B/es1xrqrzx+TLvgcbOq433/Jh3hPtTf5kWfUQVLtdIOyb67D8nEALFMbE2umg= 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 Mon, Jul 17, 2023 at 1:31=E2=80=AFPM Yu Zhao wrote: > > On Mon, Jul 17, 2023 at 7:36=E2=80=AFAM Ryan Roberts wrote: > > > > >>>> +static int alloc_anon_folio(struct vm_fault *vmf, struct folio **= folio) > > >>>> +{ > > >>>> + int i; > > >>>> + gfp_t gfp; > > >>>> + pte_t *pte; > > >>>> + unsigned long addr; > > >>>> + struct vm_area_struct *vma =3D vmf->vma; > > >>>> + int prefer =3D anon_folio_order(vma); > > >>>> + int orders[] =3D { > > >>>> + prefer, > > >>>> + prefer > PAGE_ALLOC_COSTLY_ORDER ? PAGE_ALLOC_COST= LY_ORDER : 0, > > >>>> + 0, > > >>>> + }; > > >>>> + > > >>>> + *folio =3D NULL; > > >>>> + > > >>>> + if (vmf_orig_pte_uffd_wp(vmf)) > > >>>> + goto fallback; > > >>>> + > > >>>> + for (i =3D 0; orders[i]; i++) { > > >>>> + addr =3D ALIGN_DOWN(vmf->address, PAGE_SIZE << ord= ers[i]); > > >>>> + if (addr >=3D vma->vm_start && > > >>>> + addr + (PAGE_SIZE << orders[i]) <=3D vma->vm_e= nd) > > >>>> + break; > > >>>> + } > > >>>> + > > >>>> + if (!orders[i]) > > >>>> + goto fallback; > > >>>> + > > >>>> + pte =3D pte_offset_map(vmf->pmd, vmf->address & PMD_MASK); > > >>>> + if (!pte) > > >>>> + return -EAGAIN; > > >>> > > >>> It would be a bug if this happens. So probably -EINVAL? > > >> > > >> Not sure what you mean? Hugh Dickins' series that went into v6.5-rc1= makes it > > >> possible for pte_offset_map() to fail (if I understood correctly) an= d we have to > > >> handle this. The intent is that we will return from the fault withou= t making any > > >> change, then we will refault and try again. > > > > > > Thanks for checking that -- it's very relevant. One detail is that > > > that series doesn't affect anon. IOW, collapsing PTEs into a PMD can'= t > > > happen while we are holding mmap_lock for read here, and therefore, > > > the race that could cause pte_offset_map() on shmem/file PTEs to fail > > > doesn't apply here. > > > > But Hugh's patches have changed do_anonymous_page() to handle failure f= rom > > pte_offset_map_lock(). So I was just following that pattern. If this re= ally > > can't happen, then I'd rather WARN/BUG on it, and simplify alloc_anon_f= olio()'s > > prototype to just return a `struct folio *` (and if it's null that mean= s ENOMEM). > > > > Hugh, perhaps you can comment? > > > > As an aside, it was my understanding from LWN, that we are now using a = per-VMA > > lock so presumably we don't hold mmap_lock for read here? Or perhaps th= at only > > applies to file-backed memory? > > For anon under mmap_lock for read: > 1. pte_offset_map[_lock]() fails when a parallel PF changes PMD from > none to leaf. > 2. changing PMD from non-leaf to leaf is a bug. See the comments in > the "else" branch in handle_pte_fault(). > > So for do_anonymous_page(), there is only one case > pte_offset_map[_lock]() can fail. =3D=3D=3D > For the code above, this case was > ruled out by vmf_orig_pte_uffd_wp(). Actually I was wrong about this part. =3D=3D=3D > Checking the return value from pte_offset_map[_lock]() is a good > practice. What I'm saying is that -EAGAIN would mislead people to > think, in our case, !pte is legitimate, and hence the suggestion of > replacing it with -EINVAL. Yes, -EAGAIN is suitable. > No BUG_ON() please. As I've previously mentioned, it's against > Documentation/process/coding-style.rst. > > > > +Hugh Dickins for further consultation if you need it. > > > > > >>>> + > > >>>> + for (; orders[i]; i++) { > > >>>> + addr =3D ALIGN_DOWN(vmf->address, PAGE_SIZE << ord= ers[i]); > > >>>> + vmf->pte =3D pte + pte_index(addr); > > >>>> + if (!vmf_pte_range_changed(vmf, 1 << orders[i])) > > >>>> + break; > > >>>> + } > > >>>> + > > >>>> + vmf->pte =3D NULL; > > >>>> + pte_unmap(pte); > > >>>> + > > >>>> + gfp =3D vma_thp_gfp_mask(vma); > > >>>> + > > >>>> + for (; orders[i]; i++) { > > >>>> + addr =3D ALIGN_DOWN(vmf->address, PAGE_SIZE << ord= ers[i]); > > >>>> + *folio =3D vma_alloc_folio(gfp, orders[i], vma, ad= dr, true); > > >>>> + if (*folio) { > > >>>> + clear_huge_page(&(*folio)->page, addr, 1 <= < orders[i]); > > >>>> + return 0; > > >>>> + } > > >>>> + } > > >>>> + > > >>>> +fallback: > > >>>> + *folio =3D vma_alloc_zeroed_movable_folio(vma, vmf->addres= s); > > >>>> + return *folio ? 0 : -ENOMEM; > > >>>> +} > > >>>> +#else > > >>>> +static inline int alloc_anon_folio(struct vm_fault *vmf, struct f= olio **folio) > > >>> > > >>> Drop "inline" (it doesn't do anything in .c). > > >> > > >> There are 38 instances of inline in memory.c alone, so looks like a = well used > > >> convention, even if the compiler may choose to ignore. Perhaps you c= an educate > > >> me; what's the benefit of dropping it? > > > > > > I'll let Willy and Andrew educate both of us :) > > > > > > +Matthew Wilcox +Andrew Morton please. Thank you. > > > > > >>> The rest looks good to me. > > >> > > >> Great - just incase it wasn't obvious, I decided not to overwrite vm= f->address > > >> with the aligned version, as you suggested > > > > > > Yes, I've noticed. Not overwriting has its own merits for sure. > > > > > >> for 2 reasons; 1) address is const > > >> in the struct, so would have had to change that. 2) there is a uffd = path that > > >> can be taken after the vmf->address fixup would have occured and the= path > > >> consumes that member, so it would have had to be un-fixed-up making = it more > > >> messy than the way I opted for. > > >> > > >> Thanks for the quick review as always!