[v2,07/13] net/txgbe: add Tx descriptor error statistics

Count the number of packets not sent due to Tx descriptor error.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/txgbe/txgbe_ethdev.c | 6 ++++++
 drivers/net/txgbe/txgbe_rxtx.c   | 3 +++
 drivers/net/txgbe/txgbe_rxtx.h   | 1 +
 3 files changed, 10 insertions(+)
  

On 10/28/2024 2:31 AM, Jiawen Wu wrote:
> Count the number of packets not sent due to Tx descriptor error.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>  drivers/net/txgbe/txgbe_ethdev.c | 6 ++++++
>  drivers/net/txgbe/txgbe_rxtx.c   | 3 +++
>  drivers/net/txgbe/txgbe_rxtx.h   | 1 +
>  3 files changed, 10 insertions(+)
> 
> diff --git a/drivers/net/txgbe/txgbe_ethdev.c b/drivers/net/txgbe/txgbe_ethdev.c
> index bafa9cf829..0c76e986f4 100644
> --- a/drivers/net/txgbe/txgbe_ethdev.c
> +++ b/drivers/net/txgbe/txgbe_ethdev.c
> @@ -2344,6 +2344,7 @@ txgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>  	struct txgbe_hw_stats *hw_stats = TXGBE_DEV_STATS(dev);
>  	struct txgbe_stat_mappings *stat_mappings =
>  			TXGBE_DEV_STAT_MAPPINGS(dev);
> +	struct txgbe_tx_queue *txq;
>  	uint32_t i, j;
>  
>  	txgbe_read_stats_registers(hw, hw_stats);
> @@ -2398,6 +2399,11 @@ txgbe_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>  
>  	/* Tx Errors */
>  	stats->oerrors  = 0;
> +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> +		txq = dev->data->tx_queues[i];
> +		stats->oerrors += txq->desc_error;
> +	}
> +

stats_get is implemented, but stats_reset also needs to be implemented
for this stat.

>  	return 0;
>  }
>  
> diff --git a/drivers/net/txgbe/txgbe_rxtx.c b/drivers/net/txgbe/txgbe_rxtx.c
> index 06acbd0881..fc9e7b14f5 100644
> --- a/drivers/net/txgbe/txgbe_rxtx.c
> +++ b/drivers/net/txgbe/txgbe_rxtx.c
> @@ -894,6 +894,7 @@ txgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
>  		tx_pkt = *tx_pkts++;
>  		if (txgbe_check_pkt_err(tx_pkt)) {
>  			rte_pktmbuf_free(tx_pkt);
> +			txq->desc_error++;
>  			continue;
>  		}
>  
> @@ -2523,6 +2524,7 @@ txgbe_dev_tx_queue_setup(struct rte_eth_dev *dev,
>  	txgbe_set_tx_function(dev, txq);
>  
>  	txq->ops->reset(txq);
> +	txq->desc_error = 0;
>  
>  	dev->data->tx_queues[queue_idx] = txq;
>  
> @@ -4980,6 +4982,7 @@ txgbe_tx_queue_clear_error(void *param)
>  		if (!txq->resetting)
>  			continue;
>  
> +		txq->desc_error++;
>

Why error value is increased in this function, which resets the Tx queue?
Is the intention to reset the error value here?

>  		txgbe_dev_save_tx_queue(hw, i);
>  
>  		/* tx ring reset */
> diff --git a/drivers/net/txgbe/txgbe_rxtx.h b/drivers/net/txgbe/txgbe_rxtx.h
> index e668b60b1e..622a0d3981 100644
> --- a/drivers/net/txgbe/txgbe_rxtx.h
> +++ b/drivers/net/txgbe/txgbe_rxtx.h
> @@ -412,6 +412,7 @@ struct txgbe_tx_queue {
>  	/**< indicates that IPsec TX feature is in use */
>  #endif
>  	const struct rte_memzone *mz;
> +	uint64_t	    desc_error;
>  	bool		    resetting;
>  };
>
  

> > @@ -4980,6 +4982,7 @@ txgbe_tx_queue_clear_error(void *param)
> >  		if (!txq->resetting)
> >  			continue;
> >
> > +		txq->desc_error++;
> >
> 
> Why error value is increased in this function, which resets the Tx queue?
> Is the intention to reset the error value here?

When there is a desc error to cause Tx ring hang,  the interrupt directs to
reset of the queue. So we increase the error count in the specific queue
reset function. The queue is reset, but the error that led to the resetting
is recorded. The error count is only reset at the setup function.
  

On 11/1/2024 2:06 AM, Jiawen Wu wrote:
>>> @@ -4980,6 +4982,7 @@ txgbe_tx_queue_clear_error(void *param)
>>>  		if (!txq->resetting)
>>>  			continue;
>>>
>>> +		txq->desc_error++;
>>>
>>
>> Why error value is increased in this function, which resets the Tx queue?
>> Is the intention to reset the error value here?
> 
> When there is a desc error to cause Tx ring hang,  the interrupt directs to
> reset of the queue. So we increase the error count in the specific queue
> reset function. The queue is reset, but the error that led to the resetting
> is recorded. The error count is only reset at the setup function.
> 

Got it. But this variable counts number of bad packets. Increasing it
when Tx ring hang too may be confusing as there are two different
reasons to increase the counter.

Btw, .stats_reset() needs to reset this error stat.
  

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Friday, November 1, 2024 10:46 AM
> To: Jiawen Wu <jiawenwu@trustnetic.com>; dev@dpdk.org
> Subject: Re: [PATCH v2 07/13] net/txgbe: add Tx descriptor error statistics
> 
> On 11/1/2024 2:06 AM, Jiawen Wu wrote:
> >>> @@ -4980,6 +4982,7 @@ txgbe_tx_queue_clear_error(void *param)
> >>>  		if (!txq->resetting)
> >>>  			continue;
> >>>
> >>> +		txq->desc_error++;
> >>>
> >>
> >> Why error value is increased in this function, which resets the Tx queue?
> >> Is the intention to reset the error value here?
> >
> > When there is a desc error to cause Tx ring hang,  the interrupt directs to
> > reset of the queue. So we increase the error count in the specific queue
> > reset function. The queue is reset, but the error that led to the resetting
> > is recorded. The error count is only reset at the setup function.
> >
> 
> Got it. But this variable counts number of bad packets. Increasing it
> when Tx ring hang too may be confusing as there are two different
> reasons to increase the counter.

I think I should add a comment here:
/* Increase the count of Tx desc error since it causes the queue reset. */

> Btw, .stats_reset() needs to reset this error stat.

I'll add it into .stats_reset() in patch set v3.