> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@intel.com>
> Sent: Thursday, 4 February 2021 11:26
> To: Ido Goshen <i...@cgstowernetworks.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v3 1/1] net/pcap: imissed stats support
>
> On 2/4/2021 7:56 AM, Ido Goshen wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yi...@intel.com>
> >> Sent: Thursday, 4 February 2021 2:13
> >> To: Ido Goshen <i...@cgstowernetworks.com>
> >> Cc: dev@dpdk.org
> >> Subject: Re: [PATCH v3 1/1] net/pcap: imissed stats support
> >>
> >> On 2/3/2021 11:07 PM, Ido Goshen wrote:
> >>> get value from pcap_stats.ps_drop (see man pcap_stats) the value is
> >>> adjusted in this cases:
> >>> - port stop - pcap is closed and will lose count
> >>> - stats reset - pcap doesn't provide reset api
> >>> - rollover - pcap counter size is u_32 only
> >>>
> >>> Signed-off-by: Ido Goshen <i...@cgstowernetworks.com>
> >>> ---
> >>> v3:
> >>> * code cleanup by dedicated struct and functions extraction
> >>> * multi stop support by menmonic+= accumulation
> >>> * rollover fixup
> >>>
> >>> v2:
> >>> * sum all queues (rx_missed_total += fix)
> >>> * null pcap protection
> >>> * inter stop/start persistancy (counter won't reset on stop)
> >>>
> >>> drivers/net/pcap/rte_eth_pcap.c | 59
> >> +++++++++++++++++++++++++++++++++
> >>> 1 file changed, 59 insertions(+)
> >>>
> >>> diff --git a/drivers/net/pcap/rte_eth_pcap.c
> >>> b/drivers/net/pcap/rte_eth_pcap.c index a32b1f3f3..16e8752f3 100644
> >>> --- a/drivers/net/pcap/rte_eth_pcap.c
> >>> +++ b/drivers/net/pcap/rte_eth_pcap.c
> >>> @@ -60,11 +60,21 @@ struct queue_stat {
> >>> volatile unsigned long err_pkts;
> >>> };
> >>>
> >>> +struct queue_missed_stat {
> >>> + /* last value retrieved from pcap */
> >>> + volatile unsigned int pcap;
> >>> + /* stores values lost by pcap stop or rollover */
> >>> + volatile unsigned long mnemonic;
> >>> + /* value on last reset */
> >>> + volatile unsigned long reset;
> >>> +};
> >>
> >> I am aware other stats has 'volatile' keyword, but as far as I can
> >> see it is not needed, since these are new ones can you please drop the
> 'volatile'?
> >
> > ok
> >
> >>
> >>> +
> >>> struct pcap_rx_queue {
> >>> uint16_t port_id;
> >>> uint16_t queue_id;
> >>> struct rte_mempool *mb_pool;
> >>> struct queue_stat rx_stat;
> >>> + struct queue_missed_stat missed_stat;
> >>> char name[PATH_MAX];
> >>> char type[ETH_PCAP_ARG_MAXLEN];
> >>>
> >>> @@ -144,6 +154,49 @@ RTE_LOG_REGISTER(eth_pcap_logtype,
> >> pmd.net.pcap, NOTICE);
> >>> rte_log(RTE_LOG_ ## level, eth_pcap_logtype, \
> >>> "%s(): " fmt "\n", __func__, ##args)
> >>>
> >>> +static struct queue_missed_stat*
> >>> +queue_missed_stat_update(struct rte_eth_dev *dev, unsigned int qid) {
> >>> + struct pmd_internals *internals = dev->data->dev_private;
> >>> + struct queue_missed_stat *missed_stat =
> >>> + &internals->rx_queue[qid].missed_stat;
> >>> + const struct pmd_process_private *pp = dev->process_private;
> >>> + pcap_t *pcap = pp->rx_pcap[qid];
> >>> + struct pcap_stat stat;
> >>
> >> Can you please put an empty line after variable declarations, and before
> return.
> >
> > ok
> >
> >>
> >>> + if (!pcap || (pcap_stats(pcap, &stat) != 0))
> >>> + return missed_stat;
> >>> + /* rollover check - best effort fixup assuming single rollover */
> >>> + if (stat.ps_drop < missed_stat->pcap)
> >>> + missed_stat->mnemonic += UINT_MAX;
> >>> + missed_stat->pcap = stat.ps_drop;
> >>
> >> here.
> >>
> >>> + return missed_stat;
> >>> +}
> >>> +
> >>> +static void
> >>> +queue_missed_stat_on_stop_update(struct rte_eth_dev *dev, unsigned
> >>> +int qid) {
> >>> + struct queue_missed_stat *missed_stat =
> >>> + queue_missed_stat_update(dev, qid);
> >>> + missed_stat->mnemonic += missed_stat->pcap;
> >>
> >> Better to reset 'missed_stat->pcap' afterwards, in case stats
> >> requested before port started again:
> >> missed_stat->pcap = 0;
> >
> > right, should be careful not to double count it but maybe better to
> > set it to 0 in queue_missed_stat_update in the stop return case
> > if (!pcap || (pcap_stats(pcap, &stat) != 0))
> > {
> > missed_stat->pcap = 0;
> > return missed_stat;
> > }
> > this way the missed_stat->pcap will always represent the current value
> > from pcap and not hold old value specifically in case port is stopped
> > it will be 0 and not re-added agree?
> >
>
> This also works, but there is other condition in that if block, I don't know
> when
> 'pcap_stats()' can fail, it has a risk of unexpected side affect to clear the
> stats
> randomly, I think safer to do it in 'queue_missed_stat_on_stop_update()' and I
> believe logic is more clear that way.
>
ok
> >>
> >>> +}
> >>> +
> >>> +static void
> >>> +queue_missed_stat_reset(struct rte_eth_dev *dev, unsigned int qid) {
> >>> + struct queue_missed_stat *missed_stat =
> >>> + queue_missed_stat_update(dev, qid);
> >>> + missed_stat->reset = missed_stat->pcap;
> >>
> >> I guess this should be:
> >> "missed_stat->reset = missed_stat->pcap + missed_stat->mnemonic;"
> >
> > I don't think so
> > reset should only remember where pcap was at the reset point and not store
> old values
> > trying it immediately results in
> >
> > testpmd> show port stats 0
> >
> > ######################## NIC statistics for port 0
> ########################
> > RX-packets: 0 RX-missed: 1940 RX-bytes: 0
> > RX-errors: 0
> > RX-nombuf: 0
> > TX-packets: 0 TX-errors: 0 TX-bytes: 0
> >
> > Throughput (since last show)
> > Rx-pps: 0 Rx-bps: 0
> > Tx-pps: 0 Tx-bps: 0
> >
> #################################################################
> ###########
> > testpmd> clear port stats 0
> >
> > NIC statistics for port 0 cleared
> > testpmd> show port stats 0
> >
> > ######################## NIC statistics for port 0
> ########################
> > RX-packets: 0 RX-missed: 18446744073709550646 RX-bytes: 0
> > RX-errors: 0
> > RX-nombuf: 0
> > TX-packets: 0 TX-errors: 0 TX-bytes: 0
> >
> > Throughput (since last show)
> > Rx-pps: 0 Rx-bps: 0
> > Tx-pps: 0 Tx-bps: 0
> >
> #################################################################
> ###########
> >
>
> What above shows, is it output after suggested change?
>
Yes it is **with** the suggested change (missed_stat->reset = missed_stat->pcap
+ missed_stat->mnemonic;)
Assume this sequence
port stop -> mnemonic = N, pcap = 0
reset stats -> reset = 0 + N, mnemonic = 0
show -> imissed = 0 + 0 - N
it works good w/o the suggested change (missed_stat->reset = missed_stat->pcap;)
testpmd> show port stats 0
######################## NIC statistics for port 0 ########################
RX-packets: 0 RX-missed: 3932 RX-bytes: 0
RX-errors: 0
RX-nombuf: 0
TX-packets: 0 TX-errors: 0 TX-bytes: 0
Throughput (since last show)
Rx-pps: 0 Rx-bps: 0
Tx-pps: 0 Tx-bps: 0
############################################################################
testpmd> clear port stats 0
NIC statistics for port 0 cleared
testpmd> show port stats 0
######################## NIC statistics for port 0 ########################
RX-packets: 0 RX-missed: 0 RX-bytes: 0
RX-errors: 0
RX-nombuf: 0
TX-packets: 0 TX-errors: 0 TX-bytes: 0
Throughput (since last show)
Rx-pps: 0 Rx-bps: 0
Tx-pps: 0 Tx-bps: 0
############################################################################
>
>
> missed = "(pcap + mnemonic) - reset"
> Lets assume overflow limit is 16, with original code:
> reset: 0, pcap: 2, mnemonic: 16, missed: 18
>
> clear stats:
> reset: 2, pcap: 2, mnemonic: 16, missed: 16 (wrong, it should be 0)
>
>
> OR
> reset: 0, pcap: 2, mnemonic: 0, missed: 2
>
> port stop:
> reset: 0, pcap: 0, mnemonic: 2, missed: 2
>
> port start, some traffic:
> reset: 0, pcap: 3, mnemonic: 2, missed: 5
>
> clear stats:
> reset: 3, pcap: 3, mnemonic: 2, missed: 2 (wrong, it should be 0)
>
>
> 'mnemonic' becomes part of the stats after overflow or "port stop", so I think
> it should be stored in the reset.
>
> As far as I can see suggested code change is required if I am not missing
> anything.
>
>
> >>> + missed_stat->mnemonic = 0;
> >>> +}
> >>> +
> >>> +static unsigned long
> >>> +queue_missed_stat_get(struct rte_eth_dev *dev, unsigned int qid) {
> >>> + const struct queue_missed_stat *missed_stat =
> >>> + queue_missed_stat_update(dev, qid);
> >>> + return missed_stat->pcap + missed_stat->mnemonic -
> >>> +missed_stat->reset; }
> >>> +
> >>
> >> <...>
