[v2] net/pcap: imissed stats support
Checks
Commit Message
Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
---
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 | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
Comments
On 2/1/2021 8:30 AM, Ido Goshen wrote:
> Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
> ---
> 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 | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index a32b1f3f3..18c59d61c 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -58,6 +58,8 @@ struct queue_stat {
> volatile unsigned long pkts;
> volatile unsigned long bytes;
> volatile unsigned long err_pkts;
> + volatile unsigned long missed_reset;
> + volatile unsigned long missed_mnemonic;
Can you please put some comments why 'missed_mnemonic' is required?
<...>
> @@ -695,6 +715,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_pcap_stats_missed_get(dev, i) +
> + internal->rx_queue[i].rx_stat.missed_mnemonic -
> + internal->rx_queue[i].rx_stat.missed_reset;
Instead of including the 'missed_mnemonic' to the regular calculation, what do
you think to save the 'imissed' value to 'missed_mnemonic' in 'port_stop' and
load it back in the 'eth_dev_start'?
This balanced usage can simplify the code I think.
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, 1 February 2021 13:49
> To: Ido Goshen <Ido@cgstowernetworks.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2] net/pcap: imissed stats support
>
> On 2/1/2021 8:30 AM, Ido Goshen wrote:
> > Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
> > ---
> > 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 | 28
> ++++++++++++++++++++++++++++
> > 1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/net/pcap/rte_eth_pcap.c
> > b/drivers/net/pcap/rte_eth_pcap.c index a32b1f3f3..18c59d61c 100644
> > --- a/drivers/net/pcap/rte_eth_pcap.c
> > +++ b/drivers/net/pcap/rte_eth_pcap.c
> > @@ -58,6 +58,8 @@ struct queue_stat {
> > volatile unsigned long pkts;
> > volatile unsigned long bytes;
> > volatile unsigned long err_pkts;
> > + volatile unsigned long missed_reset;
> > + volatile unsigned long missed_mnemonic;
>
> Can you please put some comments why 'missed_mnemonic' is required?
>
ok
> <...>
>
> > @@ -695,6 +715,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_pcap_stats_missed_get(dev,
> i) +
> > + internal-
> >rx_queue[i].rx_stat.missed_mnemonic -
> > + internal->rx_queue[i].rx_stat.missed_reset;
>
>
> Instead of including the 'missed_mnemonic' to the regular calculation, what
> do you think to save the 'imissed' value to 'missed_mnemonic' in 'port_stop'
> and load it back in the 'eth_dev_start'?
> This balanced usage can simplify the code I think.
Not sure I get the request, isn't it what the patch already doing?
Value is already stored in 'eth_dev_stop' and added back when needed.
What do you mean by load it back on 'eth_dev_start' - where to load it to?
Please explain further
On 2/1/2021 2:02 PM, Ido Goshen wrote:
>
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Monday, 1 February 2021 13:49
>> To: Ido Goshen <Ido@cgstowernetworks.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v2] net/pcap: imissed stats support
>>
>> On 2/1/2021 8:30 AM, Ido Goshen wrote:
>>> Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
>>> ---
>>> 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 | 28
>> ++++++++++++++++++++++++++++
>>> 1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/net/pcap/rte_eth_pcap.c
>>> b/drivers/net/pcap/rte_eth_pcap.c index a32b1f3f3..18c59d61c 100644
>>> --- a/drivers/net/pcap/rte_eth_pcap.c
>>> +++ b/drivers/net/pcap/rte_eth_pcap.c
>>> @@ -58,6 +58,8 @@ struct queue_stat {
>>> volatile unsigned long pkts;
>>> volatile unsigned long bytes;
>>> volatile unsigned long err_pkts;
>>> + volatile unsigned long missed_reset;
>>> + volatile unsigned long missed_mnemonic;
>>
>> Can you please put some comments why 'missed_mnemonic' is required?
>>
> ok
>
>> <...>
>>
>>> @@ -695,6 +715,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_pcap_stats_missed_get(dev,
>> i) +
>>> + internal-
>>> rx_queue[i].rx_stat.missed_mnemonic -
>>> + internal->rx_queue[i].rx_stat.missed_reset;
>>
>>
>> Instead of including the 'missed_mnemonic' to the regular calculation, what
>> do you think to save the 'imissed' value to 'missed_mnemonic' in 'port_stop'
>> and load it back in the 'eth_dev_start'?
>> This balanced usage can simplify the code I think.
>
> Not sure I get the request, isn't it what the patch already doing?
> Value is already stored in 'eth_dev_stop' and added back when needed.
> What do you mean by load it back on 'eth_dev_start' - where to load it to?
> Please explain further
>
Please ignore above comment, it was wrong, the intention was to not complicate
the stats function with 'missed_mnemonic' & 'missed_reset' details,
for that what do you think to move the 'missed_mnemonic' & 'missed_reset' usage
into the 'eth_pcap_stats_missed_get()'?
This also fixes imissed stats after multiple 'port_stop'.
Also a 'eth_pcap_stats_missed_reset()' can be added for same reason.
@@ -58,6 +58,8 @@ struct queue_stat {
volatile unsigned long pkts;
volatile unsigned long bytes;
volatile unsigned long err_pkts;
+ volatile unsigned long missed_reset;
+ volatile unsigned long missed_mnemonic;
};
struct pcap_rx_queue {
@@ -144,6 +146,19 @@ RTE_LOG_REGISTER(eth_pcap_logtype, pmd.net.pcap, NOTICE);
rte_log(RTE_LOG_ ## level, eth_pcap_logtype, \
"%s(): " fmt "\n", __func__, ##args)
+static unsigned long
+eth_pcap_stats_missed_get(struct rte_eth_dev *dev, unsigned int qid)
+{
+ const struct pmd_process_private *pp = dev->process_private;
+ pcap_t *pcap = pp->rx_pcap[qid];
+ if (!pcap)
+ return 0;
+ struct pcap_stat stat;
+ if (pcap_stats(pcap, &stat) != 0)
+ return 0;
+ return stat.ps_drop;
+}
+
static int
eth_pcap_rx_jumbo(struct rte_mempool *mb_pool, struct rte_mbuf *mbuf,
const u_char *data, uint16_t data_len)
@@ -621,6 +636,8 @@ eth_dev_stop(struct rte_eth_dev *dev)
/* Special iface case. Single pcap is open and shared between tx/rx. */
if (internals->single_iface) {
+ internals->rx_queue[0].rx_stat.missed_mnemonic =
+ eth_pcap_stats_missed_get(dev, 0);
pcap_close(pp->tx_pcap[0]);
pp->tx_pcap[0] = NULL;
pp->rx_pcap[0] = NULL;
@@ -641,6 +658,8 @@ eth_dev_stop(struct rte_eth_dev *dev)
for (i = 0; i < dev->data->nb_rx_queues; i++) {
if (pp->rx_pcap[i] != NULL) {
+ internals->rx_queue[i].rx_stat.missed_mnemonic =
+ eth_pcap_stats_missed_get(dev, i);
pcap_close(pp->rx_pcap[i]);
pp->rx_pcap[i] = NULL;
}
@@ -685,6 +704,7 @@ 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 +715,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_pcap_stats_missed_get(dev, i) +
+ internal->rx_queue[i].rx_stat.missed_mnemonic -
+ internal->rx_queue[i].rx_stat.missed_reset;
+ rx_missed_total += rx_missed;
}
for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS &&
@@ -708,6 +732,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 +749,9 @@ 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_pcap_stats_missed_get(dev, i);
+ internal->rx_queue[i].rx_stat.missed_mnemonic = 0;
}
for (i = 0; i < dev->data->nb_tx_queues; i++) {