Re: [PATCH 02/25] mm: Introduce mm_fault_accounting()

From: Andrew Morton
Date: Tue Jun 16 2020 - 15:00:45 EST


On Mon, 15 Jun 2020 19:19:17 -0400 Peter Xu <peterx@xxxxxxxxxx> 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@xxxxxxxxxx> 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.