[2/2] net/cxgbe: remove queue stats from basic stats
Checks
Commit Message
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
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?
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
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.
@@ -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)