Re: [PATCH net-next] eth: fbnic: Add PCIe hardware statistics

Re: [PATCH net-next] eth: fbnic: Add PCIe hardware statistics

[Leon Romanovsky]

On Tue, Nov 05, 2024 at 04:26:25PM -0800, Sanman Pradhan wrote:
> Add PCIe hardware statistics support to the fbnic driver. These stats
> provide insight into PCIe transaction performance and error conditions,
> including, read/write and completion TLP counts and DWORD counts and
> debug counters for tag, completion credit and NP credit exhaustion
> 
> The stats are exposed via ethtool and can be used to monitor PCIe
> performance and debug PCIe issues.

And how does PCIe statistics belong to ethtool?

This PCIe statistics to debug PCIe errors and arguably should be part of
PCI core and not hidden in netdev tool.

Thanks

> 
> Signed-off-by: Sanman Pradhan <sanman.p211993@gmail.com>
> ---
>  .../device_drivers/ethernet/meta/fbnic.rst    |  27 +++++
>  drivers/net/ethernet/meta/fbnic/fbnic.h       |   1 +
>  drivers/net/ethernet/meta/fbnic/fbnic_csr.h   |  39 ++++++
>  .../net/ethernet/meta/fbnic/fbnic_ethtool.c   |  77 +++++++++++-
>  .../net/ethernet/meta/fbnic/fbnic_hw_stats.c  | 114 ++++++++++++++++++
>  .../net/ethernet/meta/fbnic/fbnic_hw_stats.h  |  17 +++
>  .../net/ethernet/meta/fbnic/fbnic_netdev.c    |   3 +
>  drivers/net/ethernet/meta/fbnic/fbnic_pci.c   |   2 +
>  8 files changed, 278 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/networking/device_drivers/ethernet/meta/fbnic.rst b/Documentation/networking/device_drivers/ethernet/meta/fbnic.rst
> index 32ff114f5c26..31c6371c45f8 100644
> --- a/Documentation/networking/device_drivers/ethernet/meta/fbnic.rst
> +++ b/Documentation/networking/device_drivers/ethernet/meta/fbnic.rst
> @@ -27,3 +27,30 @@ driver takes over.
>  devlink dev info provides version information for all three components. In
>  addition to the version the hg commit hash of the build is included as a
>  separate entry.
> +
> +
> +PCIe Statistics
> +---------------
> +
> +The fbnic driver exposes PCIe hardware performance statistics through ethtool.
> +These statistics provide insights into PCIe transaction behavior and potential
> +performance bottlenecks.
> +
> +Statistics Categories
> +
> +1. PCIe Transaction Counters:
> +
> +   These counters track PCIe transaction activity:
> +        - pcie_ob_rd_tlp: Outbound read Transaction Layer Packets count
> +        - pcie_ob_rd_dword: DWORDs transferred in outbound read transactions
> +        - pcie_ob_wr_tlp: Outbound write Transaction Layer Packets count
> +        - pcie_ob_wr_dword: DWORDs transferred in outbound write transactions
> +        - pcie_ob_cpl_tlp: Outbound completion TLP count
> +        - pcie_ob_cpl_dword: DWORDs transferred in outbound completion TLPs
> +
> +2. PCIe Resource Monitoring:
> +
> +   These counters indicate PCIe resource exhaustion events:
> +        - pcie_ob_rd_no_tag: Read requests dropped due to tag unavailability
> +        - pcie_ob_rd_no_cpl_cred: Read requests dropped due to completion credit exhaustion
> +        - pcie_ob_rd_no_np_cred: Read requests dropped due to non-posted credit exhaustion
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
> index fec567c8fe4a..a8fedff48103 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic.h
> @@ -6,6 +6,7 @@
> 
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> +#include <linux/netdevice.h>
>  #include <linux/ptp_clock_kernel.h>
>  #include <linux/types.h>
>  #include <linux/workqueue.h>
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
> index 79cdd231d327..9ee562acbdfc 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
> @@ -882,6 +882,45 @@ enum {
>  #define FBNIC_MAX_QUEUES		128
>  #define FBNIC_CSR_END_QUEUE	(0x40000 + 0x400 * FBNIC_MAX_QUEUES - 1)
> 
> +#define FBNIC_TCE_DROP_CTRL		0x0403d		/* 0x100f0*/
> +
> +/* PUL User Registers*/
> +#define FBNIC_PUL_USER_OB_RD_TLP_CNT_31_0 \
> +					0x3106e		/* 0xc41b8 */
> +#define FBNIC_PUL_USER_OB_RD_DWORD_CNT_31_0 \
> +					0x31070		/* 0xc41c0 */
> +#define FBNIC_PUL_USER_OB_RD_DWORD_CNT_63_32 \
> +					0x31071		/* 0xc41c4 */
> +#define FBNIC_PUL_USER_OB_WR_TLP_CNT_31_0 \
> +					0x31072		/* 0xc41c8 */
> +#define FBNIC_PUL_USER_OB_WR_TLP_CNT_63_32 \
> +					0x31073		/* 0xc41cc */
> +#define FBNIC_PUL_USER_OB_WR_DWORD_CNT_31_0 \
> +					0x31074		/* 0xc41d0 */
> +#define FBNIC_PUL_USER_OB_WR_DWORD_CNT_63_32 \
> +					0x31075		/* 0xc41d4 */
> +#define FBNIC_PUL_USER_OB_CPL_TLP_CNT_31_0 \
> +					0x31076		/* 0xc41d8 */
> +#define FBNIC_PUL_USER_OB_CPL_TLP_CNT_63_32 \
> +					0x31077		/* 0xc41dc */
> +#define FBNIC_PUL_USER_OB_CPL_DWORD_CNT_31_0 \
> +					0x31078		/* 0xc41e0 */
> +#define FBNIC_PUL_USER_OB_CPL_DWORD_CNT_63_32 \
> +					0x31079		/* 0xc41e4 */
> +#define FBNIC_PUL_USER_OB_RD_DBG_CNT_CPL_CRED_31_0 \
> +					0x3107a		/* 0xc41e8 */
> +#define FBNIC_PUL_USER_OB_RD_DBG_CNT_CPL_CRED_63_32 \
> +					0x3107b		/* 0xc41ec */
> +#define FBNIC_PUL_USER_OB_RD_DBG_CNT_TAG_31_0 \
> +					0x3107c		/* 0xc41f0 */
> +#define FBNIC_PUL_USER_OB_RD_DBG_CNT_TAG_63_32 \
> +					0x3107d		/* 0xc41f4 */
> +#define FBNIC_PUL_USER_OB_RD_DBG_CNT_NP_CRED_31_0 \
> +					0x3107e		/* 0xc41f8 */
> +#define FBNIC_PUL_USER_OB_RD_DBG_CNT_NP_CRED_63_32 \
> +					0x3107f		/* 0xc41fc */
> +#define FBNIC_CSR_END_PUL_USER	0x31080	/* CSR section delimiter */
> +
>  /* BAR 4 CSRs */
> 
>  /* The IPC mailbox consists of 32 mailboxes, with each mailbox consisting
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
> index 1117d5a32867..9f590a42a9df 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
> @@ -6,6 +6,39 @@
>  #include "fbnic_netdev.h"
>  #include "fbnic_tlv.h"
> 
> +struct fbnic_stat {
> +	u8 string[ETH_GSTRING_LEN];
> +	unsigned int size;
> +	unsigned int offset;
> +};
> +
> +#define FBNIC_STAT_FIELDS(type, name, stat) { \
> +	.string = name, \
> +	.size = sizeof_field(struct type, stat), \
> +	.offset = offsetof(struct type, stat), \
> +}
> +
> +/* Hardware statistics not captured in rtnl_link_stats */
> +#define FBNIC_HW_STAT(name, stat) \
> +	FBNIC_STAT_FIELDS(fbnic_hw_stats, name, stat)
> +
> +static const struct fbnic_stat fbnic_gstrings_hw_stats[] = {
> +	/* PCIE */
> +	FBNIC_HW_STAT("pcie_ob_rd_tlp", pcie.ob_rd_tlp),
> +	FBNIC_HW_STAT("pcie_ob_rd_dword", pcie.ob_rd_dword),
> +	FBNIC_HW_STAT("pcie_ob_wr_tlp", pcie.ob_wr_tlp),
> +	FBNIC_HW_STAT("pcie_ob_wr_dword", pcie.ob_wr_dword),
> +	FBNIC_HW_STAT("pcie_ob_cpl_tlp", pcie.ob_cpl_tlp),
> +	FBNIC_HW_STAT("pcie_ob_cpl_dword", pcie.ob_cpl_dword),
> +	FBNIC_HW_STAT("pcie_ob_rd_no_tag", pcie.ob_rd_no_tag),
> +	FBNIC_HW_STAT("pcie_ob_rd_no_cpl_cred", pcie.ob_rd_no_cpl_cred),
> +	FBNIC_HW_STAT("pcie_ob_rd_no_np_cred", pcie.ob_rd_no_np_cred),
> +};
> +
> +#define FBNIC_HW_FIXED_STATS_LEN ARRAY_SIZE(fbnic_gstrings_hw_stats)
> +#define FBNIC_HW_STATS_LEN \
> +	(FBNIC_HW_FIXED_STATS_LEN)
> +
>  static int
>  fbnic_get_ts_info(struct net_device *netdev,
>  		  struct kernel_ethtool_ts_info *tsinfo)
> @@ -51,6 +84,43 @@ static void fbnic_set_counter(u64 *stat, struct fbnic_stat_counter *counter)
>  		*stat = counter->value;
>  }
> 
> +static void fbnic_get_strings(struct net_device *dev, u32 sset, u8 *data)
> +{
> +	int i;
> +
> +	switch (sset) {
> +	case ETH_SS_STATS:
> +		for (i = 0; i < FBNIC_HW_STATS_LEN; i++)
> +			ethtool_puts(&data, fbnic_gstrings_hw_stats[i].string);
> +		break;
> +	}
> +}
> +
> +static int fbnic_get_sset_count(struct net_device *dev, int sset)
> +{
> +	switch (sset) {
> +	case ETH_SS_STATS:
> +		return FBNIC_HW_STATS_LEN;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static void fbnic_get_ethtool_stats(struct net_device *dev,
> +				    struct ethtool_stats *stats, u64 *data)
> +{
> +	struct fbnic_net *fbn = netdev_priv(dev);
> +	const struct fbnic_stat *stat;
> +	int i;
> +
> +	fbnic_get_hw_stats(fbn->fbd);
> +
> +	for (i = 0; i < FBNIC_HW_STATS_LEN; i++) {
> +		stat = &fbnic_gstrings_hw_stats[i];
> +		data[i] = *(u64 *)((u8 *)&fbn->fbd->hw_stats + stat->offset);
> +	}
> +}
> +
>  static void
>  fbnic_get_eth_mac_stats(struct net_device *netdev,
>  			struct ethtool_eth_mac_stats *eth_mac_stats)
> @@ -117,10 +187,13 @@ static void fbnic_get_ts_stats(struct net_device *netdev,
>  }
> 
>  static const struct ethtool_ops fbnic_ethtool_ops = {
> -	.get_drvinfo		= fbnic_get_drvinfo,
>  	.get_ts_info		= fbnic_get_ts_info,
> -	.get_ts_stats		= fbnic_get_ts_stats,
> +	.get_drvinfo		= fbnic_get_drvinfo,
> +	.get_strings		= fbnic_get_strings,
> +	.get_sset_count		= fbnic_get_sset_count,
> +	.get_ethtool_stats	= fbnic_get_ethtool_stats,
>  	.get_eth_mac_stats	= fbnic_get_eth_mac_stats,
> +	.get_ts_stats		= fbnic_get_ts_stats,
>  };
> 
>  void fbnic_set_ethtool_ops(struct net_device *dev)
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hw_stats.c b/drivers/net/ethernet/meta/fbnic/fbnic_hw_stats.c
> index a0acc7606aa1..eb19b49fe306 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hw_stats.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hw_stats.c
> @@ -25,3 +25,117 @@ u64 fbnic_stat_rd64(struct fbnic_dev *fbd, u32 reg, u32 offset)
>  	 */
>  	return ((u64)upper << 32);
>  }
> +
> +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);
> +}
> +
> +static void fbnic_hw_stat_rd64(struct fbnic_dev *fbd, u32 reg, s32 offset,
> +			       struct fbnic_stat_counter *stat)
> +{
> +	u64 new_reg_value;
> +
> +	new_reg_value = fbnic_stat_rd64(fbd, reg, offset);
> +	stat->value += new_reg_value - stat->u.old_reg_value_64;
> +	stat->u.old_reg_value_64 = new_reg_value;
> +}
> +
> +static void fbnic_reset_pcie_stats_asic(struct fbnic_dev *fbd,
> +					struct fbnic_pcie_stats *pcie)
> +{
> +	fbnic_hw_stat_rst64(fbd,
> +			    FBNIC_PUL_USER_OB_RD_TLP_CNT_31_0,
> +			    1,
> +			    &pcie->ob_rd_tlp);
> +	fbnic_hw_stat_rst64(fbd,
> +			    FBNIC_PUL_USER_OB_RD_DWORD_CNT_31_0,
> +			    1,
> +			    &pcie->ob_rd_dword);
> +	fbnic_hw_stat_rst64(fbd,
> +			    FBNIC_PUL_USER_OB_CPL_TLP_CNT_31_0,
> +			    1,
> +			    &pcie->ob_cpl_tlp);
> +	fbnic_hw_stat_rst64(fbd,
> +			    FBNIC_PUL_USER_OB_CPL_DWORD_CNT_31_0,
> +			    1,
> +			    &pcie->ob_cpl_dword);
> +	fbnic_hw_stat_rst64(fbd,
> +			    FBNIC_PUL_USER_OB_WR_TLP_CNT_31_0,
> +			    1,
> +			    &pcie->ob_wr_tlp);
> +	fbnic_hw_stat_rst64(fbd,
> +			    FBNIC_PUL_USER_OB_WR_DWORD_CNT_31_0,
> +			    1,
> +			    &pcie->ob_wr_dword);
> +
> +	fbnic_hw_stat_rst64(fbd,
> +			    FBNIC_PUL_USER_OB_RD_DBG_CNT_TAG_31_0,
> +			    1,
> +			    &pcie->ob_rd_no_tag);
> +	fbnic_hw_stat_rst64(fbd,
> +			    FBNIC_PUL_USER_OB_RD_DBG_CNT_CPL_CRED_31_0,
> +			    1,
> +			    &pcie->ob_rd_no_cpl_cred);
> +	fbnic_hw_stat_rst64(fbd,
> +			    FBNIC_PUL_USER_OB_RD_DBG_CNT_NP_CRED_31_0,
> +			    1,
> +			    &pcie->ob_rd_no_np_cred);
> +}
> +
> +static void fbnic_get_pcie_stats_asic64(struct fbnic_dev *fbd,
> +					struct fbnic_pcie_stats *pcie)
> +{
> +	fbnic_hw_stat_rd64(fbd,
> +			   FBNIC_PUL_USER_OB_RD_TLP_CNT_31_0,
> +			   1,
> +			   &pcie->ob_rd_tlp);
> +	fbnic_hw_stat_rd64(fbd,
> +			   FBNIC_PUL_USER_OB_RD_DWORD_CNT_31_0,
> +			   1,
> +			   &pcie->ob_rd_dword);
> +	fbnic_hw_stat_rd64(fbd,
> +			   FBNIC_PUL_USER_OB_WR_TLP_CNT_31_0,
> +			   1,
> +			   &pcie->ob_wr_tlp);
> +	fbnic_hw_stat_rd64(fbd,
> +			   FBNIC_PUL_USER_OB_WR_DWORD_CNT_31_0,
> +			   1,
> +			   &pcie->ob_wr_dword);
> +	fbnic_hw_stat_rd64(fbd,
> +			   FBNIC_PUL_USER_OB_CPL_TLP_CNT_31_0,
> +			   1,
> +			   &pcie->ob_cpl_tlp);
> +	fbnic_hw_stat_rd64(fbd,
> +			   FBNIC_PUL_USER_OB_CPL_DWORD_CNT_31_0,
> +			   1,
> +			   &pcie->ob_cpl_dword);
> +
> +	fbnic_hw_stat_rd64(fbd,
> +			   FBNIC_PUL_USER_OB_RD_DBG_CNT_TAG_31_0,
> +			   1,
> +			   &pcie->ob_rd_no_tag);
> +	fbnic_hw_stat_rd64(fbd,
> +			   FBNIC_PUL_USER_OB_RD_DBG_CNT_CPL_CRED_31_0,
> +			   1,
> +			   &pcie->ob_rd_no_cpl_cred);
> +	fbnic_hw_stat_rd64(fbd,
> +			   FBNIC_PUL_USER_OB_RD_DBG_CNT_NP_CRED_31_0,
> +			   1,
> +			   &pcie->ob_rd_no_np_cred);
> +}
> +
> +void fbnic_reset_hw_stats(struct fbnic_dev *fbd)
> +{
> +	fbnic_reset_pcie_stats_asic(fbd, &fbd->hw_stats.pcie);
> +}
> +
> +void fbnic_get_hw_stats(struct fbnic_dev *fbd)
> +{
> +	fbnic_get_pcie_stats_asic64(fbd, &fbd->hw_stats.pcie);
> +}
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hw_stats.h b/drivers/net/ethernet/meta/fbnic/fbnic_hw_stats.h
> index 30348904b510..0be403ac211b 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hw_stats.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hw_stats.h
> @@ -11,6 +11,21 @@ struct fbnic_stat_counter {
>  	bool reported;
>  };
> 
> +struct fbnic_hw_stat {
> +	struct fbnic_stat_counter frames;
> +	struct fbnic_stat_counter bytes;
> +};
> +
> +struct fbnic_pcie_stats {
> +	struct fbnic_stat_counter ob_rd_tlp, ob_rd_dword;
> +	struct fbnic_stat_counter ob_wr_tlp, ob_wr_dword;
> +	struct fbnic_stat_counter ob_cpl_tlp, ob_cpl_dword;
> +
> +	struct fbnic_stat_counter ob_rd_no_tag;
> +	struct fbnic_stat_counter ob_rd_no_cpl_cred;
> +	struct fbnic_stat_counter ob_rd_no_np_cred;
> +};
> +
>  struct fbnic_eth_mac_stats {
>  	struct fbnic_stat_counter FramesTransmittedOK;
>  	struct fbnic_stat_counter FramesReceivedOK;
> @@ -33,8 +48,10 @@ struct fbnic_mac_stats {
> 
>  struct fbnic_hw_stats {
>  	struct fbnic_mac_stats mac;
> +	struct fbnic_pcie_stats pcie;
>  };
> 
>  u64 fbnic_stat_rd64(struct fbnic_dev *fbd, u32 reg, u32 offset);
> 
> +void fbnic_reset_hw_stats(struct fbnic_dev *fbd);
>  void fbnic_get_hw_stats(struct fbnic_dev *fbd);
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> index c08798fad203..9cb850b78795 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> @@ -627,6 +627,9 @@ struct net_device *fbnic_netdev_alloc(struct fbnic_dev *fbd)
> 
>  	fbnic_reset_queues(fbn, default_queues, default_queues);
> 
> +	/* Capture snapshot of hardware stats so netdev can calculate delta */
> +	fbnic_reset_hw_stats(fbd);
> +
>  	fbnic_reset_indir_tbl(fbn);
>  	fbnic_rss_key_fill(fbn->rss_key);
>  	fbnic_rss_init_en_mask(fbn);
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> index 2de5a6fde7e8..cd1fe1114819 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> @@ -455,6 +455,8 @@ static void __fbnic_pm_attach(struct device *dev)
>  	struct net_device *netdev = fbd->netdev;
>  	struct fbnic_net *fbn;
> 
> +	fbnic_reset_hw_stats(fbd);
> +
>  	if (fbnic_init_failure(fbd))
>  		return;
> 
> --
> 2.43.5
> 

Re: [PATCH net-next] eth: fbnic: Add PCIe hardware statistics

[Bjorn Helgaas]

On Wed, Nov 06, 2024 at 02:22:51PM +0200, Leon Romanovsky wrote:
> On Tue, Nov 05, 2024 at 04:26:25PM -0800, Sanman Pradhan wrote:
> > Add PCIe hardware statistics support to the fbnic driver. These stats
> > provide insight into PCIe transaction performance and error conditions,
> > including, read/write and completion TLP counts and DWORD counts and
> > debug counters for tag, completion credit and NP credit exhaustion
> > 
> > The stats are exposed via ethtool and can be used to monitor PCIe
> > performance and debug PCIe issues.
> 
> And how does PCIe statistics belong to ethtool?
> 
> This PCIe statistics to debug PCIe errors and arguably should be part of
> PCI core and not hidden in netdev tool.

How would this be done in the PCI core?  As far as I can tell, all
these registers are device-specific and live in some device BAR.

Bjorn

Re: [PATCH net-next] eth: fbnic: Add PCIe hardware statistics

[Andrew Lunn]

On Wed, Nov 06, 2024 at 11:12:57AM -0600, Bjorn Helgaas wrote:
> On Wed, Nov 06, 2024 at 02:22:51PM +0200, Leon Romanovsky wrote:
> > On Tue, Nov 05, 2024 at 04:26:25PM -0800, Sanman Pradhan wrote:
> > > Add PCIe hardware statistics support to the fbnic driver. These stats
> > > provide insight into PCIe transaction performance and error conditions,
> > > including, read/write and completion TLP counts and DWORD counts and
> > > debug counters for tag, completion credit and NP credit exhaustion
> > > 
> > > The stats are exposed via ethtool and can be used to monitor PCIe
> > > performance and debug PCIe issues.
> > 
> > And how does PCIe statistics belong to ethtool?
> > 
> > This PCIe statistics to debug PCIe errors and arguably should be part of
> > PCI core and not hidden in netdev tool.
> 
> How would this be done in the PCI core?  As far as I can tell, all
> these registers are device-specific and live in some device BAR.

Is this a licences PCIe core?

Could the same statistics appear in other devices which licence the
same core? Maybe this needs pulling out into a helper? If this is
true, other uses of this core might not be networking hardware, so
ethtool -S would not be the best interfaces. Then they should appear
in debugfs?

	Andrew

Re: [PATCH net-next] eth: fbnic: Add PCIe hardware statistics

[Jakub Kicinski]

On Wed, 6 Nov 2024 18:36:16 +0100 Andrew Lunn wrote:
> > How would this be done in the PCI core?  As far as I can tell, all
> > these registers are device-specific and live in some device BAR.  
> 
> Is this a licences PCIe core?
> 
> Could the same statistics appear in other devices which licence the
> same core? Maybe this needs pulling out into a helper?

The core is licensed but I believe the _USER in the defines names means
the stats sit in the integration logic not the licensed IP. I could be
wrong.

> If this is true, other uses of this core might not be networking
> hardware, so ethtool -S would not be the best interfaces. Then they
> should appear in debugfs?

I tried to push back on adding PCIe config to network tooling,
and nobody listened. Look at all the PCI stuff in devlink params.
Some vendors dump PCIe signal integrity into ethtool -S

Re: [PATCH net-next] eth: fbnic: Add PCIe hardware statistics

[Leon Romanovsky]

On Wed, Nov 06, 2024 at 04:09:58PM -0800, Jakub Kicinski wrote:
> On Wed, 6 Nov 2024 18:36:16 +0100 Andrew Lunn wrote:
> > > How would this be done in the PCI core?  As far as I can tell, all
> > > these registers are device-specific and live in some device BAR.  
> > 
> > Is this a licences PCIe core?
> > 
> > Could the same statistics appear in other devices which licence the
> > same core? Maybe this needs pulling out into a helper?
> 
> The core is licensed but I believe the _USER in the defines names means
> the stats sit in the integration logic not the licensed IP. I could be
> wrong.
> 
> > If this is true, other uses of this core might not be networking
> > hardware, so ethtool -S would not be the best interfaces. Then they
> > should appear in debugfs?
> 
> I tried to push back on adding PCIe config to network tooling,
> and nobody listened. Look at all the PCI stuff in devlink params.
> Some vendors dump PCIe signal integrity into ethtool -S

Can you please give an example? I grepped various keywords and didn't
find anything suspicious.

Thanks

Re: [PATCH net-next] eth: fbnic: Add PCIe hardware statistics

[Vadim Fedorenko]

On 07/11/2024 08:23, Leon Romanovsky wrote:
> On Wed, Nov 06, 2024 at 04:09:58PM -0800, Jakub Kicinski wrote:
>> On Wed, 6 Nov 2024 18:36:16 +0100 Andrew Lunn wrote:
>>>> How would this be done in the PCI core?  As far as I can tell, all
>>>> these registers are device-specific and live in some device BAR.
>>>
>>> Is this a licences PCIe core?
>>>
>>> Could the same statistics appear in other devices which licence the
>>> same core? Maybe this needs pulling out into a helper?
>>
>> The core is licensed but I believe the _USER in the defines names means
>> the stats sit in the integration logic not the licensed IP. I could be
>> wrong.
>>
>>> If this is true, other uses of this core might not be networking
>>> hardware, so ethtool -S would not be the best interfaces. Then they
>>> should appear in debugfs?
>>
>> I tried to push back on adding PCIe config to network tooling,
>> and nobody listened. Look at all the PCI stuff in devlink params.
>> Some vendors dump PCIe signal integrity into ethtool -S
> 
> Can you please give an example? I grepped various keywords and didn't
> find anything suspicious.

Hmm...

[root@host ~]# ethtool -i eth0 | grep driver
driver: mlx5_core
[root@host ~]# ethtool -S eth0 | grep pci
      rx_pci_signal_integrity: 1
      tx_pci_signal_integrity: 1471
      outbound_pci_stalled_rd: 0
      outbound_pci_stalled_wr: 0
      outbound_pci_stalled_rd_events: 0
      outbound_pci_stalled_wr_events: 0

Isn't it a PCIe statistics?

Re: [PATCH net-next] eth: fbnic: Add PCIe hardware statistics

[Leon Romanovsky]

On Thu, Nov 07, 2024 at 11:30:23AM +0000, Vadim Fedorenko wrote:
> On 07/11/2024 08:23, Leon Romanovsky wrote:
> > On Wed, Nov 06, 2024 at 04:09:58PM -0800, Jakub Kicinski wrote:
> > > On Wed, 6 Nov 2024 18:36:16 +0100 Andrew Lunn wrote:
> > > > > How would this be done in the PCI core?  As far as I can tell, all
> > > > > these registers are device-specific and live in some device BAR.
> > > > 
> > > > Is this a licences PCIe core?
> > > > 
> > > > Could the same statistics appear in other devices which licence the
> > > > same core? Maybe this needs pulling out into a helper?
> > > 
> > > The core is licensed but I believe the _USER in the defines names means
> > > the stats sit in the integration logic not the licensed IP. I could be
> > > wrong.
> > > 
> > > > If this is true, other uses of this core might not be networking
> > > > hardware, so ethtool -S would not be the best interfaces. Then they
> > > > should appear in debugfs?
> > > 
> > > I tried to push back on adding PCIe config to network tooling,
> > > and nobody listened. Look at all the PCI stuff in devlink params.
> > > Some vendors dump PCIe signal integrity into ethtool -S
> > 
> > Can you please give an example? I grepped various keywords and didn't
> > find anything suspicious.
> 
> Hmm...

Ohh, I looked in some other place.

> 
> [root@host ~]# ethtool -i eth0 | grep driver
> driver: mlx5_core
> [root@host ~]# ethtool -S eth0 | grep pci
>      rx_pci_signal_integrity: 1
>      tx_pci_signal_integrity: 1471
>      outbound_pci_stalled_rd: 0
>      outbound_pci_stalled_wr: 0
>      outbound_pci_stalled_rd_events: 0
>      outbound_pci_stalled_wr_events: 0
> 
> Isn't it a PCIe statistics?

I didn't do full archaeological research and stopped at 2017 there these
counters were updated to use new API, but it looks like they there from
stone age.

It was a mistake to put it there and they should be moved to PCI core
together with other hundreds debug counters which ConnectX devices have
but don't expose yet.

Thanks

Re: [PATCH net-next] eth: fbnic: Add PCIe hardware statistics

[Jakub Kicinski]

On Thu, 7 Nov 2024 14:03:57 +0200 Leon Romanovsky wrote:
> > [root@host ~]# ethtool -i eth0 | grep driver
> > driver: mlx5_core
> > [root@host ~]# ethtool -S eth0 | grep pci
> >      rx_pci_signal_integrity: 1
> >      tx_pci_signal_integrity: 1471
> >      outbound_pci_stalled_rd: 0
> >      outbound_pci_stalled_wr: 0
> >      outbound_pci_stalled_rd_events: 0
> >      outbound_pci_stalled_wr_events: 0
> > 
> > Isn't it a PCIe statistics?  
> 
> I didn't do full archaeological research and stopped at 2017 there these
> counters were updated to use new API, but it looks like they there from
> stone age.
> 
> It was a mistake to put it there and they should be moved to PCI core
> together with other hundreds debug counters which ConnectX devices have
> but don't expose yet.

Whatever hand-waving you do now, it's impossible to take you seriously
where the device driver of which you are a maintainer does the same
thing. And your direction going forward for PCIe debug, AFAIU, is the
proprietary fwctl stuff. Please stop.

Re: [PATCH net-next] eth: fbnic: Add PCIe hardware statistics

[Leon Romanovsky]

On Thu, Nov 07, 2024 at 07:40:09AM -0800, Jakub Kicinski wrote:
> On Thu, 7 Nov 2024 14:03:57 +0200 Leon Romanovsky wrote:
> > > [root@host ~]# ethtool -i eth0 | grep driver
> > > driver: mlx5_core
> > > [root@host ~]# ethtool -S eth0 | grep pci
> > >      rx_pci_signal_integrity: 1
> > >      tx_pci_signal_integrity: 1471
> > >      outbound_pci_stalled_rd: 0
> > >      outbound_pci_stalled_wr: 0
> > >      outbound_pci_stalled_rd_events: 0
> > >      outbound_pci_stalled_wr_events: 0
> > > 
> > > Isn't it a PCIe statistics?  
> > 
> > I didn't do full archaeological research and stopped at 2017 there these
> > counters were updated to use new API, but it looks like they there from
> > stone age.
> > 
> > It was a mistake to put it there and they should be moved to PCI core
> > together with other hundreds debug counters which ConnectX devices have
> > but don't expose yet.
> 
> Whatever hand-waving you do now, it's impossible to take you seriously
> where the device driver of which you are a maintainer does the same
> thing. 

I said that it is a mistake and can add that we can move it to new infrastructure.

> And your direction going forward for PCIe debug, AFAIU, is the
> proprietary fwctl stuff. Please stop.

Nice, and we are returning back to the discussion of evil vendors vs.
good people who are working in cloud companies which produce hardware
for themselves but don't call themselves vendors.

The latter can do whatever they want, but vendors are doing only crap.

The patch author added these debug counters, and magically it is fine for you:
+   These counters indicate PCIe resource exhaustion events:
+        - pcie_ob_rd_no_tag: Read requests dropped due to tag unavailability
+        - pcie_ob_rd_no_cpl_cred: Read requests dropped due to completion credit exhaustion
+        - pcie_ob_rd_no_np_cred: Read requests dropped due to non-posted credit exhaustion

For example, mlx5 devices and Broadcom have two simple PCIe counters: rx_errors and tx_errors,
which have nothing to do with fwctl.

And the idea, what you can take mistakes from the past, ignore the
feedback and repeat these mistakes, fills me with amazement.

So why don't you allow module parameters? Many drivers have them, but
new are not allowed. If I claim that "vendor XXX has it, can I add it
too?", we all know the answer.

Thanks

Re: [PATCH net-next] eth: fbnic: Add PCIe hardware statistics

[Leon Romanovsky]

On Wed, Nov 06, 2024 at 11:12:57AM -0600, Bjorn Helgaas wrote:
> On Wed, Nov 06, 2024 at 02:22:51PM +0200, Leon Romanovsky wrote:
> > On Tue, Nov 05, 2024 at 04:26:25PM -0800, Sanman Pradhan wrote:
> > > Add PCIe hardware statistics support to the fbnic driver. These stats
> > > provide insight into PCIe transaction performance and error conditions,
> > > including, read/write and completion TLP counts and DWORD counts and
> > > debug counters for tag, completion credit and NP credit exhaustion
> > > 
> > > The stats are exposed via ethtool and can be used to monitor PCIe
> > > performance and debug PCIe issues.
> > 
> > And how does PCIe statistics belong to ethtool?
> > 
> > This PCIe statistics to debug PCIe errors and arguably should be part of
> > PCI core and not hidden in netdev tool.
> 
> How would this be done in the PCI core?  As far as I can tell, all
> these registers are device-specific and live in some device BAR.

I would expect some sysfs file/directory exposed through PCI core.
That sysfs needs to be connected to the relevant device through
callback, like we are doing with .sriov_configure(). So every PCI
device will be able to expose statistics without relation to netdev.

That interface should provide read access and write access with zero
value to reset the counter/counters.

Thanks

> 
> Bjorn

Re: [PATCH net-next] eth: fbnic: Add PCIe hardware statistics

[Bjorn Helgaas]

On Wed, Nov 06, 2024 at 07:50:54PM +0200, Leon Romanovsky wrote:
> On Wed, Nov 06, 2024 at 11:12:57AM -0600, Bjorn Helgaas wrote:
> > On Wed, Nov 06, 2024 at 02:22:51PM +0200, Leon Romanovsky wrote:
> > > On Tue, Nov 05, 2024 at 04:26:25PM -0800, Sanman Pradhan wrote:
> > > > Add PCIe hardware statistics support to the fbnic driver. These stats
> > > > provide insight into PCIe transaction performance and error conditions,
> > > > including, read/write and completion TLP counts and DWORD counts and
> > > > debug counters for tag, completion credit and NP credit exhaustion
> > > > 
> > > > The stats are exposed via ethtool and can be used to monitor PCIe
> > > > performance and debug PCIe issues.
> > > 
> > > And how does PCIe statistics belong to ethtool?
> > > 
> > > This PCIe statistics to debug PCIe errors and arguably should be part of
> > > PCI core and not hidden in netdev tool.
> > 
> > How would this be done in the PCI core?  As far as I can tell, all
> > these registers are device-specific and live in some device BAR.
> 
> I would expect some sysfs file/directory exposed through PCI core.
> That sysfs needs to be connected to the relevant device through
> callback, like we are doing with .sriov_configure(). So every PCI
> device will be able to expose statistics without relation to netdev.
> 
> That interface should provide read access and write access with zero
> value to reset the counter/counters.

Seems plausible.  We do already have something sort of similar with
aer_stats_attrs[].  I don't think there's a way to reset them though,
and they're just all thrown in the top-level device directory, which
probably isn't scalable.

Bjorn

Re: [PATCH net-next] eth: fbnic: Add PCIe hardware statistics

[Leon Romanovsky]

On Wed, Nov 06, 2024 at 03:17:38PM -0600, Bjorn Helgaas wrote:
> On Wed, Nov 06, 2024 at 07:50:54PM +0200, Leon Romanovsky wrote:
> > On Wed, Nov 06, 2024 at 11:12:57AM -0600, Bjorn Helgaas wrote:
> > > On Wed, Nov 06, 2024 at 02:22:51PM +0200, Leon Romanovsky wrote:
> > > > On Tue, Nov 05, 2024 at 04:26:25PM -0800, Sanman Pradhan wrote:
> > > > > Add PCIe hardware statistics support to the fbnic driver. These stats
> > > > > provide insight into PCIe transaction performance and error conditions,
> > > > > including, read/write and completion TLP counts and DWORD counts and
> > > > > debug counters for tag, completion credit and NP credit exhaustion
> > > > > 
> > > > > The stats are exposed via ethtool and can be used to monitor PCIe
> > > > > performance and debug PCIe issues.
> > > > 
> > > > And how does PCIe statistics belong to ethtool?
> > > > 
> > > > This PCIe statistics to debug PCIe errors and arguably should be part of
> > > > PCI core and not hidden in netdev tool.
> > > 
> > > How would this be done in the PCI core?  As far as I can tell, all
> > > these registers are device-specific and live in some device BAR.
> > 
> > I would expect some sysfs file/directory exposed through PCI core.
> > That sysfs needs to be connected to the relevant device through
> > callback, like we are doing with .sriov_configure(). So every PCI
> > device will be able to expose statistics without relation to netdev.
> > 
> > That interface should provide read access and write access with zero
> > value to reset the counter/counters.
> 
> Seems plausible.  We do already have something sort of similar with
> aer_stats_attrs[].  I don't think there's a way to reset them though,

Our HW supports reset of PCIe counters.

> and they're just all thrown in the top-level device directory, which
> probably isn't scalable.

This is why directory is probably the best solution.

Thanks

> 
> Bjorn