Re: [dpdk-dev] [PATCH v3] net/nfb: new netcope driver

["Rastislav Černay" <cernay@netcope.com>]

>>> What is the point of adding when i >= RTE_ETHDEV_QUEUE_STAT_CNTRS ?

struct rte_eth_stats {
...
uint64_t q_opackets[RTE_ETHDEV_QUEUE_STAT_CNTRS]
...
}

As there can be more queues (nb_tx) then RTE_ETHDEV_QUEUE_STAT_CNTRS (16)
and struct rte_eth_stats eth_stats is allocated statically,
there is need to check so it does not write garbage somewhere.

>>> Besides, q_errors[] is for reception errors.
I will fix that, meanwhile could q_errors[] be renamed to q_ierrors[]? Also
could there be a way to publish output errors per queue, for example
q_oerrors[]?



On Mon, Mar 4, 2019 at 12:35 PM David Marchand <david.marchand@redhat.com>
wrote:

>
> On Fri, Mar 1, 2019 at 3:38 PM Rastislav Cernay <cernay@netcope.com>
> wrote:
>
>> diff --git a/drivers/net/nfb/nfb_stats.c b/drivers/net/nfb/nfb_stats.c
>> new file mode 100644
>> index 0000000..ffc27a5
>> --- /dev/null
>> +++ b/drivers/net/nfb/nfb_stats.c
>> @@ -0,0 +1,78 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause
>> + * Copyright(c) 2018 Cesnet
>> + * Copyright(c) 2018 Netcope Technologies, a.s. <info@netcope.com>
>> + * All rights reserved.
>> + */
>> +
>> +#include "nfb_stats.h"
>> +#include "nfb.h"
>> +
>> +int
>> +nfb_eth_stats_get(struct rte_eth_dev *dev,
>> +       struct rte_eth_stats *stats)
>> +{
>> +       uint16_t i;
>> +       uint16_t nb_rx = dev->data->nb_rx_queues;
>> +       uint16_t nb_tx = dev->data->nb_tx_queues;
>> +       uint64_t rx_total = 0;
>> +       uint64_t tx_total = 0;
>> +       uint64_t tx_err_total = 0;
>> +       uint64_t rx_total_bytes = 0;
>> +       uint64_t tx_total_bytes = 0;
>> +
>> +       struct ndp_rx_queue *rx_queue = *((struct ndp_rx_queue **)
>> +               dev->data->rx_queues);
>> +       struct ndp_tx_queue *tx_queue = *((struct ndp_tx_queue **)
>> +               dev->data->tx_queues);
>> +
>> +       for (i = 0; i < nb_rx; i++) {
>> +               if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
>> +                       stats->q_ipackets[i] = rx_queue[i].rx_pkts;
>> +                       stats->q_ibytes[i] = rx_queue[i].rx_bytes;
>> +               }
>> +               rx_total += stats->q_ipackets[i];
>> +               rx_total_bytes += stats->q_ibytes[i];
>> +       }
>>
>
> What is the point of adding when i >= RTE_ETHDEV_QUEUE_STAT_CNTRS ?
> Hopefully, ethdev passes a zero'd structure, but still I find it confusing.
>
>
>
>> +
>> +       for (i = 0; i < nb_tx; i++) {
>> +               if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
>> +                       stats->q_opackets[i] = tx_queue[i].tx_pkts;
>> +                       stats->q_obytes[i] = tx_queue[i].tx_bytes;
>> +                       stats->q_errors[i] = tx_queue[i].err_pkts;
>> +               }
>> +               tx_total += stats->q_opackets[i];
>> +               tx_total_bytes += stats->q_obytes[i];
>> +               tx_err_total += stats->q_errors[i];
>> +       }
>>
>
> Idem.
> Besides, q_errors[] is for reception errors.
>
> +
>> +       stats->ipackets = rx_total;
>> +       stats->opackets = tx_total;
>> +       stats->ibytes = rx_total_bytes;
>> +       stats->obytes = tx_total_bytes;
>> +       stats->oerrors = tx_err_total;
>> +       return 0;
>> +}
>>
>>
> --
> David Marchand
>