[ovs-dev] [PATCH] netdev-afxdp: Convert AFXDP_DEBUG to custom stats.

William Tu u9012063 at gmail.com
Wed Jul 24 14:57:22 UTC 2019


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 <i.maximets at samsung.com>
> >> ---
> > 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 <u9012063 at gmail.com>
> > 
> >>  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>


More information about the dev mailing list