linux-kernel - Re: [PATCH 02/25] mm: Introduce mm_fault

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: <20200617162608.GF76766@xz-x1>
Date:   Wed, 17 Jun 2020 12:26:08 -0400
From:   Peter Xu <peterx@...hat.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Gerald Schaefer <gerald.schaefer@...ibm.com>,
        Andrea Arcangeli <aarcange@...hat.com>
Subject: Re: [PATCH 02/25] mm: Introduce mm_fault_accounting()

On Tue, Jun 16, 2020 at 12:00:37PM -0700, Andrew Morton wrote:
> On Mon, 15 Jun 2020 19:19:17 -0400 Peter Xu <peterx@...hat.com> wrote:
> 
> > On Mon, Jun 15, 2020 at 03:32:40PM -0700, Linus Torvalds wrote:
> > > On Mon, Jun 15, 2020 at 3:16 PM Peter Xu <peterx@...hat.com> wrote:
> > > >
> > > > Provide this helper for doing memory page fault accounting across archs.  It
> > > > can be defined unconditionally because perf_sw_event() is always defined, and
> > > > perf_sw_event() will be a no-op if !CONFIG_PERF_EVENTS.
> > > 
> > > Well, the downside is that now it forces a separate I$ miss and all
> > > those extra arguments because it's a out-of-line function and the
> > > compiler won't see that they all go away.
> > > 
> > > Yeah, maybe some day maybe we'll have LTO and these kinds of things
> > > will not matter. And maybe they already don't. But it seems kind of
> > > sad to basically force non-optimal code generation from this series.
> > 
> > I tried to make it static inline firstly in linux/mm.h, however it'll need to
> > have linux/mm.h include linux/perf_event.h which seems to have created a loop
> > dependency of headers.  I verified current code will at least generate inlined
> > functions too for x86 (no mm_fault_accounting() in "objdump -t vmlinux") with
> > gcc10.
> > 
> > Another alternative is to make it a macro, it's just that I feel the function
> > definition is a bit cleaner.  Any further suggestions welcomed too.
> 
> Could create a new header file mm_fault.h which includes mm.h and
> perf_event.h.  A later cleanup could move other fault-related things
> into that header and add the appropriate inclusions into files which
> use these things.
> 
> btw, I think mm_account_fault() might be a better name for this function.
> 
> And some (kerneldoc) documentation would be nice.  Although this
> function is pretty self-evident.
> 
> > > 
> > > Why would you export the symbol, btw? Page fault handling is never a module.
> > 
> > I followed handle_mm_fault() which is exported too, since potentially
> > mm_fault_accounting() should always be called in the same context of
> > handle_mm_fault().  Or do you prefer me to drop it?
> 
> Let's not add an unneeded export.  If someone for some reason needs it
> later, it can be added then.

I plan to take the approach that Linus suggested, probably with
mm_account_fault() declared as static inline in memory.c.  I'll remember to add
some kerneldoc too.

Thanks!

-- 
Peter Xu