diff mbox series

[2/2] net/cxgbe: remove queue stats from basic stats

Message ID 486c59f029b52b0931d3517a5034bac8622fb906.1622738751.git.rahul.lakkireddy@chelsio.com (mailing list archive)
State Changes Requested, archived
Delegated to: Andrew Rybchenko
Headers
Series net/cxgbe: add support for xstats API |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues
ci/iol-intel-Performance success Performance Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/github-robot success github build: passed
ci/iol-abi-testing warning Testing issues
ci/iol-testing fail Testing issues

Commit Message

Rahul Lakkireddy June 3, 2021, 3:30 p.m. UTC 
  Remove queue stats from basic stats because they're now available
via xstats API. Also remove RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS flag.

Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
---
 drivers/net/cxgbe/cxgbe_ethdev.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

Comments

Andrew Rybchenko July 1, 2021, 2:50 p.m. UTC | #1 
On 6/3/21 6:30 PM, Rahul Lakkireddy wrote:
> Remove queue stats from basic stats because they're now available
> via xstats API. Also remove RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS flag.
> 
> Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
> ---
>  drivers/net/cxgbe/cxgbe_ethdev.c | 18 +++---------------
>  1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
> index 8dee3fc3b6..141a2de808 100644
> --- a/drivers/net/cxgbe/cxgbe_ethdev.c
> +++ b/drivers/net/cxgbe/cxgbe_ethdev.c
> @@ -747,22 +747,12 @@ static int cxgbe_dev_stats_get(struct rte_eth_dev *eth_dev,
>  	eth_stats->oerrors  = ps.tx_error_frames;
>  
>  	for (i = 0; i < pi->n_rx_qsets; i++) {
> -		struct sge_eth_rxq *rxq =
> -			&s->ethrxq[pi->first_rxqset + i];
> +		struct sge_eth_rxq *rxq = &s->ethrxq[pi->first_rxqset + i];
>  
> -		eth_stats->q_ipackets[i] = rxq->stats.pkts;
> -		eth_stats->q_ibytes[i] = rxq->stats.rx_bytes;
> -		eth_stats->ipackets += eth_stats->q_ipackets[i];
> -		eth_stats->ibytes += eth_stats->q_ibytes[i];
> +		eth_stats->ipackets += rxq->stats.pkts;
> +		eth_stats->ibytes += rxq->stats.rx_bytes;
>  	}
>  
> -	for (i = 0; i < pi->n_tx_qsets; i++) {
> -		struct sge_eth_txq *txq =
> -			&s->ethtxq[pi->first_txqset + i];
> -
> -		eth_stats->q_opackets[i] = txq->stats.pkts;
> -		eth_stats->q_obytes[i] = txq->stats.tx_bytes;
> -	}
>  	return 0;
>  }
>  
> @@ -1697,8 +1687,6 @@ static int eth_cxgbe_dev_init(struct rte_eth_dev *eth_dev)
>  		return 0;
>  	}
>  
> -	eth_dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
> -
>  	snprintf(name, sizeof(name), "cxgbeadapter%d", eth_dev->data->port_id);
>  	adapter = rte_zmalloc(name, sizeof(*adapter), 0);
>  	if (!adapter)
> 

As I understand it is correct to remove
RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS to avoid duplication of per-queue
stats in xstats. But I don't understand why per-queue
stats are removed from basic stats. It still makes sense and
useful. Am I missing something?
Rahul Lakkireddy July 1, 2021, 3:26 p.m. UTC | #2 
Hi Andrew,

On Thursday, July 07/01/21, 2021 at 17:50:19 +0300, Andrew Rybchenko wrote:
> On 6/3/21 6:30 PM, Rahul Lakkireddy wrote:
> > Remove queue stats from basic stats because they're now available
> > via xstats API. Also remove RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS flag.
> > 
> > Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
> > ---
> >  drivers/net/cxgbe/cxgbe_ethdev.c | 18 +++---------------
> >  1 file changed, 3 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
> > index 8dee3fc3b6..141a2de808 100644
> > --- a/drivers/net/cxgbe/cxgbe_ethdev.c
> > +++ b/drivers/net/cxgbe/cxgbe_ethdev.c
> > @@ -747,22 +747,12 @@ static int cxgbe_dev_stats_get(struct rte_eth_dev *eth_dev,
> >  	eth_stats->oerrors  = ps.tx_error_frames;
> >  
> >  	for (i = 0; i < pi->n_rx_qsets; i++) {
> > -		struct sge_eth_rxq *rxq =
> > -			&s->ethrxq[pi->first_rxqset + i];
> > +		struct sge_eth_rxq *rxq = &s->ethrxq[pi->first_rxqset + i];
> >  
> > -		eth_stats->q_ipackets[i] = rxq->stats.pkts;
> > -		eth_stats->q_ibytes[i] = rxq->stats.rx_bytes;
> > -		eth_stats->ipackets += eth_stats->q_ipackets[i];
> > -		eth_stats->ibytes += eth_stats->q_ibytes[i];
> > +		eth_stats->ipackets += rxq->stats.pkts;
> > +		eth_stats->ibytes += rxq->stats.rx_bytes;
> >  	}
> >  
> > -	for (i = 0; i < pi->n_tx_qsets; i++) {
> > -		struct sge_eth_txq *txq =
> > -			&s->ethtxq[pi->first_txqset + i];
> > -
> > -		eth_stats->q_opackets[i] = txq->stats.pkts;
> > -		eth_stats->q_obytes[i] = txq->stats.tx_bytes;
> > -	}
> >  	return 0;
> >  }
> >  
> > @@ -1697,8 +1687,6 @@ static int eth_cxgbe_dev_init(struct rte_eth_dev *eth_dev)
> >  		return 0;
> >  	}
> >  
> > -	eth_dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
> > -
> >  	snprintf(name, sizeof(name), "cxgbeadapter%d", eth_dev->data->port_id);
> >  	adapter = rte_zmalloc(name, sizeof(*adapter), 0);
> >  	if (!adapter)
> > 
> 
> As I understand it is correct to remove
> RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS to avoid duplication of per-queue
> stats in xstats. But I don't understand why per-queue
> stats are removed from basic stats. It still makes sense and
> useful. Am I missing something?

The per-queue stats are being removed from basic stats here because
these will be removed from rte_eth_stats, as per the below deprecation
notice.

* ethdev: Queue specific stats fields will be removed from ``struct rte_eth_stats``.
  Mentioned fields are: ``q_ipackets``, ``q_opackets``, ``q_ibytes``, ``q_obytes``,
  ``q_errors``.
  Instead queue stats will be received via xstats API. Current method support
  will be limited to maximum 256 queues.
  Also compile time flag ``RTE_ETHDEV_QUEUE_STAT_CNTRS`` will be removed.

Thanks,
Rahul
Andrew Rybchenko July 1, 2021, 3:33 p.m. UTC | #3 
Hi Rahul,

On 7/1/21 6:26 PM, Rahul Lakkireddy wrote:
> Hi Andrew,
> 
> On Thursday, July 07/01/21, 2021 at 17:50:19 +0300, Andrew Rybchenko wrote:
>> On 6/3/21 6:30 PM, Rahul Lakkireddy wrote:
>>> Remove queue stats from basic stats because they're now available
>>> via xstats API. Also remove RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS flag.
>>>
>>> Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
>>> ---
>>>  drivers/net/cxgbe/cxgbe_ethdev.c | 18 +++---------------
>>>  1 file changed, 3 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
>>> index 8dee3fc3b6..141a2de808 100644
>>> --- a/drivers/net/cxgbe/cxgbe_ethdev.c
>>> +++ b/drivers/net/cxgbe/cxgbe_ethdev.c
>>> @@ -747,22 +747,12 @@ static int cxgbe_dev_stats_get(struct rte_eth_dev *eth_dev,
>>>  	eth_stats->oerrors  = ps.tx_error_frames;
>>>  
>>>  	for (i = 0; i < pi->n_rx_qsets; i++) {
>>> -		struct sge_eth_rxq *rxq =
>>> -			&s->ethrxq[pi->first_rxqset + i];
>>> +		struct sge_eth_rxq *rxq = &s->ethrxq[pi->first_rxqset + i];
>>>  
>>> -		eth_stats->q_ipackets[i] = rxq->stats.pkts;
>>> -		eth_stats->q_ibytes[i] = rxq->stats.rx_bytes;
>>> -		eth_stats->ipackets += eth_stats->q_ipackets[i];
>>> -		eth_stats->ibytes += eth_stats->q_ibytes[i];
>>> +		eth_stats->ipackets += rxq->stats.pkts;
>>> +		eth_stats->ibytes += rxq->stats.rx_bytes;
>>>  	}
>>>  
>>> -	for (i = 0; i < pi->n_tx_qsets; i++) {
>>> -		struct sge_eth_txq *txq =
>>> -			&s->ethtxq[pi->first_txqset + i];
>>> -
>>> -		eth_stats->q_opackets[i] = txq->stats.pkts;
>>> -		eth_stats->q_obytes[i] = txq->stats.tx_bytes;
>>> -	}
>>>  	return 0;
>>>  }
>>>  
>>> @@ -1697,8 +1687,6 @@ static int eth_cxgbe_dev_init(struct rte_eth_dev *eth_dev)
>>>  		return 0;
>>>  	}
>>>  
>>> -	eth_dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
>>> -
>>>  	snprintf(name, sizeof(name), "cxgbeadapter%d", eth_dev->data->port_id);
>>>  	adapter = rte_zmalloc(name, sizeof(*adapter), 0);
>>>  	if (!adapter)
>>>
>>
>> As I understand it is correct to remove
>> RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS to avoid duplication of per-queue
>> stats in xstats. But I don't understand why per-queue
>> stats are removed from basic stats. It still makes sense and
>> useful. Am I missing something?
> 
> The per-queue stats are being removed from basic stats here because
> these will be removed from rte_eth_stats, as per the below deprecation
> notice.
> 
> * ethdev: Queue specific stats fields will be removed from ``struct rte_eth_stats``.
>   Mentioned fields are: ``q_ipackets``, ``q_opackets``, ``q_ibytes``, ``q_obytes``,
>   ``q_errors``.
>   Instead queue stats will be received via xstats API. Current method support
>   will be limited to maximum 256 queues.
>   Also compile time flag ``RTE_ETHDEV_QUEUE_STAT_CNTRS`` will be removed.

Thanks a lot. It is bad that I've lost it from my view.
Clear now.

Andrew.
diff mbox series

Patch

diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
index 8dee3fc3b6..141a2de808 100644
--- a/drivers/net/cxgbe/cxgbe_ethdev.c
+++ b/drivers/net/cxgbe/cxgbe_ethdev.c
@@ -747,22 +747,12 @@  static int cxgbe_dev_stats_get(struct rte_eth_dev *eth_dev,
 	eth_stats->oerrors  = ps.tx_error_frames;
 
 	for (i = 0; i < pi->n_rx_qsets; i++) {
-		struct sge_eth_rxq *rxq =
-			&s->ethrxq[pi->first_rxqset + i];
+		struct sge_eth_rxq *rxq = &s->ethrxq[pi->first_rxqset + i];
 
-		eth_stats->q_ipackets[i] = rxq->stats.pkts;
-		eth_stats->q_ibytes[i] = rxq->stats.rx_bytes;
-		eth_stats->ipackets += eth_stats->q_ipackets[i];
-		eth_stats->ibytes += eth_stats->q_ibytes[i];
+		eth_stats->ipackets += rxq->stats.pkts;
+		eth_stats->ibytes += rxq->stats.rx_bytes;
 	}
 
-	for (i = 0; i < pi->n_tx_qsets; i++) {
-		struct sge_eth_txq *txq =
-			&s->ethtxq[pi->first_txqset + i];
-
-		eth_stats->q_opackets[i] = txq->stats.pkts;
-		eth_stats->q_obytes[i] = txq->stats.tx_bytes;
-	}
 	return 0;
 }
 
@@ -1697,8 +1687,6 @@  static int eth_cxgbe_dev_init(struct rte_eth_dev *eth_dev)
 		return 0;
 	}
 
-	eth_dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
-
 	snprintf(name, sizeof(name), "cxgbeadapter%d", eth_dev->data->port_id);
 	adapter = rte_zmalloc(name, sizeof(*adapter), 0);
 	if (!adapter)