[1/1] net/pcap: imissed stats support
Checks
Commit Message
Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
---
drivers/net/pcap/rte_eth_pcap.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
Comments
On 1/25/2021 5:58 PM, Ido Goshen wrote:
> Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
> ---
> drivers/net/pcap/rte_eth_pcap.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index a32b1f3f3..83e208514 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -58,6 +58,7 @@ struct queue_stat {
> volatile unsigned long pkts;
> volatile unsigned long bytes;
> volatile unsigned long err_pkts;
> + volatile unsigned long missed_reset;
> };
>
> struct pcap_rx_queue {
> @@ -680,11 +681,23 @@ eth_dev_info(struct rte_eth_dev *dev,
> return 0;
> }
>
> +static unsigned long
> +eth_stats_get_pcap_missed(struct rte_eth_dev *dev, unsigned int qid)
> +{
> + const struct pmd_process_private *pp = dev->process_private;
> + pcap_t *pcap = pp->rx_pcap[qid];
> + struct pcap_stat stat;
> + if (pcap_stats(pcap, &stat) != 0)
If the stats requested after "port stop" this will crash, since "port stop" will
close the pcap.
Although unlikely that stats will be called after "port stop", still it may be
and better to put a protection here.
> + return 0;
> + return stat.ps_drop;
> +}
> +
> static int
> eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
> {
> unsigned int i;
> unsigned long rx_packets_total = 0, rx_bytes_total = 0;
> + unsigned long rx_missed_total = 0;
> unsigned long tx_packets_total = 0, tx_bytes_total = 0;
> unsigned long tx_packets_err_total = 0;
> const struct pmd_internals *internal = dev->data->dev_private;
> @@ -695,6 +708,10 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
> stats->q_ibytes[i] = internal->rx_queue[i].rx_stat.bytes;
> rx_packets_total += stats->q_ipackets[i];
> rx_bytes_total += stats->q_ibytes[i];
> + unsigned long rx_missed = eth_stats_get_pcap_missed(dev, i);
> + if (rx_missed)
> + rx_missed_total = rx_missed -
> + internal->rx_queue[i].rx_stat.missed_reset;
rx_missed_total +=
After a "port stop" & "port start" the 'stat.ps_drop' will be reset, if stats
cleared before, because of stored 'missed_reset', possible to see very big (and
wrong) stat values.
Better to clear the 'missed_reset' on port stop.
> }
>
> for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS &&
> @@ -708,6 +725,7 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>
> stats->ipackets = rx_packets_total;
> stats->ibytes = rx_bytes_total;
> + stats->imissed = rx_missed_total;
> stats->opackets = tx_packets_total;
> stats->obytes = tx_bytes_total;
> stats->oerrors = tx_packets_err_total;
> @@ -724,6 +742,8 @@ eth_stats_reset(struct rte_eth_dev *dev)
> for (i = 0; i < dev->data->nb_rx_queues; i++) {
> internal->rx_queue[i].rx_stat.pkts = 0;
> internal->rx_queue[i].rx_stat.bytes = 0;
> + internal->rx_queue[i].rx_stat.missed_reset =
> + eth_stats_get_pcap_missed(dev, i);
> }
>
> for (i = 0; i < dev->data->nb_tx_queues; i++) {
>
On 1/25/2021 5:58 PM, Ido Goshen wrote:
> Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
<...>
> @@ -695,6 +708,10 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
> stats->q_ibytes[i] = internal->rx_queue[i].rx_stat.bytes;
> rx_packets_total += stats->q_ipackets[i];
> rx_bytes_total += stats->q_ibytes[i];
> + unsigned long rx_missed = eth_stats_get_pcap_missed(dev, i);
> + if (rx_missed)
> + rx_missed_total = rx_missed -
> + internal->rx_queue[i].rx_stat.missed_reset;
'ps_drop' seems u_32 type, do you know how it behaves on overflow? Do you think
do we need a check here for overflow?
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, 28 January 2021 20:21
> To: Ido Goshen <Ido@cgstowernetworks.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 1/1] net/pcap: imissed stats support
>
> On 1/25/2021 5:58 PM, Ido Goshen wrote:
> > Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
>
> <...>
>
> > @@ -695,6 +708,10 @@ eth_stats_get(struct rte_eth_dev *dev, struct
> rte_eth_stats *stats)
> > stats->q_ibytes[i] = internal->rx_queue[i].rx_stat.bytes;
> > rx_packets_total += stats->q_ipackets[i];
> > rx_bytes_total += stats->q_ibytes[i];
> > + unsigned long rx_missed = eth_stats_get_pcap_missed(dev, i);
> > + if (rx_missed)
> > + rx_missed_total = rx_missed -
> > + internal->rx_queue[i].rx_stat.missed_reset;
>
> 'ps_drop' seems u_32 type, do you know how it behaves on overflow? Do you
> think do we need a check here for overflow?
Right, it may overflow after few hours.
I don't see a way to fully solve it w/o periodic sampling which is quite an overhead
To compensate and avoid getting weird high ("negative") values
I can check if the last retrieved value is higher than the current, then either
zero it (restart) which will reflect rollover, or
add UINT_MAX hoping there was only one rollover since last sample
Please advice
On 2/1/2021 8:53 AM, Ido Goshen wrote:
>
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Thursday, 28 January 2021 20:21
>> To: Ido Goshen <Ido@cgstowernetworks.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH 1/1] net/pcap: imissed stats support
>>
>> On 1/25/2021 5:58 PM, Ido Goshen wrote:
>>> Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
>>
>> <...>
>>
>>> @@ -695,6 +708,10 @@ eth_stats_get(struct rte_eth_dev *dev, struct
>> rte_eth_stats *stats)
>>> stats->q_ibytes[i] = internal->rx_queue[i].rx_stat.bytes;
>>> rx_packets_total += stats->q_ipackets[i];
>>> rx_bytes_total += stats->q_ibytes[i];
>>> + unsigned long rx_missed = eth_stats_get_pcap_missed(dev, i);
>>> + if (rx_missed)
>>> + rx_missed_total = rx_missed -
>>> + internal->rx_queue[i].rx_stat.missed_reset;
>>
>> 'ps_drop' seems u_32 type, do you know how it behaves on overflow? Do you
>> think do we need a check here for overflow?
>
> Right, it may overflow after few hours.
> I don't see a way to fully solve it w/o periodic sampling which is quite an overhead
Agree
> To compensate and avoid getting weird high ("negative") values
> I can check if the last retrieved value is higher than the current, then either
> zero it (restart) which will reflect rollover, or
> add UINT_MAX hoping there was only one rollover since last sample
> Please advice
>
I would go with single rollover assumption, but comment this in the code.
@@ -58,6 +58,7 @@ struct queue_stat {
volatile unsigned long pkts;
volatile unsigned long bytes;
volatile unsigned long err_pkts;
+ volatile unsigned long missed_reset;
};
struct pcap_rx_queue {
@@ -680,11 +681,23 @@ eth_dev_info(struct rte_eth_dev *dev,
return 0;
}
+static unsigned long
+eth_stats_get_pcap_missed(struct rte_eth_dev *dev, unsigned int qid)
+{
+ const struct pmd_process_private *pp = dev->process_private;
+ pcap_t *pcap = pp->rx_pcap[qid];
+ struct pcap_stat stat;
+ if (pcap_stats(pcap, &stat) != 0)
+ return 0;
+ return stat.ps_drop;
+}
+
static int
eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
{
unsigned int i;
unsigned long rx_packets_total = 0, rx_bytes_total = 0;
+ unsigned long rx_missed_total = 0;
unsigned long tx_packets_total = 0, tx_bytes_total = 0;
unsigned long tx_packets_err_total = 0;
const struct pmd_internals *internal = dev->data->dev_private;
@@ -695,6 +708,10 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
stats->q_ibytes[i] = internal->rx_queue[i].rx_stat.bytes;
rx_packets_total += stats->q_ipackets[i];
rx_bytes_total += stats->q_ibytes[i];
+ unsigned long rx_missed = eth_stats_get_pcap_missed(dev, i);
+ if (rx_missed)
+ rx_missed_total = rx_missed -
+ internal->rx_queue[i].rx_stat.missed_reset;
}
for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS &&
@@ -708,6 +725,7 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
stats->ipackets = rx_packets_total;
stats->ibytes = rx_bytes_total;
+ stats->imissed = rx_missed_total;
stats->opackets = tx_packets_total;
stats->obytes = tx_bytes_total;
stats->oerrors = tx_packets_err_total;
@@ -724,6 +742,8 @@ eth_stats_reset(struct rte_eth_dev *dev)
for (i = 0; i < dev->data->nb_rx_queues; i++) {
internal->rx_queue[i].rx_stat.pkts = 0;
internal->rx_queue[i].rx_stat.bytes = 0;
+ internal->rx_queue[i].rx_stat.missed_reset =
+ eth_stats_get_pcap_missed(dev, i);
}
for (i = 0; i < dev->data->nb_tx_queues; i++) {