On Tue, Jun 16, 2020 at 12:00:37PM -0700, Andrew Morton wrote: > On Mon, 15 Jun 2020 19:19:17 -0400 Peter Xu <[email protected]> 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 <[email protected]> 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
