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 5AAF7C433F5 for ; Fri, 22 Apr 2022 00:55:38 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B31FA6B0072; Thu, 21 Apr 2022 20:55:37 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B09076B0073; Thu, 21 Apr 2022 20:55:37 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9D0006B0074; Thu, 21 Apr 2022 20:55:37 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.a.hostedemail.com [64.99.140.24]) by kanga.kvack.org (Postfix) with ESMTP id 8A1CA6B0072 for ; Thu, 21 Apr 2022 20:55:37 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 5C91F604B9 for ; Fri, 22 Apr 2022 00:55:37 +0000 (UTC) X-FDA: 79382697114.18.7C3430E Received: from mail-ej1-f49.google.com (mail-ej1-f49.google.com [209.85.218.49]) by imf29.hostedemail.com (Postfix) with ESMTP id B12DC120023 for ; Fri, 22 Apr 2022 00:55:35 +0000 (UTC) Received: by mail-ej1-f49.google.com with SMTP id t11so13234466eju.13 for ; Thu, 21 Apr 2022 17:55:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Cyff/BYI4BHHE42Yn0eFG23oOJzOlK0YmbtegqLYiEA=; b=e8s2jMFqv2bC+sQVxlpzwEDZh7oycaV9S+2wmssqFmvul0CDY0M5+RlJpkTTP7Z9QR qi9yPXjrrlRQyUgqlQaVvL3akjasMHpEFWtfDD/ePQ8hkfHJdbNSK+y2EjwrrzH/Sitb /ZrMeAG6mxUhL9QmT6M7P1gmlMuuvtG6NFBDY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Cyff/BYI4BHHE42Yn0eFG23oOJzOlK0YmbtegqLYiEA=; b=dCYwrTih+gyA1re7NojhnrtAPSZ3HXVKNgBXVd9qSFOFOg+HfByFw8aqhKAiGC67Pe F7TijTXh49RIkno+mRN1oDpR2YNmgDLkrvh0AfIH1wN5Qfo6cP5VYQ+fiviH/EJkq9co TRdgWnsk93xNhDzz8C8kzc3kjnTMqeWa2ptpSaH8r3DQ71EAkcXvOLTCeo8ErDnfV4+C 7PKEox/aGtBld98jffG8tCL0OIDfgBpeKi2ctpl6iqs5KaGFtLiy7ov5K12mTZmfR9HB wGZVRQ32OCNYBypaAuMJljERIBelGnuQH6HiIjJar2nS7ttzk+hRU14W3n19tt2GceLY JA6A== X-Gm-Message-State: AOAM533bmUI0d9osva1BEZi+PrApX8Gr8z/bRile7nVH+9iBUXVMh5zy /FI8eALOVS1RxlORz5I/aHJkcihGf9sza94n+5I= X-Google-Smtp-Source: ABdhPJwpkOZjX4NdrMe+Tt65JI7VORs40FCheEfltOabbVU2zD4LuJNpU0WWO0w97LbBStp42GMPzQ== X-Received: by 2002:a17:906:c149:b0:6df:f047:1698 with SMTP id dp9-20020a170906c14900b006dff0471698mr1887846ejc.16.1650588935093; Thu, 21 Apr 2022 17:55:35 -0700 (PDT) Received: from mail-ej1-f51.google.com (mail-ej1-f51.google.com. [209.85.218.51]) by smtp.gmail.com with ESMTPSA id dd6-20020a1709069b8600b006df08710d00sm200555ejc.85.2022.04.21.17.55.34 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 21 Apr 2022 17:55:34 -0700 (PDT) Received: by mail-ej1-f51.google.com with SMTP id s18so13383587ejr.0 for ; Thu, 21 Apr 2022 17:55:34 -0700 (PDT) X-Received: by 2002:a2e:9d46:0:b0:24c:7f1d:73cc with SMTP id y6-20020a2e9d46000000b0024c7f1d73ccmr1288388ljj.358.1650588589383; Thu, 21 Apr 2022 17:49:49 -0700 (PDT) MIME-Version: 1.0 References: <20220415164413.2727220-1-song@kernel.org> <4AD023F9-FBCE-4C7C-A049-9292491408AA@fb.com> <88eafc9220d134d72db9eb381114432e71903022.camel@intel.com> <1650511496.iys9nxdueb.astroid@bobo.none> <1650530694.evuxjgtju7.astroid@bobo.none> <1650582120.hf4z0mkw8v.astroid@bobo.none> In-Reply-To: <1650582120.hf4z0mkw8v.astroid@bobo.none> From: Linus Torvalds Date: Thu, 21 Apr 2022 17:49:32 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v4 bpf 0/4] vmalloc: bpf: introduce VM_ALLOW_HUGE_VMAP To: Nicholas Piggin Cc: "akpm@linux-foundation.org" , "ast@kernel.org" , "bp@alien8.de" , "bpf@vger.kernel.org" , "daniel@iogearbox.net" , "dborkman@redhat.com" , "edumazet@google.com" , "hch@infradead.org" , "hpa@zytor.com" , "imbrenda@linux.ibm.com" , Kernel Team , "linux-kernel@vger.kernel.org" , "linux-mm@kvack.org" , "mbenes@suse.cz" , "mcgrof@kernel.org" , "pmladek@suse.com" , "Edgecombe, Rick P" , Mike Rapoport , "song@kernel.org" , Song Liu Content-Type: text/plain; charset="UTF-8" X-Rspam-User: X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: B12DC120023 X-Stat-Signature: fz6gqdc44q9xjhqxjxp6fmxrbnt7zi4r Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=linux-foundation.org header.s=google header.b=e8s2jMFq; dmarc=none; spf=pass (imf29.hostedemail.com: domain of torvalds@linuxfoundation.org designates 209.85.218.49 as permitted sender) smtp.mailfrom=torvalds@linuxfoundation.org X-HE-Tag: 1650588935-972608 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, Apr 21, 2022 at 4:30 PM Nicholas Piggin wrote: > > VM_FLUSH_RESET_PERMS was because bpf uses the arch module allocation > code which was not capable of dealing with huge pages in the arch > specific direct map manipulation stuff was unable to deal with it. > An x86 bug. .. and a power one? The only thing really special in __module_alloc() on power is that same VM_FLUSH_RESET_PERMS. Why had you otherwise disabled it there on powerpc too? > > And that bug was an issue on power too. > > I missed it, which bug was that? See above. At least it's very strongly implied by how the powerpc __module_alloc() function also used VM_FLUSH_RESET_PERMS | VM_NO_HUGE_VMAP, because there isn't much else going on. > No I don't notice. More work to support huge allocations for > executable mappings, sure. But the arch's implementation explicitly > does not support that yet. That doesn't make huge vmalloc broken! > Ridiculous. It works fine. There are several other reports of problems that weren't related to permissions (at least not obviously so). You were pointed at one of them in this thread: https://lore.kernel.org/all/14444103-d51b-0fb3-ee63-c3f182f0b546@molgen.mpg.de/ and yes, it also happened on x86-64, but my point this whole time has been that x86-64 gets A *LOT* MORE TEST COVERAGE. See the difference? The fact that it has workedc for you on powerpc doesn't mean that it's fine on powerpc. It only means that powerpc gets about one thousandth of the test coverage that x86-64 gets. > You did just effectively disable it on x86 though. I disabled it *EVERYWHERE*. What is so hard to understand about that? Why are you so convinced this is about x86? It's not. > There really aren't all these "issues" you're imagining. They > aren't noticable now, on power or s390, because they have > non-buggy HAVE_ARCH_HUGE_VMALLOC implementations. So I really think you've not tested it. How many of those powerpc or s390 machines do you think test drivers that do vmalloc_to_page() and then do something with that 'struct page *'? Seriously. Why are you so convinced that "oh, any vmalloc() can be converted to large pages"? I really think the only reason you think that is because it ran on machines that basically have almost no drivers in use, and are all very homogenous, and just didn't happen to hit the bugs. IOW, I think you are probably entirely right that x86 has its own set of excitement (the bpf thread has this thing about how x86 does RO differently from other architectures), and there are likely x86 specific bugs *TOO*. But let's just pick a random driver that uses vmalloc (literally random - I just grepped for it and started looking at it): drivers/infiniband/hw/qib/qib_file_ops.c and it unquestionably does absolutely disgusting things, and if looking at the code makes you go "nobody should do that", then I won't disagree all that much. But as an example of what it does, it basically does things like this: rcd->subctxt_uregbase = vmalloc_user(...); and then you can mmap it in user space in mmap_kvaddr(): addr = rcd->subctxt_uregbase; size = PAGE_SIZE * subctxt_cnt; ... vma->vm_pgoff = (unsigned long) addr >> PAGE_SHIFT; vma->vm_ops = &qib_file_vm_ops; and then the page fault routine is: static const struct vm_operations_struct qib_file_vm_ops = { .fault = qib_file_vma_fault, }; and that function qib_file_vma_fault() does things like this: page = vmalloc_to_page((void *)(vmf->pgoff << PAGE_SHIFT)); if (!page) return VM_FAULT_SIGBUS; get_page(page); vmf->page = page; return 0; and let me just claim (a) I bet you have never EVER tested this kind of insane code on powerpc (b) do you think this will work great if vmalloc() allocates large pages? Can you now see what I'm saying? Can you now admit that the whole "nmake vmalloc() do large pages without explicit opt-in" was a HORRIBLE MISTAKE. > If you're really going to insist on this will you apply this to fix > (some of) the performance regressions it introduced? No. That patch is disgusting and there is no excuse for applying something crap like that. What I applied was the first in a series of patches that do it sanely. That whole "a sane way forward" thing. See https://lore.kernel.org/all/20220415164413.2727220-3-song@kernel.org/ for [PATCH 2/4] in the series for this particular issue. But I'm not applying anything else than the "disable this mess" before we have more discussion and consensus. And dammit, you had better just admit that this wasn't some x86-only thing. Powerpc and s390 were BROKEN GARBAGE AND JUST DIDN'T HAVE THE TEST COVERAGE. Seriously. And the thing is, your opt-out approach was just POINTLESS. The actual cases that are performance-critical are likely in the single digits. It's probably just that big-hash case and maybe a *couple* of other cases (ie the bpf jit really wants to use it). So opt-in is clearly the correct thing to do. Do you now understand why I applied that "users must opt-in" patch? Do you now understand that this is not some "x86" thing? Linus