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 X-Spam-Level: X-Spam-Status: No, score=-3.9 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 265E5C43463 for ; Sun, 20 Sep 2020 17:04:28 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 92E17214F1 for ; Sun, 20 Sep 2020 17:04:27 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=linux-foundation.org header.i=@linux-foundation.org header.b="O7YDY5EZ" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 92E17214F1 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=linux-foundation.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 13259900005; Sun, 20 Sep 2020 13:04:27 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 10A0C900003; Sun, 20 Sep 2020 13:04:27 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F3B0D900005; Sun, 20 Sep 2020 13:04:26 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0097.hostedemail.com [216.40.44.97]) by kanga.kvack.org (Postfix) with ESMTP id D69D5900003 for ; Sun, 20 Sep 2020 13:04:26 -0400 (EDT) Received: from smtpin07.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 9738E181AEF0B for ; Sun, 20 Sep 2020 17:04:26 +0000 (UTC) X-FDA: 77284063332.07.bag95_5001d5e2713e Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin07.hostedemail.com (Postfix) with ESMTP id 7A9371803F9A7 for ; Sun, 20 Sep 2020 17:04:26 +0000 (UTC) X-HE-Tag: bag95_5001d5e2713e X-Filterd-Recvd-Size: 9372 Received: from mail-ed1-f66.google.com (mail-ed1-f66.google.com [209.85.208.66]) by imf40.hostedemail.com (Postfix) with ESMTP for ; Sun, 20 Sep 2020 17:04:25 +0000 (UTC) Received: by mail-ed1-f66.google.com with SMTP id ay8so10586505edb.8 for ; Sun, 20 Sep 2020 10:04:25 -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=Govkq2c7yVsAEyCudSuE8qhnS6+zy4O8TiKkmkv2nIk=; b=O7YDY5EZgHd0ux+0JI623n4+xCNSLKkbNn75HK0PfnOR4wlB9CfHJM9R8MpDNxC4Bt StinnqnJ77DBvptD2dta7OTELHl1G548xZdBUVNIayCFE6KgnVNuKWIqgtvMj+8UMN/e 7pSxwfkCe8prPQ3KSbQLetFJedeblKFxQgFMg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Govkq2c7yVsAEyCudSuE8qhnS6+zy4O8TiKkmkv2nIk=; b=jqnr2279gpG1J4WQ6ljAG2tH8DMVRhSH5TkqaeGDjSZqIjgFME1FABQuKpomJxVtqx GoXx4Jk8wRaSXNJ3gRA38zKrc/FRZjfd0kdT5J9RR5Hg84Se0Xe+QeV9PA83u0TDaBbc 1nC9tnyFOmvKIe7mXOclskntF4BIjxIO3XXaCQJAUhZBOfijNGDezKHr1DGSZrC6/EuI lSU81j+3tdU1NdG/H8weJ5qju19dqrzoE32OaCl3Y5ZsQz/N0biptxgkUIuWSAiM8Ssg pT2BKL1/O5NpSK4zTpfqRL1owF+Rv6yUWRLDyCCsF0BsSzQvtbt8cBrcpJZ9qh//9P/u G7pQ== X-Gm-Message-State: AOAM531/3wttup1rhND9n7uw4UqXGYycaLnzjxgJvqsJuULypzH/gDN3 xEJV0/Y1rTtM8zQxiBOzk5v6vk/HbjBrYQ== X-Google-Smtp-Source: ABdhPJxHcREMZBPBiLFEoyZQwZcbyrg/YuvAd4b4TY/+l7g4WghHVA3X2Lsx+vZ4K+tuN1cKlLG9Lw== X-Received: by 2002:aa7:c885:: with SMTP id p5mr47884269eds.127.1600621464175; Sun, 20 Sep 2020 10:04:24 -0700 (PDT) Received: from mail-wr1-f47.google.com (mail-wr1-f47.google.com. [209.85.221.47]) by smtp.gmail.com with ESMTPSA id a15sm6857511eje.16.2020.09.20.10.04.23 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 20 Sep 2020 10:04:23 -0700 (PDT) Received: by mail-wr1-f47.google.com with SMTP id o5so10341269wrn.13 for ; Sun, 20 Sep 2020 10:04:23 -0700 (PDT) X-Received: by 2002:ac2:5594:: with SMTP id v20mr15170982lfg.344.1600621076988; Sun, 20 Sep 2020 09:57:56 -0700 (PDT) MIME-Version: 1.0 References: <20200919091751.011116649@linutronix.de> <87mu1lc5mp.fsf@nanos.tec.linutronix.de> <87k0wode9a.fsf@nanos.tec.linutronix.de> In-Reply-To: <87k0wode9a.fsf@nanos.tec.linutronix.de> From: Linus Torvalds Date: Sun, 20 Sep 2020 09:57:40 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [patch RFC 00/15] mm/highmem: Provide a preemptible variant of kmap_atomic & friends To: Thomas Gleixner Cc: LKML , linux-arch , Paul McKenney , "the arch/x86 maintainers" , Sebastian Andrzej Siewior , Peter Zijlstra , Juri Lelli , Vincent Guittot , Dietmar Eggemann , Steven Rostedt , Ben Segall , Mel Gorman , Daniel Bristot de Oliveira , Will Deacon , Andrew Morton , Linux-MM , Russell King , Linux ARM , Chris Zankel , Max Filippov , linux-xtensa@linux-xtensa.org, Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , David Airlie , Daniel Vetter , intel-gfx , dri-devel , Ard Biesheuvel , Herbert Xu , Vineet Gupta , "open list:SYNOPSYS ARC ARCHITECTURE" , Arnd Bergmann , Guo Ren , linux-csky@vger.kernel.org, Michal Simek , Thomas Bogendoerfer , linux-mips@vger.kernel.org, Nick Hu , Greentime Hu , Vincent Chen , Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , linuxppc-dev , "David S. Miller" , linux-sparc Content-Type: text/plain; charset="UTF-8" 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 Sun, Sep 20, 2020 at 1:49 AM Thomas Gleixner wrote: > > Actually most usage sites of kmap atomic do not need page faults to be > disabled at all. Right. I think the pagefault disabling has (almost) nothing at all to do with the kmap() itself - it comes from the "atomic" part, not the "kmap" part. I say *almost*, because there is one issue that needs some thought: the amount of kmap nesting. The kmap_atomic() interface - and your local/temporary/whatever versions of it - depends very much inherently on being strictly nesting. In fact, it depends so much on it that maybe that should be part of the new name? It's very wrong to do addr1 = kmap_atomic(); addr2 = kmap_atomic(); ..do something with addr 1.. kunmap_atomic(addr1); .. do something with addr 2.. kunmap_atomic(addr2); because the way we allocate the slots is by using a percpu-atomic inc-return (and we deallocate using dec). So it's fundamentally a stack. And that's perfectly fine for page faults - if they do any kmaps, those will obviously nest. So the only issue with page faults might be that the stack grows _larger_. And that might need some thought. We already make the kmap stack bigger for CONFIG_DEBUG_HIGHMEM, and it's possibly that if we allow page faults we need to make the kmap stack bigger still. Btw, looking at the stack code, Ithink your new implementation of it is a bit scary: static inline int kmap_atomic_idx_push(void) { - int idx = __this_cpu_inc_return(__kmap_atomic_idx) - 1; + int idx = current->kmap_ctrl.idx++; and now that 'current->kmap_ctrl.idx' is not atomic wrt (a) NMI's (this may be ok, maybe we never do kmaps in NMIs, and with nesting I think it's fine anyway - the NMI will undo whatever it did) (b) the prev/next switch And that (b) part worries me. You do the kmap_switch_temporary() to switch the entries, but you do that *separately* from actually switching 'current' to the new value. So kmap_switch_temporary() looks safe, but I don't think it actually is. Because while it first unmaps the old entries and then remaps the new ones, an interrupt can come in, and at that point it matters what is *CURRENT*. And regardless of whether 'current' is 'prev' or 'next', that kmap_switch_temporary() loop may be doing the wrong thing, depending on which one had the deeper stack. The interrupt will be using whatever "current->kmap_ctrl.idx" is, but that might overwrite entries that are in the process of being restored (if current is still 'prev', but kmap_switch_temporary() is in the "restore @next's kmaps" pgase), or it might stomp on entries that have been pte_clear()'ed by the 'prev' thing. I dunno. The latter may be one of those "it works anyway, it overwrites things we don't care about", but the former will most definitely not work. And it will be completely impossible to debug, because it will depend on an interrupt that uses kmap_local/atomic/whatever() coming in _just_ at the right point in the scheduler, and only when the scheduler has been entered with the right number of kmap entries on the prev/next stack. And no developer will ever see this with any amount of debug code enabled, because it will only hit on legacy platforms that do this kmap anyway. So honestly, that code scares me. I think it's buggy. And even if it "happens to work", it does so for all the wrong reasons, and is very fragile. So I would suggest: - continue to use an actual per-cpu kmap_atomic_idx - make the switching code save the old idx, then unmap the old entries one by one (while doing the proper "pop" action), and then map the new entries one by one (while doing the proper "push" action). which would mean that the only index that is actually ever *USED* is the percpu one, and it's always up-to-date and pushed/popped for individual entries, rather than this - imho completely bogus - optimization where you use "p->kmap_ctrl.idx" directly and very very unsafely. Alternatively, that process counter would need about a hundred lines of commentary about exactly why it's safe. Because I don't think it is. Linus