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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CALALjgzWZLjLj1Qss9JQd3DEh-_SZcwCAEkgAE19Nsxf07EOOQ@mail.gmail.com>
Date:   Tue, 1 Mar 2022 17:51:34 -0800
From:   Joe Damato <jdamato@...tly.com>
To:     Saeed Mahameed <saeed@...nel.org>
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 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.

> >The statistics added are:
> >       - fast: successful fast path allocations
> >       - slow: slow path order-0 allocations
> >       - slow_high_order: slow path high order allocations
> >       - empty: ptr ring is empty, so a slow path allocation was forced.
> >       - refill: an allocation which triggered a refill of the cache
> >       - waive: pages obtained from the ptr ring that cannot be added to
> >         the cache due to a NUMA mismatch.
> >
>
> Let's have this documented under kernel documentation.
> https://docs.kernel.org/networking/page_pool.html
>
> I would also mention the kconfig and any user knobs APIs introduced in
> this series

Sure, I can add a doc commit in the v9 that explains the kernel config
option, the API, and the fields of the stats structures.

Thanks,
Joe