On 24.07.2019 17:57, William Tu wrote: > 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! I added the missing dot and the 'xsk_' prefix. Applied to master. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
