diff mbox series

[V4,net-next,4/4] net: hns3: support dump pfc frame statistics in tx timeout log

Message ID 20240105010119.2619873-5-shaojijie@huawei.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series There are some features for the HNS3 ethernet driver | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1113 this patch: 1113
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 1140 this patch: 1140
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1144 this patch: 1144
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 28 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jijie Shao Jan. 5, 2024, 1:01 a.m. UTC 
Continuous pfc frames may cause tx timeout.
Therefore, pfc frame statistics are added to logs.

Signed-off-by: Jijie Shao <shaojijie@huawei.com>
Reviewed-by: Simon Horman <horms@kernel.org>
---
 drivers/net/ethernet/hisilicon/hns3/hnae3.h             | 2 ++
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c         | 6 ++++--
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 2 ++
 3 files changed, 8 insertions(+), 2 deletions(-)

Comments

Jiri Pirko Jan. 5, 2024, 9:55 a.m. UTC | #1 
Fri, Jan 05, 2024 at 02:01:19AM CET, shaojijie@huawei.com wrote:
>Continuous pfc frames may cause tx timeout.
>Therefore, pfc frame statistics are added to logs.
>
>Signed-off-by: Jijie Shao <shaojijie@huawei.com>
>Reviewed-by: Simon Horman <horms@kernel.org>
>---
> drivers/net/ethernet/hisilicon/hns3/hnae3.h             | 2 ++
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c         | 6 ++++--
> drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 2 ++
> 3 files changed, 8 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
>index ff475b0eac22..bf1e386617bc 100644
>--- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
>+++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
>@@ -209,6 +209,8 @@ struct hnae3_queue {
> struct hns3_mac_stats {
> 	u64 tx_pause_cnt;
> 	u64 rx_pause_cnt;
>+	u64 tx_pfc_cnt;
>+	u64 rx_pfc_cnt;
> };
> 
> /* hnae3 loop mode */
>diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>index b618797a7e8d..8e237f0f4fc9 100644
>--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>@@ -2871,8 +2871,10 @@ static bool hns3_get_tx_timeo_queue_info(struct net_device *ndev)
> 		struct hns3_mac_stats mac_stats;
> 
> 		h->ae_algo->ops->get_mac_stats(h, &mac_stats);
>-		netdev_info(ndev, "tx_pause_cnt: %llu, rx_pause_cnt: %llu\n",
>-			    mac_stats.tx_pause_cnt, mac_stats.rx_pause_cnt);
>+		netdev_info(ndev,
>+			    "tx_pause_cnt: %llu, rx_pause_cnt: %llu, tx_pfc_cnt: %llu, rx_pfc_cnt: %llu\n",
>+			    mac_stats.tx_pause_cnt, mac_stats.rx_pause_cnt,
>+			    mac_stats.tx_pfc_cnt, mac_stats.rx_pfc_cnt);

Don't we have a better way to expose this? I mean, whenever there is a
patch that extends the amount of text written in dmesg, it smells.
We should rather reduce it.


> 	}
> 
> 	hns3_dump_queue_reg(ndev, tx_ring);
>diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>index cf85ef55a0f4..f70a1159de40 100644
>--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>@@ -775,6 +775,8 @@ static void hclge_get_mac_stat(struct hnae3_handle *handle,
> 
> 	mac_stats->tx_pause_cnt = hdev->mac_stats.mac_tx_mac_pause_num;
> 	mac_stats->rx_pause_cnt = hdev->mac_stats.mac_rx_mac_pause_num;
>+	mac_stats->tx_pfc_cnt = hdev->mac_stats.mac_tx_pfc_pause_pkt_num;
>+	mac_stats->rx_pfc_cnt = hdev->mac_stats.mac_rx_pfc_pause_pkt_num;
> }
> 
> static int hclge_parse_func_status(struct hclge_dev *hdev,
>-- 
>2.30.0
>
>
Jijie Shao Jan. 9, 2024, 8:19 a.m. UTC | #2 
on 2024/1/5 17:55, Jiri Pirko wrote:
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> @@ -2871,8 +2871,10 @@ static bool hns3_get_tx_timeo_queue_info(struct net_device *ndev)
>> 		struct hns3_mac_stats mac_stats;
>>
>> 		h->ae_algo->ops->get_mac_stats(h, &mac_stats);
>> -		netdev_info(ndev, "tx_pause_cnt: %llu, rx_pause_cnt: %llu\n",
>> -			    mac_stats.tx_pause_cnt, mac_stats.rx_pause_cnt);
>> +		netdev_info(ndev,
>> +			    "tx_pause_cnt: %llu, rx_pause_cnt: %llu, tx_pfc_cnt: %llu, rx_pfc_cnt: %llu\n",
>> +			    mac_stats.tx_pause_cnt, mac_stats.rx_pause_cnt,
>> +			    mac_stats.tx_pfc_cnt, mac_stats.rx_pfc_cnt);
> Don't we have a better way to expose this? I mean, whenever there is a
> patch that extends the amount of text written in dmesg, it smells.
> We should rather reduce it.
>
In fact, we include this part of the statistics in the ethtool -S 
statistics. However, if tx timeout occurs,the driver performs a reset 
attempt to recover it. And the statistics are cleared after the reset. 
Therefore, pfc statistics are added to tx timeout log to determine the 
timeout cause.
Jiri Pirko Jan. 9, 2024, 9 a.m. UTC | #3 
Tue, Jan 09, 2024 at 09:19:48AM CET, shaojijie@huawei.com wrote:
>
>on 2024/1/5 17:55, Jiri Pirko wrote:
>> > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> > @@ -2871,8 +2871,10 @@ static bool hns3_get_tx_timeo_queue_info(struct net_device *ndev)
>> > 		struct hns3_mac_stats mac_stats;
>> > 
>> > 		h->ae_algo->ops->get_mac_stats(h, &mac_stats);
>> > -		netdev_info(ndev, "tx_pause_cnt: %llu, rx_pause_cnt: %llu\n",
>> > -			    mac_stats.tx_pause_cnt, mac_stats.rx_pause_cnt);
>> > +		netdev_info(ndev,
>> > +			    "tx_pause_cnt: %llu, rx_pause_cnt: %llu, tx_pfc_cnt: %llu, rx_pfc_cnt: %llu\n",
>> > +			    mac_stats.tx_pause_cnt, mac_stats.rx_pause_cnt,
>> > +			    mac_stats.tx_pfc_cnt, mac_stats.rx_pfc_cnt);
>> Don't we have a better way to expose this? I mean, whenever there is a
>> patch that extends the amount of text written in dmesg, it smells.
>> We should rather reduce it.
>> 
>In fact, we include this part of the statistics in the ethtool -S statistics.
>However, if tx timeout occurs,the driver performs a reset attempt to recover
>it. And the statistics are cleared after the reset. Therefore, pfc statistics
>are added to tx timeout log to determine the timeout cause.

Does not sound correct at all. You are basically forcing user to check
the dmesg to understand the behaviour of stats he gets from ethtool. You
can expose "reset"/"recover" counter through ethtool to expose this fact
rather than dmesg print. Please don't add dmesg print.

>
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
index ff475b0eac22..bf1e386617bc 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
@@ -209,6 +209,8 @@  struct hnae3_queue {
 struct hns3_mac_stats {
 	u64 tx_pause_cnt;
 	u64 rx_pause_cnt;
+	u64 tx_pfc_cnt;
+	u64 rx_pfc_cnt;
 };
 
 /* hnae3 loop mode */
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index b618797a7e8d..8e237f0f4fc9 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -2871,8 +2871,10 @@  static bool hns3_get_tx_timeo_queue_info(struct net_device *ndev)
 		struct hns3_mac_stats mac_stats;
 
 		h->ae_algo->ops->get_mac_stats(h, &mac_stats);
-		netdev_info(ndev, "tx_pause_cnt: %llu, rx_pause_cnt: %llu\n",
-			    mac_stats.tx_pause_cnt, mac_stats.rx_pause_cnt);
+		netdev_info(ndev,
+			    "tx_pause_cnt: %llu, rx_pause_cnt: %llu, tx_pfc_cnt: %llu, rx_pfc_cnt: %llu\n",
+			    mac_stats.tx_pause_cnt, mac_stats.rx_pause_cnt,
+			    mac_stats.tx_pfc_cnt, mac_stats.rx_pfc_cnt);
 	}
 
 	hns3_dump_queue_reg(ndev, tx_ring);
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index cf85ef55a0f4..f70a1159de40 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -775,6 +775,8 @@  static void hclge_get_mac_stat(struct hnae3_handle *handle,
 
 	mac_stats->tx_pause_cnt = hdev->mac_stats.mac_tx_mac_pause_num;
 	mac_stats->rx_pause_cnt = hdev->mac_stats.mac_rx_mac_pause_num;
+	mac_stats->tx_pfc_cnt = hdev->mac_stats.mac_tx_pfc_pause_pkt_num;
+	mac_stats->rx_pfc_cnt = hdev->mac_stats.mac_rx_pfc_pause_pkt_num;
 }
 
 static int hclge_parse_func_status(struct hclge_dev *hdev,