Re: [dpdk-dev] [PATCH 5/5] librte_ethdev: add to use apistats

["Morten Brørup" <mb@smartsharesystems.com>]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Hideyuki Yamashita
> Sent: Tuesday, December 22, 2020 3:50 AM
> 
> Hello,
> 
> Thanks for your comments.
> Please see my comments inline tagged with [HY].
> 
> > > diff --git a/lib/librte_ethdev/rte_ethdev.h
> b/lib/librte_ethdev/rte_ethdev.h
> > > index f5f8919..bef9bc6 100644
> > > --- a/lib/librte_ethdev/rte_ethdev.h
> > > +++ b/lib/librte_ethdev/rte_ethdev.h
> > > @@ -160,6 +160,7 @@ extern "C" {
> > >
> > >  #include "rte_ethdev_trace_fp.h"
> > >  #include "rte_dev_info.h"
> > > +#include <rte_apistats.h>
> > >
> > >  extern int rte_eth_dev_logtype;
> > >
> > > @@ -4849,6 +4850,9 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t
> queue_id,
> > >  	nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
> > >  				     rx_pkts, nb_pkts);
> > >
> > > +	int lcore_id = rte_lcore_id();
> > > +	rte_apicounts->rx_burst_counts[lcore_id]++;

[...]

> > As a generic one: such sort of statistics can be easily collected by the
> app itself.
> > Either by just incrementing counters before rx/tx_burst function call
> directly or
> > via rx/tx callbacks.
> > So I don't see any point to push it inside ethdev layer.
> > Konstantin
> [HY]
> You are correct.
> Application can do it.
> But if applications want to do it, then every applicaiton needs
> to do it.
> The reason why I propose my patchset is to provide such
> common function (count invocation of rx_burst/tx_burst)
> so that application only needs to "retrieve the information".
> 
> I think it is benefitical than all application  do similar thing.
> Your feedback is highly appreciated.

For performance reasons, I am strongly opposed to adding anything into the ethdev rx/tx functions, unless it is absolutely critical for a large user base.

I get your argument about avoiding additional application code by doing it directly in the ethdev rx/tx functions - this is the benefit that this library adds to DPDK. So as a compromise, I suggest surrounding the added code in these functions by #ifdef/#endif, so there is no performance degradation if the library is not used.

Personally, I would prefer a much more advanced library for measuring CPU load and RX/TX usage. However, I can also imagine that simple DPDK applications would benefit from this library, because is easy to understand and requires nearly no effort to use.

> 
> Thanks!
> BR,
> Hideyuki Yamashita
> NTT TechnoCross
> 
> > >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> > >  	struct rte_eth_rxtx_callback *cb;
> > >
> > > @@ -5124,6 +5128,9 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t
> queue_id,
> > >  	}
> > >  #endif
> > >
> > > +	int lcore_id = rte_lcore_id();
> > > +	rte_apicounts->tx_burst_counts[lcore_id]++;
> > >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> > >  	struct rte_eth_rxtx_callback *cb;
> > >