RE: [PATCH v4 9/9] net/gve: add support for stats

["Guo, Junfeng" <junfeng.guo@intel.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Thursday, October 6, 2022 22:26
> To: Guo, Junfeng <junfeng.guo@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: ferruh.yigit@xilinx.com; dev@dpdk.org; Li, Xiaoyun
> <xiaoyun.li@intel.com>; awogbemila@google.com; Richardson, Bruce
> <bruce.richardson@intel.com>; Lin, Xueqin <xueqin.lin@intel.com>
> Subject: Re: [PATCH v4 9/9] net/gve: add support for stats
> 
> On 9/27/2022 8:32 AM, Junfeng Guo wrote:
> 
> >
> > Update stats add support of dev_ops stats_get/reset.
> >
> > Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> > Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> > ---
> >   doc/guides/nics/features/gve.ini |  2 +
> >   drivers/net/gve/gve_ethdev.c     | 71
> ++++++++++++++++++++++++++++++++
> >   drivers/net/gve/gve_ethdev.h     | 11 +++++
> >   drivers/net/gve/gve_rx.c         | 15 ++++++-
> >   drivers/net/gve/gve_tx.c         | 13 ++++++
> >   5 files changed, 110 insertions(+), 2 deletions(-)
> >
> > diff --git a/doc/guides/nics/features/gve.ini
> b/doc/guides/nics/features/gve.ini
> > index cdc46b08a3..180408aa80 100644
> > --- a/doc/guides/nics/features/gve.ini
> > +++ b/doc/guides/nics/features/gve.ini
> > @@ -10,6 +10,8 @@ MTU update           = Y
> >   TSO                  = Y
> >   RSS hash             = Y
> >   L4 checksum offload  = Y
> > +Basic stats          = Y
> > +Stats per queue      = Y
> 
> "stats per queue" is something else, agree that it is bad naming, please
> check features.rst file.

Sure, will check the features file and update accordingly. Thanks!

> 
> >   Linux                = Y
> >   x86-32               = Y
> >   x86-64               = Y
> > diff --git a/drivers/net/gve/gve_ethdev.c
> b/drivers/net/gve/gve_ethdev.c
> > index b9b8e51b02..cd474b8128 100644
> > --- a/drivers/net/gve/gve_ethdev.c
> > +++ b/drivers/net/gve/gve_ethdev.c
> > @@ -328,6 +328,75 @@ gve_dev_info_get(struct rte_eth_dev *dev,
> struct rte_eth_dev_info *dev_info)
> >          return 0;
> >   }
> >
> > +static int
> > +gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
> > +{
> > +       uint16_t i;
> > +
> > +       for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > +               struct gve_tx_queue *txq = dev->data->tx_queues[i];
> > +               if (txq == NULL)
> > +                       continue;
> > +
> > +               stats->opackets += txq->packets;
> > +               stats->obytes += txq->bytes;
> > +               stats->oerrors += txq->errors;
> > +
> > +               if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
> > +                       stats->q_opackets[i] = txq->packets;
> > +                       stats->q_obytes[i] = txq->bytes;
> 
> Queue stats update is moved to xstat [1], we are waiting existing PMDs
> to adopt it but for new drivers it is better to implement new method.
> 
> Can you please either drop queue stats completely, or implement it via
> xstats?
> 
> [1]
> https://elixir.bootlin.com/dpdk/v22.07/source/doc/guides/rel_notes/dep
> recation.rst#L118

Sure, thanks for reminding this!
Seems that it would be better to drop the stats feature at this point since lack
of time and bandwidth... But we can still measure the performance via other
tools like TRex. And we can plan the implementation of the new method in 
the coming release. Thanks!