netdev - Re: [PATCH v3 net-next 1/2] net: 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: <YlQj11JOHOYB+f62@lunn.ch>
Date:   Mon, 11 Apr 2022 14:49:27 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Ilias Apalodimas <ilias.apalodimas@...aro.org>
Cc:     Lorenzo Bianconi <lorenzo@...nel.org>, netdev@...r.kernel.org,
        lorenzo.bianconi@...hat.com, davem@...emloft.net, kuba@...nel.org,
        pabeni@...hat.com, thomas.petazzoni@...tlin.com,
        linux@...linux.org.uk, jbrouer@...hat.com, jdamato@...tly.com
Subject: Re: [PATCH v3 net-next 1/2] net: page_pool: introduce ethtool stats

On Mon, Apr 11, 2022 at 03:34:21PM +0300, Ilias Apalodimas wrote:
> On Mon, 11 Apr 2022 at 15:28, Lorenzo Bianconi <lorenzo@...nel.org> wrote:
> >
> > > Hi Lorenzo,
> >
> > Hi Ilias,
> >
> > >
> > > [...]
> > >
> > > >
> > > >         for_each_possible_cpu(cpu) {
> > > >                 const struct page_pool_recycle_stats *pcpu =
> > > > @@ -66,6 +87,47 @@ bool page_pool_get_stats(struct page_pool *pool,
> > > >         return true;
> > > >  }
> > > >  EXPORT_SYMBOL(page_pool_get_stats);
> > > > +
> > > > +u8 *page_pool_ethtool_stats_get_strings(u8 *data)
> > > > +{
> > > > +       int i;
> > > > +
> > > > +       for (i = 0; i < ARRAY_SIZE(pp_stats); i++) {
> > > > +               memcpy(data, pp_stats[i], ETH_GSTRING_LEN);
> > > > +               data += ETH_GSTRING_LEN;
> > > > +       }
> > > > +
> > > > +       return data;
> > >
> > > Is there a point returning data here or can we make this a void?
> >
> > it is to add the capability to add more strings in the driver code after
> > running page_pool_ethtool_stats_get_strings.
> 
> But the current driver isn't using it.

It could be you need it for the mlx5 driver, which puts the TLS
counters after the page pool counters. Or you could just move them to
the end. I don't think the order of statistics are ABI, just the
strings themselves..

> I don't have too much
> experience with how drivers consume ethtool stats, but would it make
> more sense to return a length instead of a pointer? Maybe Andrew has
> an idea.

Either is acceptable. Even if you do make it a void, the driver can
use the stats_get_count() and do the maths. But a length or a pointer
is simpler.

   Andrew