[ovs-dev] [PATCH] netdev-afxdp: Convert AFXDP_DEBUG to custom stats.
Ilya Maximets
i.maximets at samsung.com
Wed Jul 24 14:07:25 UTC 2019
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.
>
> 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.
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?
>
> 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 */
>> + for (i = 0; i < netdev_n_rxq(netdev); i++) {
>> + xsk_info = dev->xsks[i];
>> + optlen = sizeof stat;
>> +
>> + if (xsk_info && !getsockopt(xsk_socket__fd(xsk_info->xsk), SOL_XDP,
>> + XDP_STATISTICS, &stat, &optlen)) {
>> +#define XDP_CSTAT(NAME) \
>> + snprintf(custom_stats->counters[c].name, \
>> + NETDEV_CUSTOM_STATS_NAME_SIZE, "queue_%d_" #NAME, i); \
>> + custom_stats->counters[c++].value = stat.NAME;
>> + XDP_CSTATS;
>> +#undef XDP_CSTAT
>> + }
>> + }
>> + custom_stats->size = c;
>> + ovs_mutex_unlock(&dev->mutex);
>> +
>> + return 0;
>> +}
>> +
>> int
>> netdev_afxdp_get_stats(const struct netdev *netdev,
>> struct netdev_stats *stats)
>> diff --git a/lib/netdev-afxdp.h b/lib/netdev-afxdp.h
>> index 6a72bd9a8..e2f400b72 100644
>> --- a/lib/netdev-afxdp.h
>> +++ b/lib/netdev-afxdp.h
>> @@ -33,6 +33,7 @@ struct smap;
>> struct dp_packet;
>> struct netdev_rxq;
>> struct netdev_stats;
>> +struct netdev_custom_stats;
>>
>> int netdev_afxdp_rxq_construct(struct netdev_rxq *rxq_);
>> void netdev_afxdp_rxq_destruct(struct netdev_rxq *rxq_);
>> @@ -51,6 +52,9 @@ int netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args);
>> int netdev_afxdp_get_numa_id(const struct netdev *netdev);
>> int netdev_afxdp_get_stats(const struct netdev *netdev_,
>> struct netdev_stats *stats);
>> +int netdev_afxdp_get_custom_stats(const struct netdev *netdev,
>> + struct netdev_custom_stats *custom_stats);
>> +
>>
>> void free_afxdp_buf(struct dp_packet *p);
>> int netdev_afxdp_reconfigure(struct netdev *netdev);
>> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
>> index 2432cd176..f48192373 100644
>> --- a/lib/netdev-linux.c
>> +++ b/lib/netdev-linux.c
>> @@ -3293,6 +3293,7 @@ const struct netdev_class netdev_afxdp_class = {
>> .construct = netdev_afxdp_construct,
>> .destruct = netdev_afxdp_destruct,
>> .get_stats = netdev_afxdp_get_stats,
>> + .get_custom_stats = netdev_afxdp_get_custom_stats,
>> .get_status = netdev_linux_get_status,
>> .set_config = netdev_afxdp_set_config,
>> .get_config = netdev_afxdp_get_config,
>> --
>> 2.17.1
>>
>
>
More information about the dev
mailing list