Re: [dpdk-dev] [PATCH v3] examples/ipsec-secgw: add per core packet stats

[Anoob Joseph <anoobj@marvell.com>]

Hi Konstantin,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Wednesday, May 13, 2020 6:13 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal <akhil.goyal@nxp.com>;
> Nicolau, Radu <radu.nicolau@intel.com>
> Cc: Narayana Prasad Raju Athreya <pathreya@marvell.com>; dev@dpdk.org
> Subject: [EXT] RE: [PATCH v3] examples/ipsec-secgw: add per core packet stats
> 
> External Email
> 
> ----------------------------------------------------------------------
> 
> > > > @@ -1099,6 +1151,10 @@ ipsec_poll_mode_worker(void)
> > > >  	const uint64_t drain_tsc = (rte_get_tsc_hz() + US_PER_S - 1)
> > > >  			/ US_PER_S * BURST_TX_DRAIN_US;
> > > >  	struct lcore_rx_queue *rxql;
> > > > +#if (STATS_INTERVAL > 0)
> > > > +	const uint64_t timer_period = STATS_INTERVAL * rte_get_timer_hz();
> > > > +	uint64_t timer_tsc = 0;
> > > > +#endif /* STATS_INTERVAL */
> > > >
> > > >  	prev_tsc = 0;
> > > >  	lcore_id = rte_lcore_id();
> > > > @@ -1159,6 +1215,19 @@ ipsec_poll_mode_worker(void)
> > > >  			drain_tx_buffers(qconf);
> > > >  			drain_crypto_buffers(qconf);
> > > >  			prev_tsc = cur_tsc;
> > > > +#if (STATS_INTERVAL > 0)
> > > > +			if (lcore_id == rte_get_master_lcore()) {
> > > > +				/* advance the timer */
> > > > +				timer_tsc += diff_tsc;
> > > > +
> > > > +				/* if timer has reached its timeout */
> > > > +				if (unlikely(timer_tsc >= timer_period)) {
> > > > +					print_stats();
> > > > +					/* reset the timer */
> > > > +					timer_tsc = 0;
> > > > +				}
> > > > +			}
> > > > +#endif /* STATS_INTERVAL */
> > >
> > > I still don't understand why to do it in data-path thread.
> > > As I said in previous comments, in DPDK there is a control thread
> > > that can be used for such house-keeping tasks.
> > > Why not to use it (via rte_alarm or so) and keep data-path threads less
> affected.
> >
> > [Anoob] From Marvell's estimates, this stats collection and reporting
> > will be expensive and so cannot be enabled by default. This is required for
> analyzing the traffic distribution in cases where the performance isn't scaling as
> expected.
> 
> Understood.
> 
>  > And this patch achieves the desired feature.
> 
> Ok, but why not to do it in control (house-keeping) thread?
> That would achieve desired goal and keep data-path unaffected.
> 
> > If Intel would like to improve the approach, that can be taken up as a separate
> patch.
> 
> This is not a vendor specific part.
> You making changes in common data-path code that is used by all ipsec-secgw
> users.
> I think it is everyone benefit (and responsibility) to keep common data-path code
> clean, tidy and fast.
> If we can avoid polluting it with extra code, I don't see a reason not to do it.

[Anoob] I cannot say I've fully understood what you have suggested. I've submitted v4 based on what I understood from your comments. Please have a look at it.
 
> 
> >
> > >
> > > >  		}
> > > >
> > > >  		for (i = 0; i < qconf->nb_rx_queue; ++i) { @@ -1169,8 +1238,10
> > > @@
> > > > ipsec_poll_mode_worker(void)
> > > >  			nb_rx = rte_eth_rx_burst(portid, queueid,
> > > >  					pkts, MAX_PKT_BURST);
> > > >
> > > > -			if (nb_rx > 0)
> > > > +			if (nb_rx > 0) {
> > > > +				core_stats_update_rx(nb_rx);
> > > >  				process_pkts(qconf, pkts, nb_rx, portid);
> > > > +			}
> > > >
> > > >  			/* dequeue and process completed crypto-ops */
> > > >  			if (is_unprotected_port(portid)) diff --git
> > > > a/examples/ipsec-secgw/ipsec-secgw.h
> > > > b/examples/ipsec-secgw/ipsec-secgw.h
> > > > index 4b53cb5..5b3561f 100644
> > > > --- a/examples/ipsec-secgw/ipsec-secgw.h
> > > > +++ b/examples/ipsec-secgw/ipsec-secgw.h
> > > > @@ -6,6 +6,8 @@
> > > >
> > > >  #include <stdbool.h>
> > > >
> > > > +#define STATS_INTERVAL 0
> > >
> > > Shouldn't it be:
> > > #ifndef STATS_INTERVAL
> > > #define STATS_INTERVAL	0
> > > #endif
> > > ?
> >
> > [Anoob] Will update in v4.
> >
> > >
> > > To allow user specify statis interval via EXTRA_CFLAGS='-
> DSTATS_INTERVAL=10'
> > > or so.
> > >
> > > > +
> > > >  #define NB_SOCKETS 4
> > > >
> > > >  #define MAX_PKT_BURST 32
> > > > @@ -69,6 +71,17 @@ struct ethaddr_info {
> > > >  	uint64_t src, dst;
> > > >  };
> > > >
> > > > +#if (STATS_INTERVAL > 0)
> > > > +struct ipsec_core_statistics {
> > > > +	uint64_t tx;
> > > > +	uint64_t rx;
> > > > +	uint64_t dropped;
> > > > +	uint64_t burst_rx;
> > > > +} __rte_cache_aligned;
> > > > +
> > > > +struct ipsec_core_statistics core_statistics[RTE_MAX_LCORE];
> > > > +#endif
> > > > +/* STATS_INTERVAL */
> > > > +
> > > >  extern struct ethaddr_info ethaddr_tbl[RTE_MAX_ETHPORTS];
> > > >
> > > >  /* Port mask to identify the unprotected ports */ @@ -85,4 +98,59
> > > > @@ is_unprotected_port(uint16_t port_id)
> > > >  	return unprotected_port_mask & (1 << port_id);  }
> > > >
> > > > +static inline void
> > > > +core_stats_update_rx(int n)
> > > > +{
> > > > +#if (STATS_INTERVAL > 0)
> > > > +	int lcore_id = rte_lcore_id();
> > > > +	core_statistics[lcore_id].rx += n;
> > > > +	if (n == MAX_PKT_BURST)
> > > > +		core_statistics[lcore_id].burst_rx += n; #else
> > > > +	RTE_SET_USED(n);
> > > > +#endif /* STATS_INTERVAL */
> > > > +}
> > > > +
> > > > +static inline void
> > > > +core_stats_update_tx(int n)
> > > > +{
> > > > +#if (STATS_INTERVAL > 0)
> > > > +	int lcore_id = rte_lcore_id();
> > > > +	core_statistics[lcore_id].tx += n; #else
> > > > +	RTE_SET_USED(n);
> > > > +#endif /* STATS_INTERVAL */
> > > > +}
> > > > +
> > > > +static inline void
> > > > +core_stats_update_drop(int n)
> > > > +{
> > > > +#if (STATS_INTERVAL > 0)
> > > > +	int lcore_id = rte_lcore_id();
> > > > +	core_statistics[lcore_id].dropped += n; #else
> > > > +	RTE_SET_USED(n);
> > > > +#endif /* STATS_INTERVAL */
> > > > +}
> > > > +
> > > > +/* helper routine to free bulk of packets */ static inline void
> > > > +free_pkts(struct rte_mbuf *mb[], uint32_t n) {
> > > > +	uint32_t i;
> > > > +
> > > > +	for (i = 0; i != n; i++)
> > > > +		rte_pktmbuf_free(mb[i]);
> > > > +
> > > > +	core_stats_update_drop(n);
> > > > +}
> > > > +
> > > > +/* helper routine to free single packet */ static inline void
> > > > +free_pkt(struct rte_mbuf *mb) {
> > > > +	rte_pktmbuf_free(mb);
> > > > +	core_stats_update_drop(1);
> > >
> > > Probably just:
> > > free_pkts(&mb, 1);
> > > ?
> >
> > [Anoob] Will update in v4.
> >
> > >
> > > > +}
> > > > +
> > > >  #endif /* _IPSEC_SECGW_H_ */