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

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 1 Mar 2022 20:32:16 -0800
From:   Saeed Mahameed <saeed@...nel.org>
To:     Joe Damato <jdamato@...tly.com>
Cc:     netdev@...r.kernel.org, kuba@...nel.org,
        ilias.apalodimas@...aro.org, davem@...emloft.net, hawk@...nel.org,
        ttoukan.linux@...il.com, brouer@...hat.com, leon@...nel.org,
        linux-rdma@...r.kernel.org, saeedm@...dia.com
Subject: Re: [net-next v8 1/4] page_pool: Add allocation stats

On 01 Mar 17:51, Joe Damato wrote:
>On Tue, Mar 1, 2022 at 3:50 PM Saeed Mahameed <saeed@...nel.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 :).