On Wed, Jul 24, 2019 at 05:07:25PM +0300, Ilya Maximets wrote: > On 24.07.2019 1:19, William Tu wrote: > > On Tue, Jul 23, 2019 at 06:16:46PM +0300, Ilya Maximets wrote: > >> These are valid statistics of a network interface and should be > >> exposed via custom stats. > >> > >> The same MACRO trick as in vswitchd/bridge.c is used to reduce code > >> duplication and easily add new stats if necessary in the future. > >> > >> Signed-off-by: Ilya Maximets <[email protected]> > >> --- > > Thanks for the patch. > > > > I notice two things > > > > 1) ovs-vsctl list int > > shows that > > statistics : {"queue_0_rx_dropped"=0, > > "queue_0_rx_invalid_descs"=0, "queue_0_tx_invalid_descs"=0, > > rx_bytes=1230, rx_packets=15, tx_bytes=4429, tx_packets=37} > > > > There is an extra quote for afxdp counters, I don't know whether > > it's expected or not. > > Yeah. That's expected. You may see that DPDK custom stats also mostly quoted. > It's not connected with custom stats, it's the way how ovsdb output formatters > decide to quote some strings or not. > I have a patch for this, will send soon. > OK thanks. > > > > 2) Should we account and collect the "queue_N_rx_dropped" into > > standard stats (struct netdev_stats)? > > So the netdev_stas->rx_dropped += queue_0_rx_dropped + queue_1_rx_dropped > > ... > > That's a good point. However, looking at the kernel code, I see that > some drivers (veth) already includes these stats into common rx_dropped > counter. Some other drivers doesn't do that. So, it's, probably, better > to not do that in OVS to not count them twice. OK, that makes sense.
btw, I saw DPDK's AF_XDP is including these stats to imissed. (See eth_stats_get at rte_eth_af_xdp.c) Need to keep in mind when comparing these stats. > However, maybe it'll be better to be more precise in naming. > We could add 'xsk_' prefix to counters to strictly point to their source. > Like this: > xsk_queue_0_rx_dropped > xsk_queue_0_rx_invalid_descs > xsk_queue_0_tx_invalid_descs > > If it's OK for you, I could make this change before applying the patch. > What do you think? OK looks better. One minor feedback inline below > > > > > Otherwise, looks good to me. > > > > Acked-by: William Tu <[email protected]> > > > >> lib/netdev-afxdp.c | 64 ++++++++++++++++++++++++++++++++-------------- > >> lib/netdev-afxdp.h | 4 +++ > >> lib/netdev-linux.c | 1 + > >> 3 files changed, 50 insertions(+), 19 deletions(-) > >> > >> diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c > >> index 6b0b93e7f..15cd6b5ab 100644 > >> --- a/lib/netdev-afxdp.c > >> +++ b/lib/netdev-afxdp.c > >> @@ -448,21 +448,6 @@ xsk_destroy_all(struct netdev *netdev) > >> } > >> } > >> > >> -static inline void OVS_UNUSED > >> -log_xsk_stat(struct xsk_socket_info *xsk OVS_UNUSED) { > >> - struct xdp_statistics stat; > >> - socklen_t optlen; > >> - > >> - optlen = sizeof stat; > >> - ovs_assert(getsockopt(xsk_socket__fd(xsk->xsk), SOL_XDP, > >> XDP_STATISTICS, > >> - &stat, &optlen) == 0); > >> - > >> - VLOG_DBG_RL(&rl, "rx dropped %llu, rx_invalid %llu, tx_invalid %llu", > >> - stat.rx_dropped, > >> - stat.rx_invalid_descs, > >> - stat.tx_invalid_descs); > >> -} > >> - > >> int > >> netdev_afxdp_set_config(struct netdev *netdev, const struct smap *args, > >> char **errp OVS_UNUSED) > >> @@ -711,10 +696,6 @@ netdev_afxdp_rxq_recv(struct netdev_rxq *rxq_, struct > >> dp_packet_batch *batch, > >> /* TODO: return the number of remaining packets in the queue. */ > >> *qfill = 0; > >> } > >> - > >> -#ifdef AFXDP_DEBUG > >> - log_xsk_stat(xsk_info); > >> -#endif > >> return 0; > >> } > >> > >> @@ -1012,6 +993,51 @@ netdev_afxdp_destruct(struct netdev *netdev) > >> ovs_mutex_destroy(&dev->mutex); > >> } > >> > >> +int > >> +netdev_afxdp_get_custom_stats(const struct netdev *netdev, > >> + struct netdev_custom_stats *custom_stats) > >> +{ > >> + struct netdev_linux *dev = netdev_linux_cast(netdev); > >> + struct xsk_socket_info *xsk_info; > >> + struct xdp_statistics stat; > >> + uint32_t i, c = 0; > >> + socklen_t optlen; > >> + > >> + ovs_mutex_lock(&dev->mutex); > >> + > >> +#define XDP_CSTATS > >> \ > >> + XDP_CSTAT(rx_dropped) > >> \ > >> + XDP_CSTAT(rx_invalid_descs) > >> \ > >> + XDP_CSTAT(tx_invalid_descs) > >> + > >> +#define XDP_CSTAT(NAME) + 1 > >> + enum { N_XDP_CSTATS = XDP_CSTATS }; > >> +#undef XDP_CSTAT > >> + > >> + custom_stats->counters = xcalloc(netdev_n_rxq(netdev) * N_XDP_CSTATS, > >> + sizeof *custom_stats->counters); > >> + > >> + /* Account the stats for each xsk */ /* Account the stats for each xsk. */ Missing dot. Thanks William <snip> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
