Re: [net-next v8 1/4] page_pool: Add allocation stats

[Saeed Mahameed <saeed@kernel.org>]

On 01 Mar 17:51, Joe Damato wrote:
>On Tue, Mar 1, 2022 at 3:50 PM Saeed Mahameed <saeed@kernel.org> wrote:
>>
>> On 01 Mar 14:10, Joe Damato wrote:
>> >Add per-pool statistics counters for the allocation path of a page pool.
>> >These stats are incremented in softirq context, so no locking or per-cpu
>> >variables are needed.
>> >
>> >This code is disabled by default and a kernel config option is provided for
>> >users who wish to enable them.
>> >
>>
>> Sorry for the late review Joe,
>
>No worries, thanks for taking a look.
>
>> Why disabled by default ? if your benchmarks showed no diff.
>>
>> IMHO If we believe in this, we should have it enabled by default.
>
>I think keeping it disabled by default makes sense for three reasons:
>  - The benchmarks on my hardware don't show a difference, but less
>powerful hardware may be more greatly impacted.
>  - The new code uses more memory when enabled for storing the stats.
>  - These stats are useful for debugging and performance
>investigations, but generally speaking I think the vast majority of
>everyday kernel users won't be looking at this data.
>
>Advanced users who need this information (and are willing to pay the
>cost in memory and potentially CPU) can enable the code relatively
>easily, so I think keeping it defaulted to off makes sense.

I partially agree, since we have other means to detect if page_pool is
effective or not without these stats.

But here is my .02$: the difference in performance when page_pool is
effective and when isn't is huge, these counters are useful on production
systems when the page pool is under stress.

Simply put, the benefits of the page_pool outweigh the overhead of counting
(if even measurable), these counters should've been added long time ago.

if you are aiming for debug only counters then you should've introduced this
feature as a static key (tracepoints) to be enabled on the fly and the
overhead is paid only when enabled for the debug period.

Anyway, not a huge deal :).