| Message ID | 20241106064015.4118-1-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/mlx5e: Report rx_discards_phy via rx_missed_errors | expand |
On 06/11/2024 8:40, Yafang Shao wrote: > We observed a high number of rx_discards_phy events on some servers when > running `ethtool -S`. However, this important counter is not currently > reflected in the /proc/net/dev statistics file, making it challenging to > monitor effectively. > > Since rx_missed_errors represents packets dropped due to buffer exhaustion, > it makes sense to include rx_discards_phy in this counter to enhance > monitoring visibility. This change will help administrators track these > events more effectively through standard interfaces. > Hi, Thanks for your patch. It's a matter of interpretation... The documentation in Documentation/ABI/testing/sysfs-class-net-statistics refers to the driver for the exact meaning. rx_discards_phy counts packet drops due to exhaustion of the physical port memory (not in the host), this happen way before steering the packet to any receive queue. Today, rx_missed_errors counts SW/host memory buffer exhaustion of the receive queues. I don't think that rx_missed_errors should mix both. Maybe some other counter can be used for rx_discards_phy, like rx_fifo_errors? > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > Cc: Saeed Mahameed <saeedm@nvidia.com> > Cc: Tariq Toukan <tariqt@nvidia.com> > Cc: Leon Romanovsky <leon@kernel.org> > --- > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > index 6f686fabed44..42c1b791a74c 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > @@ -3903,7 +3903,8 @@ mlx5e_get_stats(struct net_device *dev, struct rtnl_link_stats64 *stats) > mlx5e_fold_sw_stats64(priv, stats); > } > > - stats->rx_missed_errors = priv->stats.qcnt.rx_out_of_buffer; > + stats->rx_missed_errors = priv->stats.qcnt.rx_out_of_buffer + > + PPORT_2863_GET(pstats, if_in_discards); > > stats->rx_length_errors = > PPORT_802_3_GET(pstats, a_in_range_length_errors) +
On Wed, Nov 6, 2024 at 5:56 PM Tariq Toukan <ttoukan.linux@gmail.com> wrote: > > > > On 06/11/2024 8:40, Yafang Shao wrote: > > We observed a high number of rx_discards_phy events on some servers when > > running `ethtool -S`. However, this important counter is not currently > > reflected in the /proc/net/dev statistics file, making it challenging to > > monitor effectively. > > > > Since rx_missed_errors represents packets dropped due to buffer exhaustion, > > it makes sense to include rx_discards_phy in this counter to enhance > > monitoring visibility. This change will help administrators track these > > events more effectively through standard interfaces. > > > > Hi, > > Thanks for your patch. > > It's a matter of interpretation... > The documentation in > Documentation/ABI/testing/sysfs-class-net-statistics refers to the > driver for the exact meaning. > > rx_discards_phy counts packet drops due to exhaustion of the physical > port memory (not in the host), this happen way before steering the > packet to any receive queue. > Today, rx_missed_errors counts SW/host memory buffer exhaustion of the > receive queues. > I don't think that rx_missed_errors should mix both. Thanks for your detailed explanation. > > Maybe some other counter can be used for rx_discards_phy, like > rx_fifo_errors? It appears that rx_fifo_errors is a more appropriate counter for this purpose. I will submit a v2. Thanks for your suggestion.
On 06/11/2024 13:49, Yafang Shao wrote: > On Wed, Nov 6, 2024 at 5:56 PM Tariq Toukan <ttoukan.linux@gmail.com> wrote: >> >> >> >> On 06/11/2024 8:40, Yafang Shao wrote: >>> We observed a high number of rx_discards_phy events on some servers when >>> running `ethtool -S`. However, this important counter is not currently >>> reflected in the /proc/net/dev statistics file, making it challenging to >>> monitor effectively. >>> >>> Since rx_missed_errors represents packets dropped due to buffer exhaustion, >>> it makes sense to include rx_discards_phy in this counter to enhance >>> monitoring visibility. This change will help administrators track these >>> events more effectively through standard interfaces. >>> >> >> Hi, >> >> Thanks for your patch. >> >> It's a matter of interpretation... >> The documentation in >> Documentation/ABI/testing/sysfs-class-net-statistics refers to the >> driver for the exact meaning. I think this documentation is outdated, a more recent one is in if_link.h: * @rx_missed_errors: Count of packets missed by the host. * Folded into the "drop" counter in `/proc/net/dev`. * * Counts number of packets dropped by the device due to lack * of buffer space. This usually indicates that the host interface * is slower than the network interface, or host is not keeping up * with the receive packet rate. * * This statistic corresponds to hardware events and is not used * on software devices. >> >> rx_discards_phy counts packet drops due to exhaustion of the physical >> port memory (not in the host), this happen way before steering the >> packet to any receive queue. >> Today, rx_missed_errors counts SW/host memory buffer exhaustion of the >> receive queues. >> I don't think that rx_missed_errors should mix both. > > Thanks for your detailed explanation. > >> >> Maybe some other counter can be used for rx_discards_phy, like >> rx_fifo_errors? > > It appears that rx_fifo_errors is a more appropriate counter for this purpose. > I will submit a v2. Thanks for your suggestion. Probably not a good idea: * This statistics was used interchangeably with @rx_over_errors. * Not recommended for use in drivers for high speed interfaces.
On Wed, 6 Nov 2024 21:23:47 +0200 Gal Pressman wrote: > > It appears that rx_fifo_errors is a more appropriate counter for this purpose. > > I will submit a v2. Thanks for your suggestion. > > Probably not a good idea: > * This statistics was used interchangeably with @rx_over_errors. > * Not recommended for use in drivers for high speed interfaces. FWIW we can change the definition. Let me copy paste below the commit which added the docs because it has the background. tl;dr is that I was trying to push drivers towards a single stat to keep things simple. If we have a clear definition of how rx_fifo_errors would differ - we can reuse it and update the doc. For example if rx_discards_phy usually means that the adapter itself is overwhelmed (too many rules etc) that would be a pretty clear, since rx_missed is supposed to primarily indicate that the host rings are full or perhaps the PCIe interface of the NIC is struggling. But not the packet processing. commit 0db0c34cfbc9838c1a14cb04dd880602abd699a7 Author: Jakub Kicinski <kuba@kernel.org> Date: Thu Sep 3 16:14:31 2020 -0700 net: tighten the definition of interface statistics This patch is born out of an investigation into which IEEE statistics correspond to which struct rtnl_link_stats64 members. Turns out that there seems to be reasonable consensus on the matter, among many drivers. To save others the time (and it took more time than I'm comfortable admitting) I'm adding comments referring to IEEE attributes to struct rtnl_link_stats64. Up until now we had two forms of documentation for stats - in Documentation/ABI/testing/sysfs-class-net-statistics and the comments on struct rtnl_link_stats64 itself. While the former is very cautious in defining the expected behavior, the latter feel quite dated and may not be easy to understand for modern day driver author (e.g. rx_over_errors). At the same time modern systems are far more complex and once obvious definitions lost their clarity. For example - does rx_packet count at the MAC layer (aFramesReceivedOK)? packets processed correctly by hardware? received by the driver? or maybe received by the stack? I tried to clarify the expectations, further clarifications from others are very welcome. The part hardest to untangle is rx_over_errors vs rx_fifo_errors vs rx_missed_errors. After much deliberation I concluded that for modern HW only two of the counters will make sense. The distinction between internal FIFO overflow and packets dropped due to back-pressure from the host is likely too implementation (driver and device) specific to expose in the standard stats. Now - which two of those counters we select to use is anyone's pick: sysfs documentation suggests rx_over_errors counts packets which did not fit into buffers due to MTU being too small, which I reused. There don't seem to be many modern drivers using it (well, CAN drivers seem to love this statistic). Of the remaining two I picked rx_missed_errors to report device drops. bnxt reports it and it's folded into "drop"s in procfs (while rx_fifo_errors is an error, and modern devices usually receive the frame OK, they just can't admit it into the pipeline). Of the drivers I looked at only AMD Lance-like and NS8390-like use all three of these counters. rx_missed_errors counts missed frames, rx_over_errors counts overflow events, and rx_fifo_errors counts frames which were truncated because they didn't fit into buffers. This suggests that rx_fifo_errors may be the correct stat for truncated packets, but I'd think a FIFO stat counting truncated packets would be very confusing to a modern reader.
On Thu, Nov 7, 2024 at 3:23 AM Gal Pressman <gal@nvidia.com> wrote: > > On 06/11/2024 13:49, Yafang Shao wrote: > > On Wed, Nov 6, 2024 at 5:56 PM Tariq Toukan <ttoukan.linux@gmail.com> wrote: > >> > >> > >> > >> On 06/11/2024 8:40, Yafang Shao wrote: > >>> We observed a high number of rx_discards_phy events on some servers when > >>> running `ethtool -S`. However, this important counter is not currently > >>> reflected in the /proc/net/dev statistics file, making it challenging to > >>> monitor effectively. > >>> > >>> Since rx_missed_errors represents packets dropped due to buffer exhaustion, > >>> it makes sense to include rx_discards_phy in this counter to enhance > >>> monitoring visibility. This change will help administrators track these > >>> events more effectively through standard interfaces. > >>> > >> > >> Hi, > >> > >> Thanks for your patch. > >> > >> It's a matter of interpretation... > >> The documentation in > >> Documentation/ABI/testing/sysfs-class-net-statistics refers to the > >> driver for the exact meaning. > > I think this documentation is outdated, a more recent one is in if_link.h: Should we sync the documentation in if_link.h with Documentation/ABI/testing/sysfs-class-net-statistics?
On Thu, Nov 7, 2024 at 9:17 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 6 Nov 2024 21:23:47 +0200 Gal Pressman wrote: > > > It appears that rx_fifo_errors is a more appropriate counter for this purpose. > > > I will submit a v2. Thanks for your suggestion. > > > > Probably not a good idea: > > * This statistics was used interchangeably with @rx_over_errors. > > * Not recommended for use in drivers for high speed interfaces. > > FWIW we can change the definition. Let me copy paste below the commit > which added the docs because it has the background. > > tl;dr is that I was trying to push drivers towards a single stat to > keep things simple. If we have a clear definition of how rx_fifo_errors > would differ - we can reuse it and update the doc. For example if > rx_discards_phy usually means that the adapter itself is overwhelmed > (too many rules etc) that would be a pretty clear, since rx_missed is > supposed to primarily indicate that the host rings are full or perhaps > the PCIe interface of the NIC is struggling. But not the packet > processing. Thanks for providing the background. What do you suggest—should we report rx_discards_phy via rx_fifo_errors and update the documentation accordingly?
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c index 6f686fabed44..42c1b791a74c 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c @@ -3903,7 +3903,8 @@ mlx5e_get_stats(struct net_device *dev, struct rtnl_link_stats64 *stats) mlx5e_fold_sw_stats64(priv, stats); } - stats->rx_missed_errors = priv->stats.qcnt.rx_out_of_buffer; + stats->rx_missed_errors = priv->stats.qcnt.rx_out_of_buffer + + PPORT_2863_GET(pstats, if_in_discards); stats->rx_length_errors = PPORT_802_3_GET(pstats, a_in_range_length_errors) +
We observed a high number of rx_discards_phy events on some servers when running `ethtool -S`. However, this important counter is not currently reflected in the /proc/net/dev statistics file, making it challenging to monitor effectively. Since rx_missed_errors represents packets dropped due to buffer exhaustion, it makes sense to include rx_discards_phy in this counter to enhance monitoring visibility. This change will help administrators track these events more effectively through standard interfaces. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Cc: Saeed Mahameed <saeedm@nvidia.com> Cc: Tariq Toukan <tariqt@nvidia.com> Cc: Leon Romanovsky <leon@kernel.org> --- drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)