linux-kernel - Re: [PATCH RFC PKS/PMEM 22/58] fs/f2fs: Utilize new kmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201012200254.GB20115@casper.infradead.org>
Date:   Mon, 12 Oct 2020 21:02:54 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Ira Weiny <ira.weiny@...el.com>
Cc:     Dave Hansen <dave.hansen@...el.com>,
        Eric Biggers <ebiggers@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>, linux-aio@...ck.org,
        linux-efi@...r.kernel.org, kvm@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-mmc@...r.kernel.org,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        dri-devel@...ts.freedesktop.org, linux-mm@...ck.org,
        target-devel@...r.kernel.org, linux-mtd@...ts.infradead.org,
        linux-kselftest@...r.kernel.org, samba-technical@...ts.samba.org,
        ceph-devel@...r.kernel.org, drbd-dev@...ts.linbit.com,
        devel@...verdev.osuosl.org, linux-cifs@...r.kernel.org,
        linux-nilfs@...r.kernel.org, linux-scsi@...r.kernel.org,
        linux-nvdimm@...ts.01.org, linux-rdma@...r.kernel.org,
        x86@...nel.org, amd-gfx@...ts.freedesktop.org,
        linux-afs@...ts.infradead.org, cluster-devel@...hat.com,
        linux-cachefs@...hat.com, intel-wired-lan@...ts.osuosl.org,
        xen-devel@...ts.xenproject.org, linux-ext4@...r.kernel.org,
        Fenghua Yu <fenghua.yu@...el.com>, ecryptfs@...r.kernel.org,
        linux-um@...ts.infradead.org, intel-gfx@...ts.freedesktop.org,
        linux-erofs@...ts.ozlabs.org, reiserfs-devel@...r.kernel.org,
        linux-block@...r.kernel.org, linux-bcache@...r.kernel.org,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        io-uring@...r.kernel.org, linux-nfs@...r.kernel.org,
        linux-ntfs-dev@...ts.sourceforge.net, netdev@...r.kernel.org,
        kexec@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net,
        linux-fsdevel@...r.kernel.org, bpf@...r.kernel.org,
        linuxppc-dev@...ts.ozlabs.org, linux-btrfs@...r.kernel.org
Subject: Re: [PATCH RFC PKS/PMEM 22/58] fs/f2fs: Utilize new kmap_thread()

On Mon, Oct 12, 2020 at 12:53:54PM -0700, Ira Weiny wrote:
> On Mon, Oct 12, 2020 at 05:44:38PM +0100, Matthew Wilcox wrote:
> > On Mon, Oct 12, 2020 at 09:28:29AM -0700, Dave Hansen wrote:
> > > kmap_atomic() is always preferred over kmap()/kmap_thread().
> > > kmap_atomic() is _much_ more lightweight since its TLB invalidation is
> > > always CPU-local and never broadcast.
> > > 
> > > So, basically, unless you *must* sleep while the mapping is in place,
> > > kmap_atomic() is preferred.
> > 
> > But kmap_atomic() disables preemption, so the _ideal_ interface would map
> > it only locally, then on preemption make it global.  I don't even know
> > if that _can_ be done.  But this email makes it seem like kmap_atomic()
> > has no downsides.
> 
> And that is IIUC what Thomas was trying to solve.
> 
> Also, Linus brought up that kmap_atomic() has quirks in nesting.[1]
> 
> >From what I can see all of these discussions support the need to have something
> between kmap() and kmap_atomic().
> 
> However, the reason behind converting call sites to kmap_thread() are different
> between Thomas' patch set and mine.  Both require more kmap granularity.
> However, they do so with different reasons and underlying implementations but
> with the _same_ resulting semantics; a thread local mapping which is
> preemptable.[2]  Therefore they each focus on changing different call sites.
> 
> While this patch set is huge I think it serves a valuable purpose to identify a
> large number of call sites which are candidates for this new semantic.

Yes, I agree.  My problem with this patch-set is that it ties it to
some Intel feature that almost nobody cares about.  Maybe we should
care about it, but you didn't try very hard to make anyone care about
it in the cover letter.

For a future patch-set, I'd like to see you just introduce the new
API.  Then you can optimise the Intel implementation of it afterwards.
Those patch-sets have entirely different reviewers.