Page MenuHomeFreeBSD

powerpc/pmap64: Make moea64 statistics optional

Authored by jhibbits on Jul 10 2019, 12:00 AM.



It turns out statistics accounting is very expensive in the pmap driver,
and doesn't seem necessary in the common case. Make this optional
behind a MOEA64_STATS #define, which one can set if they really need

This saves ~7-8% on buildworld time on a POWER9.

Found by bdragon.

Diff Detail

rS FreeBSD src repository - subversion
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Looks good.
But I think MOEA64_STATS should become a kernel config option, to make it easier to enable the statistics and to have this option documented.

Can these easily become per CPU counters instead? It seems like that would explain the performance loss, but per cpu should have hardly any overhead.

Yeah, counter(9) seems like the actual solution here.

What do the statistics give us in general for pmap? I don't see statistics on other archs, so I'm not sure what the benefit is. I understood for bringup and early stabilization, but we're in pretty solid territory now.

Not opposed to counter(9), just curious its value now.

That's a fair point. I've used them to assess hash-table spill rates when performance is not great, but not for a long time. We could also just delete them.

I still see merit in keeping statistics available, for assessing performance issues as you mentioned, but I don't see it necessary in the general case. If there are no objections, I'll take @luporl's request and add an option for MOEA64_STATS, possibly enabled in CURRENT, but disabled in STABLE.

Make MOEA64_STATS a compile option

luporl added inline comments.
31 ↗(On Diff #60074)

Wouldn't it be better to include opt_pmap.h at mmu_oea64.h only?

This revision is now accepted and ready to land.Jul 24 2019, 10:54 AM
jhibbits added inline comments.
31 ↗(On Diff #60074)

Generally we want to keep opt_* includes out of header files. But in this case it's a private header that's not installed at all, so I can do that.

This revision was automatically updated to reflect the committed changes.
jhibbits marked an inline comment as done.