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

Ilya Maximets i.maximets at samsung.com
Wed Jul 24 16:45:12 UTC 2019


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 <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!
I added the missing dot and the 'xsk_' prefix. Applied to master.

Best regards, Ilya Maximets.


More information about the dev mailing list