netdev - Re: [PATCH net-next] net: mvneta: add support for page_pool_get

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: <Yk2dhD2rjQQaF4Pc@lore-desk>
Date:   Wed, 6 Apr 2022 16:02:44 +0200
From:   Lorenzo Bianconi <lorenzo@...nel.org>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     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, ilias.apalodimas@...aro.org, jdamato@...tly.com
Subject: Re: [PATCH net-next] net: mvneta: add support for page_pool_get_stats

> > +static void mvneta_ethtool_update_pp_stats(struct mvneta_port *pp,
> > +					   struct page_pool_stats *stats)
> > +{
> > +	int i;
> > +
> > +	memset(stats, 0, sizeof(*stats));
> > +	for (i = 0; i < rxq_number; i++) {
> > +		struct page_pool *page_pool = pp->rxqs[i].page_pool;
> > +		struct page_pool_stats pp_stats = {};
> > +
> > +		if (!page_pool_get_stats(page_pool, &pp_stats))
> > +			continue;
> > +
> > +		stats->alloc_stats.fast += pp_stats.alloc_stats.fast;
> > +		stats->alloc_stats.slow += pp_stats.alloc_stats.slow;
> > +		stats->alloc_stats.slow_high_order +=
> > +			pp_stats.alloc_stats.slow_high_order;
> > +		stats->alloc_stats.empty += pp_stats.alloc_stats.empty;
> > +		stats->alloc_stats.refill += pp_stats.alloc_stats.refill;
> > +		stats->alloc_stats.waive += pp_stats.alloc_stats.waive;
> > +		stats->recycle_stats.cached += pp_stats.recycle_stats.cached;
> > +		stats->recycle_stats.cache_full +=
> > +			pp_stats.recycle_stats.cache_full;
> > +		stats->recycle_stats.ring += pp_stats.recycle_stats.ring;
> > +		stats->recycle_stats.ring_full +=
> > +			pp_stats.recycle_stats.ring_full;
> > +		stats->recycle_stats.released_refcnt +=
> > +			pp_stats.recycle_stats.released_refcnt;
> 
> Am i right in saying, these are all software stats? They are also
> generic for any receive queue using the page pool?

yes, these stats are accounted by the kernel so they are sw stats, but I guess
xdp ones are sw as well, right?

> 
> It seems odd the driver is doing the addition here. Why not pass stats
> into page_pool_get_stats()? That will make it easier when you add
> additional statistics?
> 
> I'm also wondering if ethtool -S is even the correct API. It should be
> for hardware dependent statistics, those which change between
> implementations. Where as these statistics should be generic. Maybe
> they should be in /sys/class/net/ethX/statistics/ and the driver
> itself is not even involved, the page pool code implements it?

I do not have a strong opinion on it, but I can see an issue for some drivers
(e.g. mvpp2 iirc) where page_pools are not specific for each net_device but are shared
between multiple ports, so maybe it is better to allow the driver to decide how
to report them. What do you think?

Regards,
Lorenzo

> 
>        Andrew

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)