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 C558BC25B75 for ; Fri, 31 May 2024 19:07:09 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 291C26B008A; Fri, 31 May 2024 15:07:09 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 21B3B6B0098; Fri, 31 May 2024 15:07:09 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 06DB46B009A; Fri, 31 May 2024 15:07:09 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id DAA826B008A for ; Fri, 31 May 2024 15:07:08 -0400 (EDT) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 886A28187A for ; Fri, 31 May 2024 19:07:08 +0000 (UTC) X-FDA: 82179623736.14.42578E8 Received: from mail-ed1-f45.google.com (mail-ed1-f45.google.com [209.85.208.45]) by imf13.hostedemail.com (Postfix) with ESMTP id A584320011 for ; Fri, 31 May 2024 19:07:05 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="C7/8NXib"; spf=pass (imf13.hostedemail.com: domain of shy828301@gmail.com designates 209.85.208.45 as permitted sender) smtp.mailfrom=shy828301@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=1717182425; 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=QVtamtoLjBUIgm4i4LenDIZErQ4EHLQZ6pMrvvKRsL0=; b=gJ+2l238HaKHRsKr6H6/7Z53VFSGYGx/Iyi9Y0/WPW9c1jdYk61KqnxE/IJxfo2+6A2bys 8alERjs08B1fx+uCCkaITvMCWni79aqjiPZ+mnEoGITaPlNMdfOvC55LHP1zmcHePhVStA NzAJRywdThr05BXtwuaP6XPpiv5WsEw= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="C7/8NXib"; spf=pass (imf13.hostedemail.com: domain of shy828301@gmail.com designates 209.85.208.45 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1717182425; a=rsa-sha256; cv=none; b=ZKp0gQ3jr1UFXh652ofRcWe7CjvT63YbU1tYTLSyIDd3nJ6PYkt4d1i+KLlN1FCgNO0TWG +DD+TFbsuYglbxLiCuxhXSmfeJ50s3a2HJwxl7R2dL6jkFSI2MkVktFgOsWm7NZHt5ottd QMlM87fDKwJmWmm34cE3WMdqX0Ialy0= Received: by mail-ed1-f45.google.com with SMTP id 4fb4d7f45d1cf-57a20c600a7so2576399a12.3 for ; Fri, 31 May 2024 12:07:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1717182424; x=1717787224; 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=QVtamtoLjBUIgm4i4LenDIZErQ4EHLQZ6pMrvvKRsL0=; b=C7/8NXib/BYt6Nuu5fbHUzR+obnvE1PHCuJqE1M9x0GQiXI0kVw8NB1Hacei9eFmWS nPHs4Vg1BltCPImwbjAGcN6Ml7JWhgpYBlAzZZnu/+vztjCQ4+OuMFi28IpHhm1dnI5R DIX/m50lnkhSwM5+qCQiwktpD5d2nBwyKohwkhv7iNdfh1oSle97l4pHgOjeQIv+FcWB YAF1/TLIRSxtRCZNrUZeNcF/LJBBfmp9sP7KZwiJEFETQ+5xrjHcDlEWw2D65HmpipjD 3SIynGv1GN/hcRYryOO0RwngRxwhsxecgA9xY1oBgMPa5o6q4UnoOTP0oQ5X88tbDqSu S79A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717182424; x=1717787224; 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=QVtamtoLjBUIgm4i4LenDIZErQ4EHLQZ6pMrvvKRsL0=; b=bVVEtk24xuT6VaPKdHQk+EpRnHOTyDrGnor0LKK6LyaB1ai+GTMYVNDzWLwaX7w3ig 2VeJG1lWfvGz8y5Ne8zGmWjuKrts8c6sheqIz7/8VeC89Fiy3UvxfO8GQmLXCl4XxjQv Q2dpRis8fDLa8p6O6TF99k0uQSGYxpnSyx2Ry/y9lom6jjhZ+mWqGgIA69UiDyK5C0Fz 4iixHKluGGyu/edoHy51B/BO94emtEwyANmcUwdElSPOXS17JdHPgD+JZ4UTR0KNZB/Y jXDpnjdVZLcCK7tQzr3iBc+qv1QuHVe3ocmJMELoPE/E1FYNWc4ukYsT6Hcl/AlPSmfw ASIg== X-Forwarded-Encrypted: i=1; AJvYcCXeBE6giy/qnKr9xmnTmbGj+NDLeEZYzcB8soK64cc+ZnEsh/T/TGgQDxtznn8dPKMCFTOaMQmwDcMxOOTHWG9pUc0= X-Gm-Message-State: AOJu0YzrlGUMxlg3TyB9UKdpC1POpufkQHtyJrx+UVF7dRY4ED2E2V4w Zad3/vUfDaGL/exZIEKjSJArl/okdd3RZONE/I7WVFkmZvynqadZy+B7ngTcmT2BziwbH88H0oV cty4WHKH1UkXgDZLRfMqiaOmrrZ0= X-Google-Smtp-Source: AGHT+IHF4MW9s+DpQRuXD1FsKWZU8O0P9SLamvtQYcbpNqjX8gv3aZ/wSJJeGHjiYrcfIJ6biq+kpPmTLemxlxFImsc= X-Received: by 2002:a50:9509:0:b0:57a:3114:7d07 with SMTP id 4fb4d7f45d1cf-57a3638221emr1932877a12.9.1717182423876; Fri, 31 May 2024 12:07:03 -0700 (PDT) MIME-Version: 1.0 References: <202405311534.86cd4043-lkp@intel.com> <890e5a79-8574-4a24-90ab-b9888968d5e5@redhat.com> In-Reply-To: From: Yang Shi Date: Fri, 31 May 2024 12:06:52 -0700 Message-ID: Subject: Re: [linus:master] [mm] efa7df3e3b: kernel_BUG_at_include/linux/page_ref.h To: David Hildenbrand Cc: kernel test robot , Peter Xu , Jason Gunthorpe , Vivek Kasireddy , Rik van Riel , oe-lkp@lists.linux.dev, lkp@intel.com, linux-kernel@vger.kernel.org, Andrew Morton , Matthew Wilcox , Christopher Lameter , linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: A584320011 X-Stat-Signature: n3ahh6rrb8j8iwoeicn7xwrs8k6p3bh9 X-Rspam-User: X-HE-Tag: 1717182425-896704 X-HE-Meta: U2FsdGVkX18+igdzni45ZqV3GohjODi2BgGb2E3tUow+GHx9RSJLM3upqiAXaDiIoghsLugFY62e3hYxb+fjqy3RAOEegvtYfA+Ahepv3YZIRDORNZjgDq1+IedJK5Cm+1wpNtMba3RXFukBbCYWcdRV1+ORtaQ9v8UyjDt8ktS/4IvJl4y3sHLz0JxQFfuhlTZf2dtGJUCq6dritXV14ug+YiDuRtkbKX9agLE8MMNiK6cllaVoGCRUCvAznFwbsb+pEMYHGrQdkBDNWVAdHsxrXZUn33PMWctwEzyLU4o1GQS6UtUSG4/prfXOIP5hRBV740iaIhhAe+gJViQbNngltpl6kyZjOT9MABfztUz1X7mDtRpFRDNkgB0CFsyfkuxYUCKk6ef8ypf/+ghND/Lkd3FKR8GHclwz1jjvOKS6mu45pVovidrVa6+LDDsfUyI9U7EOeh3I9VCm8mhsKRNsAzW/JGLVj09u0xRFdLjXuF+JuCyx6g/Mmguv5+tuRJuuTW8NrwHgyKhqPoqvEL2PAt3WGgoeP6DqzyeM1Dxal3/DXmabOjHY+Yw/jnCgv55rs20bDfVE/15nGP8/zlQBvBmTI1d2qwfr/Yb4gCPVYAnSq8hpHTVm/eFcIzY+cMrvhx1DA/DlPJXzB0Ui22wNj+1cxs6LpGBCxW5s2gLrtCr3seKsVBxdXc9z0gbDDWAtB3Y5HzzUXUpMIrYKKUcmAd1zD4eivbI04ZkZq1SI0CS8O4cT9/8BbFIwsntEBCwXqrbeaHyVKVnCClCvMXEg/UkseHWmMQjo3wWbMOxMQ4xXFo8zd9eDrUkXaD4onyZtAnaIlhOpMUrbipB0atchEFrcdYReOi1UnBOswFac/Ky2xuiQYqtel2SDVkfG8ltB9480tphx6Gqov0YwRNZoHg7NMJiCPU7LzHegwgwdjZQ8oReI9NnFBeTgfUMqxQ+lAFZPW6t3zg9zTQA wEFC935r oq9wZ99Weex1gn/+4E2rSqEM4J+XAB7hsmdwWfrgXLsCsCmHpXPop5bOXnhc9Z8oqf/K0RgRGh5SQ/XaD6i8ZaKdQ1ugOA+nu2Vu8PhTNjVeuzfbP+CnbEHugxFBuXgG8/t7U+SbyEblwAGI4GHD8Ou7NnQ+HrPRMdSMscZQFeYT4R1E6khEigTfUl/08l83WhMr+pRkG9Gu1VCecnxBv1pFr0oc6XQ0+Q+laBzcdEgqhHrj9UDI9jCOjQX913sm65uR7+qrGY0a7ypcmW2fXa4ncd4bmi10jGZ6tzUiNhTFcmkhtiFFY2e66uPOCr5DipwU27qzVkttzPNt85eh6XqqmniLo1M25K8tX2m1GwjYV+FzFF5GBx4IMGsanlVGvs3IzZRE7uRyXocR18YcHvSGjjNieM84PO7Yh1pA3/OWQ+v+0j3M6poMcJ+6dKdSkmii++ESxb6mvZ8L0YiLd5yvgwKZUdnFmI8Ix/cL/q/sRJGvCKyEflDZudOis/N0cpr3JNhv2oW0BUjJG3aZK0ZcVxg== 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 Fri, May 31, 2024 at 11:38=E2=80=AFAM David Hildenbrand wrote: > > On 31.05.24 20:30, Yang Shi wrote: > > On Fri, May 31, 2024 at 11:24=E2=80=AFAM David Hildenbrand wrote: > >> > >> On 31.05.24 20:13, Yang Shi wrote: > >>> On Fri, May 31, 2024 at 11:07=E2=80=AFAM Yang Shi wrote: > >>>> > >>>> On Fri, May 31, 2024 at 10:46=E2=80=AFAM David Hildenbrand wrote: > >>>>> > >>>>> On 31.05.24 18:50, Yang Shi wrote: > >>>>>> On Fri, May 31, 2024 at 1:24=E2=80=AFAM kernel test robot wrote: > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> Hello, > >>>>>>> > >>>>>>> kernel test robot noticed "kernel_BUG_at_include/linux/page_ref.h= " on: > >>>>>>> > >>>>>>> commit: efa7df3e3bb5da8e6abbe37727417f32a37fba47 ("mm: align larg= er anonymous mappings on THP boundaries") > >>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git m= aster > >>>>>>> > >>>>>>> [test failed on linus/master e0cce98fe279b64f4a7d81b7f5c3a23= d80b92fbc] > >>>>>>> [test failed on linux-next/master 6dc544b66971c7f9909ff038b621491= 05272d26a] > >>>>>>> > >>>>>>> in testcase: trinity > >>>>>>> version: trinity-x86_64-6a17c218-1_20240527 > >>>>>>> with following parameters: > >>>>>>> > >>>>>>> runtime: 300s > >>>>>>> group: group-00 > >>>>>>> nr_groups: 5 > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> compiler: gcc-13 > >>>>>>> test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -sm= p 2 -m 16G > >>>>>>> > >>>>>>> (please refer to attached dmesg/kmsg for entire log/backtrace) > >>>>>>> > >>>>>>> > >>>>>>> we noticed the issue does not always happen. 34 times out of 50 r= uns as below. > >>>>>>> the parent is clean. > >>>>>>> > >>>>>>> 1803d0c5ee1a3bbe efa7df3e3bb5da8e6abbe377274 > >>>>>>> ---------------- --------------------------- > >>>>>>> fail:runs %reproduction fail:runs > >>>>>>> | | | > >>>>>>> :50 68% 34:50 dmesg.Kernel_pan= ic-not_syncing:Fatal_exception > >>>>>>> :50 68% 34:50 dmesg.RIP:try_ge= t_folio > >>>>>>> :50 68% 34:50 dmesg.invalid_op= code:#[##] > >>>>>>> :50 68% 34:50 dmesg.kernel_BUG= _at_include/linux/page_ref.h > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> If you fix the issue in a separate patch/commit (i.e. not just a = new version of > >>>>>>> the same patch/commit), kindly add following tags > >>>>>>> | Reported-by: kernel test robot > >>>>>>> | Closes: https://lore.kernel.org/oe-lkp/202405311534.86cd4043-lk= p@intel.com > >>>>>>> > >>>>>>> > >>>>>>> [ 275.267158][ T4335] ------------[ cut here ]------------ > >>>>>>> [ 275.267949][ T4335] kernel BUG at include/linux/page_ref.h:275= ! > >>>>>>> [ 275.268526][ T4335] invalid opcode: 0000 [#1] KASAN PTI > >>>>>>> [ 275.269001][ T4335] CPU: 0 PID: 4335 Comm: trinity-c3 Not tain= ted 6.7.0-rc4-00061-gefa7df3e3bb5 #1 > >>>>>>> [ 275.269787][ T4335] Hardware name: QEMU Standard PC (i440FX + = PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014 > >>>>>>> [ 275.270679][ T4335] RIP: 0010:try_get_folio (include/linux/page= _ref.h:275 (discriminator 3) mm/gup.c:79 (discriminator 3)) > >>>>>>> [ 275.271159][ T4335] Code: c3 cc cc cc cc 44 89 e6 48 89 df e8 e= 4 54 11 00 eb ae 90 0f 0b 90 31 db eb d5 9c 58 0f 1f 40 00 f6 c4 02 0f 84 4= 6 ff ff ff 90 <0f> 0b 48 c7 c6 a0 54 d2 87 48 89 df e8 a9 e9 ff ff 90 0f 0b= be 04 > >>>>>> > >>>>>> If I read this BUG correctly, it is: > >>>>>> > >>>>>> VM_BUG_ON(!in_atomic() && !irqs_disabled()); > >>>>>> > >>>>> > >>>>> Yes, that seems to be the one. > >>>>> > >>>>>> try_grab_folio() actually assumes it is in an atomic context (irq > >>>>>> disabled or preempt disabled) for this call path. This is achieved= by > >>>>>> disabling irq in gup fast or calling it in rcu critical section in > >>>>>> page cache lookup path > >>>>> > >>>>> try_grab_folio()->try_get_folio()->folio_ref_try_add_rcu() > >>>>> > >>>>> Is called (mm-unstable) from: > >>>>> > >>>>> (1) gup_fast function, here IRQs are disable > >>>>> (2) gup_hugepte(), possibly problematic > >>>>> (3) memfd_pin_folios(), possibly problematic > >>>>> (4) __get_user_pages(), likely problematic > >>>>> > >>>>> (1) should be fine. > >>>>> > >>>>> (2) is possibly problematic on the !fast path. If so, due to commit > >>>>> a12083d721d7 ("mm/gup: handle hugepd for follow_page()") ? C= Cing Peter. > >>>>> > >>>>> (3) is possibly wrong. CCing Vivek. > >>>>> > >>>>> (4) is what we hit here > >>>>> > >>>>>> > >>>>>> And try_grab_folio() is used when the folio is a large folio. The > >>>>> > >>>>> > >>>>> We come via process_vm_rw()->pin_user_pages_remote()->__get_user_pa= ges()->try_grab_folio() > >>>>> > >>>>> That code was added in > >>>>> > >>>>> commit 57edfcfd3419b4799353d8cbd6ce49da075cfdbd > >>>>> Author: Peter Xu > >>>>> Date: Wed Jun 28 17:53:07 2023 -0400 > >>>>> > >>>>> mm/gup: accelerate thp gup even for "pages !=3D NULL" > >>>>> > >>>>> The acceleration of THP was done with ctx.page_mask, however= it'll be > >>>>> ignored if **pages is non-NULL. > >>>>> > >>>>> > >>>>> Likely the try_grab_folio() in __get_user_pages() is wrong? > >>>>> > >>>>> As documented, we already hold a refcount. Likely we should better = do a > >>>>> folio_ref_add() and sanity check the refcount. > >>>> > >>>> Yes, a plain folio_ref_add() seems ok for these cases. > >>>> > >>>> In addition, the comment of folio_try_get_rcu() says, which is just = a > >>>> wrapper of folio_ref_try_add_rcu(): > >>>> > >>>> You can also use this function if you're holding a lock that prevent= s > >>>> pages being frozen & removed; eg the i_pages lock for the page cache > >>>> or the mmap_lock or page table lock for page tables. In this case, = it > >>>> will always succeed, and you could have used a plain folio_get(), bu= t > >>>> it's sometimes more convenient to have a common function called from > >>>> both locked and RCU-protected contexts. > >>>> > >>>> So IIUC we can use the plain folio_get() at least for > >>>> process_vm_readv/writev since mmap_lock is held in this path. > >>>> > >>>>> > >>>>> > >>>>> In essence, I think: try_grab_folio() should only be called from GU= P-fast where > >>>>> IRQs are disabled. > >>>> > >>>> Yes, I agree. Just the fast path should need to call try_grab_folio(= ). > >>> > >>> try_grab_folio() also handles FOLL_PIN and FOLL_GET, so we may just > >>> keep calling it and add a flag to try_grab_folio, just like: > >>> > >>> if flag is true > >>> folio_ref_add() > >>> else > >>> try_get_folio() > >> > >> > >> try_grab_page() is what we use on the GUP-slow path. We'd likely want = a > >> folio variant of that. > >> > >> We might want to call that gup_try_grab_folio() and rename the other o= ne > >> to gup_fast_try_grab_folio(). > > > > Won't we duplicate the most code with two versions try_grab_folio()? > > > > I meant something like: > > > > try_grab_folio(struct page *page, int refs, unsigned int flags, bool fa= st) > > { > > if fast > > try_get_folio() > > else > > folio_ref_add() > > } > > > > That's insufficient to handle FOLL_PIN. Likely we should do this: > > diff --git a/mm/gup.c b/mm/gup.c > index 231711efa390d..fea93a64bf235 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -203,8 +203,8 @@ static void gup_put_folio(struct folio *folio, int re= fs, unsigned int flags) > } > > /** > - * try_grab_page() - elevate a page's refcount by a flag-dependent amoun= t > - * @page: pointer to page to be grabbed > + * try_grab_folio() - elevate a folios's refcount by a flag-dependent am= ount > + * @folio: pointer to folio to be grabbed > * @flags: gup flags: these are the FOLL_* flag values. > * > * This might not do anything at all, depending on the flags argument. > @@ -216,16 +216,16 @@ static void gup_put_folio(struct folio *folio, int = refs, unsigned int flags) > * time. Cases: please see the try_grab_folio() documentation, with > * "refs=3D1". > * > + * Must not be called from GUP-fast: the folio must not get freed concur= rently. > + * > * Return: 0 for success, or if no action was required (if neither FOLL= _PIN > * nor FOLL_GET was set, nothing is done). A negative error code for fa= ilure: > * > * -ENOMEM FOLL_GET or FOLL_PIN was set, but the page could= not > * be grabbed. > */ > -int __must_check try_grab_page(struct page *page, unsigned int flags) > +int __must_check try_grab_page(struct folio *folio, unsigned int flags) > { > - struct folio *folio =3D page_folio(page); > - > if (WARN_ON_ONCE(folio_ref_count(folio) <=3D 0)) > return -ENOMEM; > > @@ -239,7 +239,7 @@ int __must_check try_grab_page(struct page *page, uns= igned int flags) > * Don't take a pin on the zero page - it's not going an= ywhere > * and it is used in a *lot* of places. > */ > - if (is_zero_page(page)) > + if (is_zero_folio(folio)) > return 0; > > /* > @@ -260,6 +260,11 @@ int __must_check try_grab_page(struct page *page, un= signed int flags) > return 0; > } > > +int __must_check try_grab_page(struct page *page, unsigned int flags) > +{ > + return gup_try_grab_folio(page_folio(page), flags); > +} > + > /** > * unpin_user_page() - release a dma-pinned page > * @page: pointer to page to be released > > > Then, fix the callers and rename the other one to gup_fast_*. I see your point. Replace try_grab_page() to try_grab_folio() for slow path, it returns 0 or errno, but it should never fail in slow path since we already hold at least one reference IIUC. The fast version should just like old try_grab_folio(), which returns the pointer to folio or NULL. > > > -- > Cheers, > > David / dhildenb >