netdev - Re: [net-next v6 1/2] page_pool: Add page

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: <CALALjgwqLhTe8zFPugbW7XcMqnhRTKevv-zuVY+CWOjSYTLQRQ@mail.gmail.com>
Date:   Wed, 23 Feb 2022 09:05:06 -0800
From:   Joe Damato <jdamato@...tly.com>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>
Cc:     netdev@...r.kernel.org, kuba@...nel.org,
        ilias.apalodimas@...aro.org, davem@...emloft.net, hawk@...nel.org,
        saeed@...nel.org, ttoukan.linux@...il.com, brouer@...hat.com
Subject: Re: [net-next v6 1/2] page_pool: Add page_pool stats

On Wed, Feb 23, 2022 at 8:06 AM Jesper Dangaard Brouer
<jbrouer@...hat.com> wrote:
>
>
>
> On 23/02/2022 01.00, Joe Damato wrote:
> > Add per-cpu per-pool statistics counters for the allocation path of a page
> > pool.
> >
> > This code is disabled by default and a kernel config option is provided for
> > users who wish to enable them.
> >
> > 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.
> >
> > Signed-off-by: Joe Damato <jdamato@...tly.com>
> > ---
> >   include/net/page_pool.h | 18 ++++++++++++++++++
> >   net/Kconfig             | 13 +++++++++++++
> >   net/core/page_pool.c    | 37 +++++++++++++++++++++++++++++++++----
> >   3 files changed, 64 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> > index 97c3c19..bedc82f 100644
> > --- a/include/net/page_pool.h
> > +++ b/include/net/page_pool.h
> > @@ -135,7 +135,25 @@ struct page_pool {
> >       refcount_t user_cnt;
> >
> >       u64 destroy_cnt;
> > +#ifdef CONFIG_PAGE_POOL_STATS
> > +     struct page_pool_stats __percpu *stats ____cacheline_aligned_in_smp;
> > +#endif
> > +};
>
> Adding this to the end of the struct and using attribute
> ____cacheline_aligned_in_smp cause the structure have a lot of wasted
> padding in the end.
>
> I recommend using the tool pahole to see the struct layout.

I mentioned placement near xdp_mem_id in the cover letter for this
code and in my reply on the v5 [1] , but I didn't hear back, so I
really had no idea what you preferred.

I'll move it near xdp_mem_id for the v7, and hope that's what you are
saying here.

> > +
> > +#ifdef CONFIG_PAGE_POOL_STATS
> > +struct page_pool_stats {
> > +     struct {
> > +             u64 fast; /* fast path allocations */
> > +             u64 slow; /* slow-path order 0 allocations */
> > +             u64 slow_high_order; /* slow-path high order allocations */
> > +             u64 empty; /* failed refills due to empty ptr ring, forcing
> > +                         * slow path allocation
> > +                         */
> > +             u64 refill; /* allocations via successful refill */
> > +             u64 waive;  /* failed refills due to numa zone mismatch */
> > +     } alloc;
> >   };
> > +#endif
>
> All of these stats are for page_pool allocation "RX" side, which is
> protected by softirq/NAPI.

Yes.

> Thus, I find it unnecessary to do __percpu stats.

Unnecessary, sure, but doesn't seem harmful and it allows us to expand
this to other stats in a future change.

> As Ilias have pointed out-before, the __percpu stats (first) becomes
> relevant once we want stats for the free/"return" path ... which is not
> part of this patchset.

Does that need to be covered in this patchset?

I believe Ilias' response which mentioned the recycle stats indicates
that they can be added in the future [2]. I took this to mean a
separate patchset, assuming that this first change is accepted.

As I understand your comment, then: this change will not be accepted
unless it is expanded to include recycle stats or if the per-cpu
designation is removed.

Are the cache-line placement and per-cpu designations the only
remaining issues with this change?

[1]: https://lore.kernel.org/all/CALALjgzUQUuEVkNXous0kOcHHqiSrTem+n9MjQh6q-8+Azi-sg@mail.gmail.com/
[2]: https://lore.kernel.org/all/YfJhIpBGW6suBwkY@hades/