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] [day] [month] [year] [list]

Message-ID: <CAC_iWj+8hYruZce3MLucUiJmcF_0NWQUo7Q+1cqJJPYPjjrEBA@mail.gmail.com>
Date:   Wed, 23 Feb 2022 21:01:21 +0200
From:   Ilias Apalodimas <ilias.apalodimas@...aro.org>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>
Cc:     Joe Damato <jdamato@...tly.com>, netdev@...r.kernel.org,
        kuba@...nel.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

Hi all

[...]
> > 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.
>
>
> > +
> > +#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.
> Thus, I find it unnecessary to do __percpu stats.
>
>
> 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.

Do we really mind though?  The point was to have the percpu variables
in order to plug in any stats that weren't napi-protected.  I think we
can always plug in the recycling stats later?  OTOH if you prefer
having them in now I can work with Joe and we can get that supported
as well.

Cheers
/Ilias
>
> --Jesper
>