[ovs-dev] [PATCH v2 1/2] netdev-dpdk: Fix not reporting rx_oversize_errors in stats.

Stokes, Ian ian.stokes at intel.com
Fri Aug 23 14:05:45 UTC 2019


On 8/23/2019 2:58 PM, Ilya Maximets wrote:
> On 23.08.2019 16:20, Stokes, Ian wrote:
>> On 8/19/2019 12:18 PM, Ilya Maximets wrote:
>>> There is a big code duplication issue with DPDK xstats that led to
>>> missed "rx_oversize_errors" statistics. It's defined but not used.
>>> Fix that by actually using this stat along with code refactoring that
>>> will allow us to not make same mistakes in the future.
>>> Macro definitions are perfectly suitable to automate code generation
>>> in such cases and already used in a couple of places in OVS for similar
>>> purposes.
>>
>> LGTM, one minor query below.
>>
>>
>>> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
>>> ---
>>>    lib/netdev-dpdk.c | 102 +++++++++++++++-------------------------------
>>>    1 file changed, 32 insertions(+), 70 deletions(-)
>>>
>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>> index 48057835f..52ecf7576 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -110,34 +110,6 @@ BUILD_ASSERT_DECL(MAX_NB_MBUF % ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)
>>>    BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF))
>>>                      % MP_CACHE_SZ == 0);
>>>    -/*
>>> - * DPDK XSTATS Counter names definition
>>> - */
>>> -#define XSTAT_RX_64_PACKETS              "rx_size_64_packets"
>>> -#define XSTAT_RX_65_TO_127_PACKETS       "rx_size_65_to_127_packets"
>>> -#define XSTAT_RX_128_TO_255_PACKETS      "rx_size_128_to_255_packets"
>>> -#define XSTAT_RX_256_TO_511_PACKETS      "rx_size_256_to_511_packets"
>>> -#define XSTAT_RX_512_TO_1023_PACKETS     "rx_size_512_to_1023_packets"
>>> -#define XSTAT_RX_1024_TO_1522_PACKETS    "rx_size_1024_to_1522_packets"
>>> -#define XSTAT_RX_1523_TO_MAX_PACKETS     "rx_size_1523_to_max_packets"
>>> -
>>> -#define XSTAT_TX_64_PACKETS              "tx_size_64_packets"
>>> -#define XSTAT_TX_65_TO_127_PACKETS       "tx_size_65_to_127_packets"
>>> -#define XSTAT_TX_128_TO_255_PACKETS      "tx_size_128_to_255_packets"
>>> -#define XSTAT_TX_256_TO_511_PACKETS      "tx_size_256_to_511_packets"
>>> -#define XSTAT_TX_512_TO_1023_PACKETS     "tx_size_512_to_1023_packets"
>>> -#define XSTAT_TX_1024_TO_1522_PACKETS    "tx_size_1024_to_1522_packets"
>>> -#define XSTAT_TX_1523_TO_MAX_PACKETS     "tx_size_1523_to_max_packets"
>>> -
>>> -#define XSTAT_RX_MULTICAST_PACKETS       "rx_multicast_packets"
>>> -#define XSTAT_TX_MULTICAST_PACKETS       "tx_multicast_packets"
>>> -#define XSTAT_RX_BROADCAST_PACKETS       "rx_broadcast_packets"
>>> -#define XSTAT_TX_BROADCAST_PACKETS       "tx_broadcast_packets"
>>> -#define XSTAT_RX_UNDERSIZED_ERRORS       "rx_undersized_errors"
>>> -#define XSTAT_RX_OVERSIZE_ERRORS         "rx_oversize_errors"
>>> -#define XSTAT_RX_FRAGMENTED_ERRORS       "rx_fragmented_errors"
>>> -#define XSTAT_RX_JABBER_ERRORS           "rx_jabber_errors"
>>> -
>>>    /* Size of vHost custom stats. */
>>>    #define VHOST_CUSTOM_STATS_SIZE          1
>>>    @@ -2682,51 +2654,41 @@ netdev_dpdk_convert_xstats(struct netdev_stats *stats,
>>>                               const struct rte_eth_xstat_name *names,
>>>                               const unsigned int size)
>>>    {
>>> +/* DPDK XSTATS Counter names definition. */
>>> +#define DPDK_XSTATS \
>>> +    DPDK_XSTAT(multicast,               "rx_multicast_packets"            ) \
>> Should above be rx_multicast_packets to keep with the rx/tx naming convention with the rest of the stats?
> It's a field of 'struct netdev_stats', we can't change it.
> At least, not in this patch.


Ah, gotcha, just looked a bit funny at first glance, not sure why it 
wasn't named like that in the first place.

In that case Acked.

Ian

>
> Best regards, Ilya Maximets.
>
>>
>> Ian
>>
>>> +    DPDK_XSTAT(tx_multicast_packets,    "tx_multicast_packets"            ) \
>>> +    DPDK_XSTAT(rx_broadcast_packets,    "rx_broadcast_packets"            ) \
>>> +    DPDK_XSTAT(tx_broadcast_packets,    "tx_broadcast_packets"            ) \
>>> +    DPDK_XSTAT(rx_undersized_errors,    "rx_undersized_errors"            ) \
>>> +    DPDK_XSTAT(rx_oversize_errors,      "rx_oversize_errors"              ) \
>>> +    DPDK_XSTAT(rx_fragmented_errors,    "rx_fragmented_errors"            ) \
>>> +    DPDK_XSTAT(rx_jabber_errors,        "rx_jabber_errors"                ) \
>>> +    DPDK_XSTAT(rx_1_to_64_packets,      "rx_size_64_packets"              ) \
>>> +    DPDK_XSTAT(rx_65_to_127_packets,    "rx_size_65_to_127_packets"       ) \
>>> +    DPDK_XSTAT(rx_128_to_255_packets,   "rx_size_128_to_255_packets"      ) \
>>> +    DPDK_XSTAT(rx_256_to_511_packets,   "rx_size_256_to_511_packets"      ) \
>>> +    DPDK_XSTAT(rx_512_to_1023_packets,  "rx_size_512_to_1023_packets"     ) \
>>> +    DPDK_XSTAT(rx_1024_to_1522_packets, "rx_size_1024_to_1522_packets"    ) \
>>> +    DPDK_XSTAT(rx_1523_to_max_packets,  "rx_size_1523_to_max_packets"     ) \
>>> +    DPDK_XSTAT(tx_1_to_64_packets,      "tx_size_64_packets"              ) \
>>> +    DPDK_XSTAT(tx_65_to_127_packets,    "tx_size_65_to_127_packets"       ) \
>>> +    DPDK_XSTAT(tx_128_to_255_packets,   "tx_size_128_to_255_packets"      ) \
>>> +    DPDK_XSTAT(tx_256_to_511_packets,   "tx_size_256_to_511_packets"      ) \
>>> +    DPDK_XSTAT(tx_512_to_1023_packets,  "tx_size_512_to_1023_packets"     ) \
>>> +    DPDK_XSTAT(tx_1024_to_1522_packets, "tx_size_1024_to_1522_packets"    ) \
>>> +    DPDK_XSTAT(tx_1523_to_max_packets,  "tx_size_1523_to_max_packets"     )
>>> +
>>>        for (unsigned int i = 0; i < size; i++) {
>>> -        if (strcmp(XSTAT_RX_64_PACKETS, names[i].name) == 0) {
>>> -            stats->rx_1_to_64_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_RX_65_TO_127_PACKETS, names[i].name) == 0) {
>>> -            stats->rx_65_to_127_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_RX_128_TO_255_PACKETS, names[i].name) == 0) {
>>> -            stats->rx_128_to_255_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_RX_256_TO_511_PACKETS, names[i].name) == 0) {
>>> -            stats->rx_256_to_511_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_RX_512_TO_1023_PACKETS, names[i].name) == 0) {
>>> -            stats->rx_512_to_1023_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_RX_1024_TO_1522_PACKETS, names[i].name) == 0) {
>>> -            stats->rx_1024_to_1522_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_RX_1523_TO_MAX_PACKETS, names[i].name) == 0) {
>>> -            stats->rx_1523_to_max_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_TX_64_PACKETS, names[i].name) == 0) {
>>> -            stats->tx_1_to_64_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_TX_65_TO_127_PACKETS, names[i].name) == 0) {
>>> -            stats->tx_65_to_127_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_TX_128_TO_255_PACKETS, names[i].name) == 0) {
>>> -            stats->tx_128_to_255_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_TX_256_TO_511_PACKETS, names[i].name) == 0) {
>>> -            stats->tx_256_to_511_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_TX_512_TO_1023_PACKETS, names[i].name) == 0) {
>>> -            stats->tx_512_to_1023_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_TX_1024_TO_1522_PACKETS, names[i].name) == 0) {
>>> -            stats->tx_1024_to_1522_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_TX_1523_TO_MAX_PACKETS, names[i].name) == 0) {
>>> -            stats->tx_1523_to_max_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_RX_MULTICAST_PACKETS, names[i].name) == 0) {
>>> -            stats->multicast = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_TX_MULTICAST_PACKETS, names[i].name) == 0) {
>>> -            stats->tx_multicast_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_RX_BROADCAST_PACKETS, names[i].name) == 0) {
>>> -            stats->rx_broadcast_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_TX_BROADCAST_PACKETS, names[i].name) == 0) {
>>> -            stats->tx_broadcast_packets = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_RX_UNDERSIZED_ERRORS, names[i].name) == 0) {
>>> -            stats->rx_undersized_errors = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_RX_FRAGMENTED_ERRORS, names[i].name) == 0) {
>>> -            stats->rx_fragmented_errors = xstats[i].value;
>>> -        } else if (strcmp(XSTAT_RX_JABBER_ERRORS, names[i].name) == 0) {
>>> -            stats->rx_jabber_errors = xstats[i].value;
>>> +#define DPDK_XSTAT(MEMBER, NAME) \
>>> +        if (strcmp(NAME, names[i].name) == 0) {   \
>>> +            stats->MEMBER = xstats[i].value;      \
>>> +            continue;                             \
>>>            }
>>> +        DPDK_XSTATS;
>>> +#undef DPDK_XSTAT
>>>        }
>>> +#undef DPDK_XSTATS
>>>    }
>>>      static int
>>


More information about the dev mailing list