diff mbox series

[V2,04/14] net/hns3: add Rx and Tx bytes stats

Message ID 1614693534-27620-5-git-send-email-oulijun@huawei.com (mailing list archive)
State Changes Requested
Delegated to: Ferruh Yigit
Headers show
Series Features and bugfixes for hns3 | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Lijun Ou March 2, 2021, 1:58 p.m. UTC
From: "Min Hu (Connor)" <humin29@huawei.com>

In current HNS3 PMD, Rx/Tx bytes from packet stats are not
implemented.

This patch implemented Rx/Tx bytes using soft counters.
Rx/Tx bytes stats will be enabled if the macro
RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS is defined.

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
 drivers/net/hns3/hns3_rxtx.c          | 24 ++++++++++++++++++++++++
 drivers/net/hns3/hns3_rxtx_vec_neon.h | 15 +++++++++++++++
 drivers/net/hns3/hns3_rxtx_vec_sve.c  | 11 +++++++++++
 drivers/net/hns3/hns3_stats.c         | 22 ++++++++++++++++++----
 4 files changed, 68 insertions(+), 4 deletions(-)

Comments

Ferruh Yigit March 3, 2021, 1:28 p.m. UTC | #1
On 3/2/2021 1:58 PM, Lijun Ou wrote:
> From: "Min Hu (Connor)" <humin29@huawei.com>
> 
> In current HNS3 PMD, Rx/Tx bytes from packet stats are not
> implemented.
> 
> This patch implemented Rx/Tx bytes using soft counters.
> Rx/Tx bytes stats will be enabled if the macro
> RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS is defined.
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> ---
>   drivers/net/hns3/hns3_rxtx.c          | 24 ++++++++++++++++++++++++
>   drivers/net/hns3/hns3_rxtx_vec_neon.h | 15 +++++++++++++++
>   drivers/net/hns3/hns3_rxtx_vec_sve.c  | 11 +++++++++++
>   drivers/net/hns3/hns3_stats.c         | 22 ++++++++++++++++++----
>   4 files changed, 68 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
> index 5e79177..a8bd2cc 100644
> --- a/drivers/net/hns3/hns3_rxtx.c
> +++ b/drivers/net/hns3/hns3_rxtx.c
> @@ -2181,6 +2181,10 @@ hns3_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
>   					       cksum_err);
>   		hns3_rxd_to_vlan_tci(rxq, rxm, l234_info, &rxd);
>   
> +#ifdef RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS
> +		/* Increment bytes counter  */
> +		rxq->basic_stats.bytes += rxm->pkt_len;
> +#endif

copy/paste from previous version:

Why statistics enabled only with macro?
It is not common to use macro to enable the stats, what do you think to remove 
it, to be consistent with rest of the PMDs?
Lijun Ou March 3, 2021, 2:08 p.m. UTC | #2
在 2021/3/3 21:28, Ferruh Yigit 写道:
> On 3/2/2021 1:58 PM, Lijun Ou wrote:
>> From: "Min Hu (Connor)" <humin29@huawei.com>
>>
>> In current HNS3 PMD, Rx/Tx bytes from packet stats are not
>> implemented.
>>
>> This patch implemented Rx/Tx bytes using soft counters.
>> Rx/Tx bytes stats will be enabled if the macro
>> RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS is defined.
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> ---
>>   drivers/net/hns3/hns3_rxtx.c          | 24 ++++++++++++++++++++++++
>>   drivers/net/hns3/hns3_rxtx_vec_neon.h | 15 +++++++++++++++
>>   drivers/net/hns3/hns3_rxtx_vec_sve.c  | 11 +++++++++++
>>   drivers/net/hns3/hns3_stats.c         | 22 ++++++++++++++++++----
>>   4 files changed, 68 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
>> index 5e79177..a8bd2cc 100644
>> --- a/drivers/net/hns3/hns3_rxtx.c
>> +++ b/drivers/net/hns3/hns3_rxtx.c
>> @@ -2181,6 +2181,10 @@ hns3_recv_pkts(void *rx_queue, struct rte_mbuf 
>> **rx_pkts, uint16_t nb_pkts)
>>                              cksum_err);
>>           hns3_rxd_to_vlan_tci(rxq, rxm, l234_info, &rxd);
>> +#ifdef RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS
>> +        /* Increment bytes counter  */
>> +        rxq->basic_stats.bytes += rxm->pkt_len;
>> +#endif
> 
> copy/paste from previous version:
> 
> Why statistics enabled only with macro?
> It is not common to use macro to enable the stats, what do you think to 
> remove it, to be consistent with rest of the PMDs?
I'm sorry. I thought it was a success.
  Firstly, the macro is used to control the statistics to ensure 
performance and facilitate flexible usage. For example, the macro needs 
to be disabled when high performance is required.
secondly the byte statistics of other vendors are implemented by reading 
and writing registers. Therefore, macros are not used.By the way, the 
MLX driver has a precedent (code snippets can be intercepted here).
> _______________________________________________
> Linuxarm mailing list -- linuxarm@openeuler.org
> To unsubscribe send an email to linuxarm-leave@openeuler.org
Ferruh Yigit March 3, 2021, 2:24 p.m. UTC | #3
On 3/3/2021 2:08 PM, oulijun wrote:
> 
> 
> 在 2021/3/3 21:28, Ferruh Yigit 写道:
>> On 3/2/2021 1:58 PM, Lijun Ou wrote:
>>> From: "Min Hu (Connor)" <humin29@huawei.com>
>>>
>>> In current HNS3 PMD, Rx/Tx bytes from packet stats are not
>>> implemented.
>>>
>>> This patch implemented Rx/Tx bytes using soft counters.
>>> Rx/Tx bytes stats will be enabled if the macro
>>> RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS is defined.
>>>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>> ---
>>>   drivers/net/hns3/hns3_rxtx.c          | 24 ++++++++++++++++++++++++
>>>   drivers/net/hns3/hns3_rxtx_vec_neon.h | 15 +++++++++++++++
>>>   drivers/net/hns3/hns3_rxtx_vec_sve.c  | 11 +++++++++++
>>>   drivers/net/hns3/hns3_stats.c         | 22 ++++++++++++++++++----
>>>   4 files changed, 68 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
>>> index 5e79177..a8bd2cc 100644
>>> --- a/drivers/net/hns3/hns3_rxtx.c
>>> +++ b/drivers/net/hns3/hns3_rxtx.c
>>> @@ -2181,6 +2181,10 @@ hns3_recv_pkts(void *rx_queue, struct rte_mbuf 
>>> **rx_pkts, uint16_t nb_pkts)
>>>                              cksum_err);
>>>           hns3_rxd_to_vlan_tci(rxq, rxm, l234_info, &rxd);
>>> +#ifdef RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS
>>> +        /* Increment bytes counter  */
>>> +        rxq->basic_stats.bytes += rxm->pkt_len;
>>> +#endif
>>
>> copy/paste from previous version:
>>
>> Why statistics enabled only with macro?
>> It is not common to use macro to enable the stats, what do you think to remove 
>> it, to be consistent with rest of the PMDs?
> I'm sorry. I thought it was a success.
>   Firstly, the macro is used to control the statistics to ensure performance and 
> facilitate flexible usage. For example, the macro needs to be disabled when high 
> performance is required.
> secondly the byte statistics of other vendors are implemented by reading and 
> writing registers. Therefore, macros are not used.By the way, the MLX driver has 
> a precedent (code snippets can be intercepted here).

It is not convenient for a user re-compile the DPDK to be able to get the ethdev 
byte statistics, stats are not developer/debug information, end user may need 
them. And this recompilation may not be an option for the distributed software.

How much performance affect it has if you enable it always?

And just to double check, isn't there a way to get this information from HW 
without calculating it in the driver?

The compile time flags, specially after meson switch, hard to enable and a 
little hidden, it is very easy to have broken code there, that is why it better 
to prevent compile time flags as much as possible.
Lijun Ou March 4, 2021, 1:36 a.m. UTC | #4
在 2021/3/3 22:24, Ferruh Yigit 写道:
> On 3/3/2021 2:08 PM, oulijun wrote:
>>
>>
>> 在 2021/3/3 21:28, Ferruh Yigit 写道:
>>> On 3/2/2021 1:58 PM, Lijun Ou wrote:
>>>> From: "Min Hu (Connor)" <humin29@huawei.com>
>>>>
>>>> In current HNS3 PMD, Rx/Tx bytes from packet stats are not
>>>> implemented.
>>>>
>>>> This patch implemented Rx/Tx bytes using soft counters.
>>>> Rx/Tx bytes stats will be enabled if the macro
>>>> RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS is defined.
>>>>
>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>> ---
>>>>   drivers/net/hns3/hns3_rxtx.c          | 24 ++++++++++++++++++++++++
>>>>   drivers/net/hns3/hns3_rxtx_vec_neon.h | 15 +++++++++++++++
>>>>   drivers/net/hns3/hns3_rxtx_vec_sve.c  | 11 +++++++++++
>>>>   drivers/net/hns3/hns3_stats.c         | 22 ++++++++++++++++++----
>>>>   4 files changed, 68 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/net/hns3/hns3_rxtx.c 
>>>> b/drivers/net/hns3/hns3_rxtx.c
>>>> index 5e79177..a8bd2cc 100644
>>>> --- a/drivers/net/hns3/hns3_rxtx.c
>>>> +++ b/drivers/net/hns3/hns3_rxtx.c
>>>> @@ -2181,6 +2181,10 @@ hns3_recv_pkts(void *rx_queue, struct 
>>>> rte_mbuf **rx_pkts, uint16_t nb_pkts)
>>>>                              cksum_err);
>>>>           hns3_rxd_to_vlan_tci(rxq, rxm, l234_info, &rxd);
>>>> +#ifdef RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS
>>>> +        /* Increment bytes counter  */
>>>> +        rxq->basic_stats.bytes += rxm->pkt_len;
>>>> +#endif
>>>
>>> copy/paste from previous version:
>>>
>>> Why statistics enabled only with macro?
>>> It is not common to use macro to enable the stats, what do you think 
>>> to remove it, to be consistent with rest of the PMDs?
>> I'm sorry. I thought it was a success.
>>   Firstly, the macro is used to control the statistics to ensure 
>> performance and facilitate flexible usage. For example, the macro 
>> needs to be disabled when high performance is required.
>> secondly the byte statistics of other vendors are implemented by 
>> reading and writing registers. Therefore, macros are not used.By the 
>> way, the MLX driver has a precedent (code snippets can be intercepted 
>> here).
> 
> It is not convenient for a user re-compile the DPDK to be able to get 
> the ethdev byte statistics, stats are not developer/debug information, 
> end user may need them. And this recompilation may not be an option for 
> the distributed software.
> 
> How much performance affect it has if you enable it always?
> 
We theoretically analyzed that being on a critical path might have a 
performance impact. Maybe the actual mountain is negligible.
> And just to double check, isn't there a way to get this information from 
> HW without calculating it in the driver?
> 
OK
> The compile time flags, specially after meson switch, hard to enable and 
> a little hidden, it is very easy to have broken code there, that is why 
> it better to prevent compile time flags as much as possible.
> .
After internal analysis, you can accept your comments and remove macros.
I will fix it.
>
diff mbox series

Patch

diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index 5e79177..a8bd2cc 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -2181,6 +2181,10 @@  hns3_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 					       cksum_err);
 		hns3_rxd_to_vlan_tci(rxq, rxm, l234_info, &rxd);
 
+#ifdef RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS
+		/* Increment bytes counter  */
+		rxq->basic_stats.bytes += rxm->pkt_len;
+#endif
 		rx_pkts[nb_rx++] = rxm;
 		continue;
 pkt_err:
@@ -2401,6 +2405,10 @@  hns3_recv_scattered_pkts(void *rx_queue,
 					       cksum_err);
 		hns3_rxd_to_vlan_tci(rxq, first_seg, l234_info, &rxd);
 
+#ifdef RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS
+		/* Increment bytes counter */
+		rxq->basic_stats.bytes += first_seg->pkt_len;
+#endif
 		rx_pkts[nb_rx++] = first_seg;
 		first_seg = NULL;
 		continue;
@@ -3516,6 +3524,13 @@  hns3_tx_fill_hw_ring(struct hns3_tx_queue *txq,
 	for (i = 0; i < mainpart; i += PER_LOOP_NUM) {
 		hns3_tx_backup_4mbuf(tx_entry + i, pkts + i);
 		hns3_tx_setup_4bd(txdp + i, pkts + i);
+
+#ifdef RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS
+		/* Increment bytes counter */
+		uint32_t j;
+		for (j = 0; j < PER_LOOP_NUM; j++)
+			txq->basic_stats.bytes += pkts[i + j]->pkt_len;
+#endif
 	}
 	if (unlikely(leftover > 0)) {
 		for (i = 0; i < leftover; i++) {
@@ -3523,6 +3538,11 @@  hns3_tx_fill_hw_ring(struct hns3_tx_queue *txq,
 					     pkts + mainpart + i);
 			hns3_tx_setup_1bd(txdp + mainpart + i,
 					  pkts + mainpart + i);
+
+#ifdef RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS
+			/* Increment bytes counter */
+			txq->basic_stats.bytes += pkts[mainpart + i]->pkt_len;
+#endif
 		}
 	}
 }
@@ -3661,6 +3681,10 @@  hns3_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 		desc->tx.tp_fe_sc_vld_ra_ri |=
 				 rte_cpu_to_le_16(BIT(HNS3_TXD_FE_B));
 
+#ifdef RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS
+		/* Increment bytes counter */
+		txq->basic_stats.bytes += tx_pkt->pkt_len;
+#endif
 		nb_hold += i;
 		txq->next_to_use = tx_next_use;
 		txq->tx_bd_ready -= i;
diff --git a/drivers/net/hns3/hns3_rxtx_vec_neon.h b/drivers/net/hns3/hns3_rxtx_vec_neon.h
index a693b4b..54d358d 100644
--- a/drivers/net/hns3/hns3_rxtx_vec_neon.h
+++ b/drivers/net/hns3/hns3_rxtx_vec_neon.h
@@ -61,6 +61,11 @@  hns3_xmit_fixed_burst_vec(void *__restrict tx_queue,
 		for (i = 0; i < n; i++, tx_pkts++, tx_desc++) {
 			hns3_vec_tx(tx_desc, *tx_pkts);
 			tx_entry[i].mbuf = *tx_pkts;
+
+#ifdef RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS
+			/* Increment bytes counter */
+			txq->basic_stats.bytes += (*tx_pkts)->pkt_len;
+#endif
 		}
 
 		nb_commit -= n;
@@ -72,6 +77,11 @@  hns3_xmit_fixed_burst_vec(void *__restrict tx_queue,
 	for (i = 0; i < nb_commit; i++, tx_pkts++, tx_desc++) {
 		hns3_vec_tx(tx_desc, *tx_pkts);
 		tx_entry[i].mbuf = *tx_pkts;
+
+#ifdef RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS
+		/* Increment bytes counter */
+		txq->basic_stats.bytes += (*tx_pkts)->pkt_len;
+#endif
 	}
 
 	next_to_use += nb_commit;
@@ -116,6 +126,11 @@  hns3_desc_parse_field(struct hns3_rx_queue *rxq,
 		if (likely(bd_base_info & BIT(HNS3_RXD_L3L4P_B)))
 			hns3_rx_set_cksum_flag(pkt, pkt->packet_type,
 					       cksum_err);
+
+#ifdef RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS
+		/* Increment bytes counter */
+		rxq->basic_stats.bytes += pkt->pkt_len;
+#endif
 	}
 
 	return retcode;
diff --git a/drivers/net/hns3/hns3_rxtx_vec_sve.c b/drivers/net/hns3/hns3_rxtx_vec_sve.c
index b02bae7..8b9172a 100644
--- a/drivers/net/hns3/hns3_rxtx_vec_sve.c
+++ b/drivers/net/hns3/hns3_rxtx_vec_sve.c
@@ -58,6 +58,11 @@  hns3_desc_parse_field_sve(struct hns3_rx_queue *rxq,
 		if (likely(key->bd_base_info[i] & BIT(HNS3_RXD_L3L4P_B)))
 			hns3_rx_set_cksum_flag(rx_pkts[i],
 					rx_pkts[i]->packet_type, cksum_err);
+
+#ifdef RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS
+		/* Increment bytes counter */
+		rxq->basic_stats.bytes += rx_pkts[i]->pkt_len;
+#endif
 	}
 
 	return retcode;
@@ -408,6 +413,12 @@  hns3_tx_fill_hw_ring_sve(struct hns3_tx_queue *txq,
 		svst1_scatter_u64offset_u64(pg, (uint64_t *)&txdp->tx.paylen,
 					    offsets, svdup_n_u64(valid_bit));
 
+#ifdef RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS
+		/* Increment bytes counter */
+		uint32_t idx;
+		for (idx = 0; idx < svcntd(); idx++)
+			txq->basic_stats.bytes += pkts[idx]->pkt_len;
+#endif
 		/* update index for next loop */
 		i += svcntd();
 		pkts += svcntd();
diff --git a/drivers/net/hns3/hns3_stats.c b/drivers/net/hns3/hns3_stats.c
index e0e40ca..7796585 100644
--- a/drivers/net/hns3/hns3_stats.c
+++ b/drivers/net/hns3/hns3_stats.c
@@ -358,6 +358,7 @@  static const struct hns3_xstats_name_offset hns3_tx_queue_strings[] = {
 			    HNS3_NUM_RESET_XSTATS)
 
 static void hns3_tqp_stats_clear(struct hns3_hw *hw);
+static void hns3_tqp_basic_stats_clear(struct rte_eth_dev *dev);
 
 /*
  * Query all the MAC statistics data of Network ICL command ,opcode id: 0x0034.
@@ -543,16 +544,30 @@  hns3_stats_get(struct rte_eth_dev *eth_dev, struct rte_eth_stats *rte_stats)
 		return ret;
 	}
 
-	/* Get the error stats of received packets */
+	/* Get the error stats and bytes of received packets */
 	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
 		rxq = eth_dev->data->rx_queues[i];
 		if (rxq) {
 			cnt = rxq->err_stats.l2_errors +
 				rxq->err_stats.pkt_len_errors;
 			rte_stats->ierrors += cnt;
+
+#ifdef RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS
+			rte_stats->ibytes += rxq->basic_stats.bytes;
+#endif
 		}
 	}
 
+#ifdef RTE_LIBRTE_HNS3_PMD_SOFT_COUNTERS
+	/* Get the bytes of received packets */
+	struct hns3_tx_queue *txq;
+	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
+		txq = eth_dev->data->tx_queues[i];
+		if (txq)
+			rte_stats->obytes += txq->basic_stats.bytes;
+	}
+#endif
+
 	rte_stats->oerrors = 0;
 	/*
 	 * If HW statistics are reset by stats_reset, but a lot of residual
@@ -623,6 +638,7 @@  hns3_stats_reset(struct rte_eth_dev *eth_dev)
 	 * their source.
 	 */
 	hns3_tqp_stats_clear(hw);
+	hns3_tqp_basic_stats_clear(eth_dev);
 
 	return 0;
 }
@@ -807,7 +823,6 @@  hns3_rxq_basic_stats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 		rxq_stats->packets =
 			stats->rcb_rx_ring_pktnum[i] > rxq_stats->errors ?
 			stats->rcb_rx_ring_pktnum[i] - rxq_stats->errors : 0;
-		rxq_stats->bytes = 0;
 		for (j = 0; j < HNS3_NUM_RXQ_BASIC_STATS; j++) {
 			val = (char *)rxq_stats +
 				hns3_rxq_basic_stats_strings[j].offset;
@@ -836,7 +851,7 @@  hns3_txq_basic_stats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
 
 		txq_stats = &txq->basic_stats;
 		txq_stats->packets = stats->rcb_tx_ring_pktnum[i];
-		txq_stats->bytes = 0;
+
 		for (j = 0; j < HNS3_NUM_TXQ_BASIC_STATS; j++) {
 			val = (char *)txq_stats +
 				hns3_txq_basic_stats_strings[j].offset;
@@ -1328,7 +1343,6 @@  hns3_dev_xstats_reset(struct rte_eth_dev *dev)
 	if (ret)
 		return ret;
 
-	hns3_tqp_basic_stats_clear(dev);
 	hns3_tqp_dfx_stats_clear(dev);
 
 	/* Clear reset stats */