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 3913AC25B75 for ; Mon, 3 Jun 2024 20:45:10 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C47F26B0082; Mon, 3 Jun 2024 16:45:09 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BF6C46B008A; Mon, 3 Jun 2024 16:45:09 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id ABEFB6B008C; Mon, 3 Jun 2024 16:45:09 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 8E1246B008A for ; Mon, 3 Jun 2024 16:45:09 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 43C4EA08D9 for ; Mon, 3 Jun 2024 20:45:09 +0000 (UTC) X-FDA: 82190757138.06.BA5F0A4 Received: from mail-ej1-f51.google.com (mail-ej1-f51.google.com [209.85.218.51]) by imf09.hostedemail.com (Postfix) with ESMTP id 69A37140015 for ; Mon, 3 Jun 2024 20:45:07 +0000 (UTC) Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=NO35APl6; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf09.hostedemail.com: domain of shy828301@gmail.com designates 209.85.218.51 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=1717447507; 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=zPraTLrUkjRW4/mcsUdWV0uoJeXl7pZ41wf9X3pKnpI=; b=bQqdjwkszL1qk24Fdc+ZuZN291/sxiWktUome/EPGuG2ISQp/wwOpIo7DRNRae31XSwDft AeQbqt32A0OFbPzVKh/2UxJjqqdYxmK5e+MchTdqqBsV98GlcF+a0kDdec6UzqpmOnZsR3 OrMPRf/w7WDaf3XHquUeyjVHqqDhEBM= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=NO35APl6; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf09.hostedemail.com: domain of shy828301@gmail.com designates 209.85.218.51 as permitted sender) smtp.mailfrom=shy828301@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1717447507; a=rsa-sha256; cv=none; b=Q6qN5Sv9WVf8VOy+cn7PaLsxePNAROQOoSL2JWm59gSUr+7TXXHtmEmcSYtjv1e3Ns4VP1 jTX/K8Pal4OyiQ4Nrgg7qAIFyW5gUE0VRKx8rWCmd4TMFMTuwn7LLNFF5ymBBa2emkywYi HlxR3Rf5ZPvEXrLE3JWHCA1Y26h1mGI= Received: by mail-ej1-f51.google.com with SMTP id a640c23a62f3a-a68a288b8a4so144903966b.2 for ; Mon, 03 Jun 2024 13:45:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1717447506; x=1718052306; 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=zPraTLrUkjRW4/mcsUdWV0uoJeXl7pZ41wf9X3pKnpI=; b=NO35APl6IJUvpCqwEb1EuWj+0EP3ioC59VQhD9f7BoWkTNenVwvfdvrcNx568alZWU dQVbPF+an3VdFHQjpdE4Erp3fOf4qnmCmSNnoPHwhv9h20GX6Y/GTIZM2e1Q7JoDXSU4 ur5i/s54nPnAIhg2HDMtiyGC1xByxtY+wDukWFs8Vom0XIcY+MKz24vkVyE6/jfMJC+S I5i/HUVgsst6HkCiUKTO1EUlMsyMhKYgoLnrNhKVEu98XfRF8K7IPl1kV6B1/5NrBboh s1AogdLTYAG2+nVUMYATETjQL8rbAxrHTR3NvaZWCZ/dbplzOy78JsViCcGOzFuBntKb GyoA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717447506; x=1718052306; 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=zPraTLrUkjRW4/mcsUdWV0uoJeXl7pZ41wf9X3pKnpI=; b=J/URF7g7R0/2pvYQ793gPDuYUji10WwJXbxjJlVKq+I2oEPkU+zKbkz4oRp/Av+iYn TTizI0MwoLeUnt73wmMvgT+LzFhcfIVEhLsaSCW3OYyGq6IRuadkfltqYjkwxGdCxSrR uOVnJBpAuzNlQtBUsenU5EIrDGNRU16PZXNP3aQACr+UoIAZJC7yvXJmtlFZV3xB3GOf /X7LpKBxz2CbHdL3eCEi/ZiU18stq9dllR6XS3UUQ+Zb29xTQSQkAYgLiyuG5NevFKkH gnxHIeBzI7/3x+OUL+cOUVM/HRM01HXkcGyMEKMYwQh24BPspL+7Q7QXsrULN5dYc79s q5nQ== X-Forwarded-Encrypted: i=1; AJvYcCUuVDwIgMSTxDQdVOq2Ku6LS/x9kLJPI8rUEJg6osw1LsCuaNFJEUFpZvQQWI8h2cIBSefXTbh9hFM7W2ZXr9s9U7U= X-Gm-Message-State: AOJu0YxnrU2favdzRe5+1wSMrK0qFXbu3zibjozwgcOyDn9l1lno/IFI YgzsQYWphuB7M9Lh/RkYuNAUOpjRBfr+oRwuXMLOAjF5M5Olb/IypIgJLZeymAONz0ZBJKhgUiQ NMdkPgtoYJWKLPFjABKVz9wY4ISg= X-Google-Smtp-Source: AGHT+IGvv7NpXz0ajPxlgCEgCnse+tn3+KdGCBZ8bhyYUAHK1uGzTPmCHgdTdsdljjKDspcMOYareCoO0h5j0wMylmE= X-Received: by 2002:a50:9509:0:b0:578:69cb:1efc with SMTP id 4fb4d7f45d1cf-57a363479b7mr8687306a12.9.1717447505447; Mon, 03 Jun 2024 13:45:05 -0700 (PDT) MIME-Version: 1.0 References: <202405311534.86cd4043-lkp@intel.com> <890e5a79-8574-4a24-90ab-b9888968d5e5@redhat.com> <0edfcfed-e8c4-4c46-bbce-528c07084792@redhat.com> <8da12503-839d-459f-a2fa-4abd6d21935d@redhat.com> <9d9a5730-161b-4a9d-a696-1cf6d0c5123c@redhat.com> In-Reply-To: <9d9a5730-161b-4a9d-a696-1cf6d0c5123c@redhat.com> From: Yang Shi Date: Mon, 3 Jun 2024 13:44:53 -0700 Message-ID: Subject: Re: [linus:master] [mm] efa7df3e3b: kernel_BUG_at_include/linux/page_ref.h To: David Hildenbrand Cc: Peter Xu , kernel test robot , 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-Queue-Id: 69A37140015 X-Stat-Signature: bdnmk7rpsqwhdabso98megbzgpfxqiqu X-Rspam-User: X-Rspamd-Server: rspam11 X-HE-Tag: 1717447507-54133 X-HE-Meta: U2FsdGVkX1/qti7EA7EZtZj7506n9SpU2q9YzmSo+vQkTyEJTctxoAdLMTZhI0rAvDyFUPBLdksY/FzG4S6y8Nh4p8aC+rmTcOnynRpHrqt4CqpnimuVdKlyJfIWJg1yQlVYfYT0MSP9DTLeQmGCdC34rU3j/TG+TugfgRLR4W4ds9UMl1AcTWUHI/vRQGy1l5OVmMp1JOzS4kmlV0+G9ToYtpUKFpc8UurJwVOt3/G00Qxe1w45gb4R7ngfMP1DQdscoVECspZj9n2nxpz4vj2ByXar8XvaAHAG1nG2Cgoxl5G2l1oW21KcV/Ss29jjDiEM7BXoHIpQvfKQwcW9eJ/ngXgpFfLP7bcZKmPUKuozUqYJGbVS8qwTJ4ceUg1KuuVBMtUNNsvqzJKYXyZ0CtrmXdBrK6yb0Y0tiJhDL7zzypJczUxFzFTWXVsxOyjCIKn3CNV22ssLVO3dqPt3hYBmgbmGSGfno8HZOqXwDZfvM00Ab8l38fbX23/6rS4eqMMcl/WEmZDNCruARJurQ2hkJ/gj0CFC4AQIteyFj7GhkJ6KfVOyccCa087DTnL0ba6VRdLxYTAyhUmJ9aDqOciftTLX8TEOrAMNwLJ7mAKaPNkfJtu9fD9uep52FlSV5qy5dMm5apMJCrRxnrFGz4DcPFzZoweO6dlibD5rkEpJtzQopvnrTGT37S+l2T0qAlxy+/IY0AaixhOTNtyjnw7EYwrIxLGytMjcAgmwZwKKt8BKPrZn/X0faO8AfXOmfd7HIQJ0azcFgToq4YKk+c76Xq4WIsnidHaxI9dXrpmzgkQ6eOemR21SgJYGE/Rs9F7y43h9ekW9gtOf1xC0xDn0TZzonPk2yWttKPRzTCS8Le5rZ5b4j4yuv6qck6MWpvMpOZtR63w27cs0T1ckvoyXdZ/0kDLdOU6fWze4g64tqwmp8BLS4/jFi47nc0/J4hud/hWN9vcl4rdaaMl FOQXf/VE JM4fJRkIB81GN70P5UweQaD23WyMnw8ggyc/QIkFXnlnHLS8XeuLQ2KBilPM5bGfq6IZpe0zJFAYpsWN9rZ+ZqWaPqCCau2xeeIv9p5WaIDsU1AqzphJG9BLHRqcGtZEl/O6QqkaSW9PG5cuD/C1CThhe/U8Cz9imYEL3ufaAwJcSHIUgosngKhP7y0943IU4iHgFwuFafgDphh4es2OuogvzMqtJXLQ3Zb5rFwIybLL9FzDCX61wd8yapkYNIKnVksqyMjUahaJ6STdPJpP8iiQL4rA89ORQSwVxeRRA7HguD6CDLORLwRp1onrXlIqDQ+skFuWYiESXLvzcfteYkjGfhZpivlqL8OJDJv7Biw0pMjzm/t1w5g8jeVcG8J71RDIFVKOnevyzXI6ZaCwVm8ZQbiyCvoNF8A9NgCQEXQx6TMA= 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 Mon, Jun 3, 2024 at 1:38=E2=80=AFPM David Hildenbrand = wrote: > > >> try_get_folio() is all about grabbing a folio that might get freed > >> concurrently. That's why it calls folio_ref_try_add_rcu() and does > >> complicated stuff. > > > > IMHO we can define it.. e.g. try_get_page() wasn't defined as so. > > > > If we want to be crystal clear on that and if we think that's what we w= ant, > > again I would suggest we rename it differently from try_get_page() to a= void > > future misuses, then add documents. We may want to also even assert the > > Yes, absolutely. > > > rcu/irq implications in try_get_folio() at entrance, then that'll be > > detected even without TINY_RCU config. > > > >> > >> On !CONFIG_TINY_RCU, it performs a folio_ref_add_unless(). That's > >> essentially a atomic_add_unless(), which in the worst case ends up bei= ng a > >> cmpxchg loop. > >> > >> > >> Stating that we should be using try_get_folio() in paths where we are = sure > >> the folio refcount is not 0 is the same as using folio_try_get() where > >> folio_get() would be sufficient. > >> > >> The VM_BUG_ON in folio_ref_try_add_rcu() really tells us here that we = are > >> using a function in the wrong context, although in our case, it is saf= e to > >> use (there is now BUG). Which is true, because we know we have a folio > >> reference and can simply use a simple folio_ref_add(). > >> > >> Again, just like we have folio_get() and folio_try_get(), we should > >> distinguish in GUP whether we are adding more reference to a folio (an= d > >> effectively do what folio_get() would), or whether we are actually gra= bbing > >> a folio that could be freed concurrently (what folio_try_get() would d= o). > > > > Yes we can. Again, IMHO it's a matter of whether it will worth it. > > > > Note that even with SMP and even if we keep this code, the > > atomic_add_unless only affects gup slow on THP only, and even with that > > overhead it is much faster than before when that path was introduced.. = and > > per my previous experience we don't care too much there, really. > > > > So it's literally only three paths that are relevant here on the "unles= s" > > overhead: > > > > - gup slow on THP (I just added it; used to be even slower..) > > > > - vivik's new path > > > > - hugepd (which may be gone for good in a few months..) > > > > IMHO none of them has perf concerns. The real perf concern paths is > > gup-fast when pgtable entry existed, but that must use atomic_add_unles= s() > > anyway. Even gup-slow !thp case won't regress as that uses try_get_pag= e(). > > My point is primarily that we should be clear that the one thing is > GUP-fast, and the other is for GUP-slow. > > Sooner or later we'll see more code that uses try_grab_page() to be > converted to folios, and people might naturally use try_grab_folio(), > just like we did with Vivik's code. > > And I don't think we'll want to make GUP-slow in general using > try_grab_folio() in the future. > > So ... > > > > > So again, IMHO the easist way to fix this WARN is we drop the TINY_RCU = bit, > > if nobody worries on UP perf. > > > > I don't have a strong opinion, if any of us really worry about above th= ree > > use cases on "unless" overhead, and think it worthwhile to add the code= to > > support it, I won't object. But to me it's adding pain with no real ben= efit > > we could ever measure, and adding complexity to backport too since we'l= l > > need a fix for as old as 6.6. > > ... for the sake of fixing this WARN, I don't primarily care. Adjusting > the TINY_RCU feels wrong because I suspect somebody had good reasons to > do it like that, and it actually reported something valuable (using the > wrong function for the job). I think this is the major concern about what fix we should do. If that tiny rcu optimization still makes sense and is useful, we'd better keep it. But I can't tell. Leaving it as is may be safer. > > In any case, if we take the easy route to fix the WARN, I'll come back > and clean the functions here up properly. > > -- > Cheers, > > David / dhildenb >