netdev - Re: [PATCH net-next 4/5] eth: fbnic: add PCIe hardware statistics

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: <4a543595-672a-4554-9cef-9a863ed23ca8@lunn.ch>
Date: Mon, 18 Nov 2024 16:40:31 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
	pabeni@...hat.com, alexanderduyck@...com,
	Sanman Pradhan <sanman.p211993@...il.com>
Subject: Re: [PATCH net-next 4/5] eth: fbnic: add PCIe hardware statistics

On Mon, Nov 18, 2024 at 07:35:01AM -0800, Jakub Kicinski wrote:
> On Sun, 17 Nov 2024 21:19:00 +0100 Andrew Lunn wrote:
> > > +/* PUL User Registers*/
> > > +#define FBNIC_PUL_USER_OB_RD_TLP_CNT_31_0 \
> > > +					0x3106e		/* 0xc41b8 */  
> > 
> > Is there a comment somewhere which explains what these comments mean?
> > Otherwise they appear to be useless magic numbers.
> 
> The number on the left is the number on the right divided by 4.
> We index the registers as an array of u32s so the define is an index,
> but for grep-ability we add the absolute address in the comment.

Ah, O.K. Maybe a comment about this would be nice.

> > > +static void fbnic_hw_stat_rst64(struct fbnic_dev *fbd, u32 reg, s32 offset,
> > > +				struct fbnic_stat_counter *stat)
> > > +{
> > > +	/* Record initial counter values and compute deltas from there to ensure
> > > +	 * stats start at 0 after reboot/reset. This avoids exposing absolute
> > > +	 * hardware counter values to userspace.
> > > +	 */
> > > +	stat->u.old_reg_value_64 = fbnic_stat_rd64(fbd, reg, offset);  
> > 
> > I don't know how you use these stats, but now they are in debugfs, do
> > they actually need to be 0 after reboot/reset? For most debugging
> > performance issues with statistics, i want to know how much a counter
> > goes up per second, so userspace will be reading the values twice with
> > a sleep, and doing a subtractions anyway. So why not make the kernel
> > code simpler?
> 
> Right, there is no strong reason to reset debugfs stats. OTOH
> consistent behavior across all stats is nice (from rtnl stats 
> to debugfs). And we will need the reset helpers for other stats.
> Meta uses a lot of multi-host systems, the NIC is reset much
> less often than the machines. Starting at 0 is convenient for 
> manual debug.

If the code is being reused, then O.K.

Reviewed-by: Andrew Lunn <andrew@...n.ch>

    Andrew