| Message ID | 20211015150406.12949-1-david.marchand@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,RFC] netdev-dpdk: Expose per rxq/txq basic statistics. | expand |
Hi, On 10/15/21 17:04, David Marchand wrote: > When troubleshooting multiqueue setups, having per queue statistics helps > checking packets repartition in rx and tx queues. > > Per queue statistics are exported by most DPDK drivers (with capability > RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS). But since OVS only filters statistics > it exposes, there is nothing to request in DPDK API. > > Extend existing filter with a regular expression. > string_ends_with() helper is not used anymore, and removed as a > consequence. > > Querying statistics with > $ ovs-vsctl get interface dpdk0 statistics | \ > sed -e 's#[{}]##g' -e 's#, #\n#g' > > and comparing gives: > @@ -13,7 +13,12 @@ > rx_phy_crc_errors=0 > rx_phy_in_range_len_errors=0 > rx_phy_symbol_errors=0 > +rx_q0_bytes=0 > rx_q0_errors=0 > +rx_q0_packets=0 > +rx_q1_bytes=0 > +rx_q1_errors=0 > +rx_q1_packets=0 > rx_wqe_errors=0 > tx_broadcast_packets=0 > tx_bytes=0 > @@ -27,3 +32,13 @@ > tx_pp_rearm_queue_errors=0 > tx_pp_timestamp_future_errors=0 > tx_pp_timestamp_past_errors=0 > +tx_q0_bytes=0 > +tx_q0_packets=0 > +tx_q1_bytes=0 > +tx_q1_packets=0 > +tx_q2_bytes=0 > +tx_q2_packets=0 > +tx_q3_bytes=0 > +tx_q3_packets=0 > +tx_q4_bytes=0 > +tx_q4_packets=0 > > Signed-off-by: David Marchand <david.marchand@redhat.com> > --- > Sending this as a RFC for feedback. I think this is a very good idea. > vhost user ports are left untouched for now, but could be added in the > future. I actually need it for Vhost-user ports for a feature I'm working on. Do you want me to implement it for Vhost-user ports on top of your patch, or you prefer to support both in a v1? > --- > lib/netdev-dpdk.c | 32 ++++++++++++++++++++++++++------ > lib/util.c | 13 ------------- > lib/util.h | 2 -- > 3 files changed, 26 insertions(+), 21 deletions(-) > For this part: Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com> Thanks, Maxime
On 15/10/2021 16:04, David Marchand wrote: > When troubleshooting multiqueue setups, having per queue statistics helps > checking packets repartition in rx and tx queues. > > Per queue statistics are exported by most DPDK drivers (with capability > RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS). But since OVS only filters statistics > it exposes, there is nothing to request in DPDK API. > > Extend existing filter with a regular expression. > string_ends_with() helper is not used anymore, and removed as a > consequence. > > Querying statistics with > $ ovs-vsctl get interface dpdk0 statistics | \ > sed -e 's#[{}]##g' -e 's#, #\n#g' > > and comparing gives: > @@ -13,7 +13,12 @@ > rx_phy_crc_errors=0 > rx_phy_in_range_len_errors=0 > rx_phy_symbol_errors=0 > +rx_q0_bytes=0 > rx_q0_errors=0 > +rx_q0_packets=0 > +rx_q1_bytes=0 > +rx_q1_errors=0 > +rx_q1_packets=0 > rx_wqe_errors=0 > tx_broadcast_packets=0 > tx_bytes=0 > @@ -27,3 +32,13 @@ > tx_pp_rearm_queue_errors=0 > tx_pp_timestamp_future_errors=0 > tx_pp_timestamp_past_errors=0 > +tx_q0_bytes=0 > +tx_q0_packets=0 > +tx_q1_bytes=0 > +tx_q1_packets=0 > +tx_q2_bytes=0 > +tx_q2_packets=0 > +tx_q3_bytes=0 > +tx_q3_packets=0 > +tx_q4_bytes=0 > +tx_q4_packets=0 > > Signed-off-by: David Marchand <david.marchand@redhat.com> The patch looks good but I tested with with ixgbe, and seeing something I am unsure of. I changed the number of rxqs to 11 but I am only seeing stats for q0. i'm not sure if this because there was no traffic yet (even though there is also no traffic on q0). i will set up a traffic gen and see what happens when the q1-q10 receives traffic. One other comment below. # /root/ovs/utilities/ovs-appctl dpif-netdev/pmd-rxq-show pmd thread numa_id 0 core_id 8: isolated : false port: myport queue-id: 0 (enabled) pmd usage: 0 % port: myport queue-id: 3 (enabled) pmd usage: 0 % port: myport queue-id: 4 (enabled) pmd usage: 0 % port: myport queue-id: 7 (enabled) pmd usage: 0 % port: myport queue-id: 8 (enabled) pmd usage: 0 % port: urport queue-id: 0 (enabled) pmd usage: 0 % overhead: 0 % pmd thread numa_id 0 core_id 10: isolated : false port: myport queue-id: 1 (enabled) pmd usage: 0 % port: myport queue-id: 2 (enabled) pmd usage: 0 % port: myport queue-id: 5 (enabled) pmd usage: 0 % port: myport queue-id: 6 (enabled) pmd usage: 0 % port: myport queue-id: 9 (enabled) pmd usage: 0 % port: myport queue-id: 10 (enabled) pmd usage: 0 % overhead: 0 % # /root/ovs/utilities/ovs-vsctl get Interface myport statistics {flow_director_filter_add_errors=0, flow_director_filter_remove_errors=0, mac_local_errors=0, mac_remote_errors=0, ovs_rx_qos_drops=0, ovs_tx_failure_drops=0, ovs_tx_invalid_hwol_drops=0, ovs_tx_mtu_exceeded_drops=0, ovs_tx_qos_drops=0, rx_128_to_255_packets=0, rx_1_to_64_packets=0, rx_256_to_511_packets=0, rx_512_to_1023_packets=0, rx_65_to_127_packets=0, rx_broadcast_packets=0, rx_bytes=0, rx_crc_errors=0, rx_dropped=0, rx_errors=0, rx_fcoe_crc_errors=0, rx_fcoe_dropped=0, rx_fcoe_mbuf_allocation_errors=0, rx_fragment_errors=0, rx_illegal_byte_errors=0, rx_jabber_errors=0, rx_length_errors=0, rx_mac_short_packet_dropped=0, rx_management_dropped=0, rx_management_packets=0, rx_mbuf_allocation_errors=0, rx_missed_errors=0, rx_oversize_errors=0, rx_packets=0, rx_priority0_dropped=0, rx_priority0_mbuf_allocation_errors=0, rx_priority1_dropped=0, rx_priority1_mbuf_allocation_errors=0, rx_priority2_dropped=0, rx_priority2_mbuf_allocation_errors=0, rx_priority3_dropped=0, rx_priority3_mbuf_allocation_errors=0, rx_priority4_dropped=0, rx_priority4_mbuf_allocation_errors=0, rx_priority5_dropped=0, rx_priority5_mbuf_allocation_errors=0, rx_priority6_dropped=0, rx_priority6_mbuf_allocation_errors=0, rx_priority7_dropped=0, rx_priority7_mbuf_allocation_errors=0, rx_q0_bytes=0, rx_q0_errors=0, rx_q0_packets=0, rx_undersize_errors=0, tx_128_to_255_packets=0, tx_1_to_64_packets=0, tx_256_to_511_packets=0, tx_512_to_1023_packets=0, tx_65_to_127_packets=0, tx_broadcast_packets=0, tx_bytes=0, tx_dropped=0, tx_errors=0, tx_management_packets=0, tx_multicast_packets=0, tx_packets=0, tx_q0_bytes=0, tx_q0_packets=0, tx_q1_bytes=0, tx_q1_packets=0, tx_q2_bytes=0, tx_q2_packets=0} > --- > Sending this as a RFC for feedback. > vhost user ports are left untouched for now, but could be added in the > future. > > --- > lib/netdev-dpdk.c | 32 ++++++++++++++++++++++++++------ > lib/util.c | 13 ------------- > lib/util.h | 2 -- > 3 files changed, 26 insertions(+), 21 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index ca92c947a2..12447cc4b2 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -18,6 +18,7 @@ > #include "netdev-dpdk.h" > > #include <errno.h> > +#include <regex.h> > #include <signal.h> > #include <stdlib.h> > #include <string.h> > @@ -1583,6 +1584,13 @@ netdev_dpdk_get_xstat_name(struct netdev_dpdk *dev, uint64_t id) > return dev->rte_xstats_names[id].name; > } > > +/* We filter out everything except per rxq/txq basic stats, and dropped, > + * error and management counters. > + * Note: rx_qX_errors is handled by the _errors$ pattern. > + */ > +#define DPDK_STATS_REGEX_FILTER \ > + "(^(rx_q|tx_q)[0-9]*_(packets|bytes)$|_errors$|_dropped$|_management_)" > + iirc, there was some pmds that were not conforming correctly with this rx_q syntax, but I *think* they were fixed a few releases ago in DPDK. Just mentioning because if there was still some, they could be fixed or the regex could be expanded to capture them. > static bool > netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) > OVS_REQUIRES(dev->mutex) > @@ -1617,6 +1625,20 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) > sizeof *dev->rte_xstats_names); > > if (dev->rte_xstats_names) { > + int err; > + regex_t r; > + > + err = regcomp(&r, DPDK_STATS_REGEX_FILTER, REG_EXTENDED); > + if (err != 0) { > + size_t errbuf_size = regerror(err, &r, NULL, 0); > + char *errbuf = xcalloc(1, errbuf_size); > + > + regerror(err, &r, errbuf, errbuf_size); > + VLOG_DBG("Could not setup stats regexp: %s", errbuf); > + free(errbuf); > + goto out; > + } > + > /* Retreive xstats names */ > rte_xstats_len = > rte_eth_xstats_get_names(dev->port_id, > @@ -1626,10 +1648,12 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) > if (rte_xstats_len < 0) { > VLOG_WARN("Cannot get XSTATS names for port: " > DPDK_PORT_ID_FMT, dev->port_id); > + regfree(&r); > goto out; > } else if (rte_xstats_len != dev->rte_xstats_names_size) { > VLOG_WARN("XSTATS size doesn't match for port: " > DPDK_PORT_ID_FMT, dev->port_id); > + regfree(&r); > goto out; > } > > @@ -1648,12 +1672,7 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) > for (uint32_t i = 0; i < rte_xstats_len; i++) { > id = rte_xstats[i].id; > name = netdev_dpdk_get_xstat_name(dev, id); > - /* We need to filter out everything except > - * dropped, error and management counters */ > - if (string_ends_with(name, "_errors") || > - strstr(name, "_management_") || > - string_ends_with(name, "_dropped")) { > - > + if (regexec(&r, name, 0, NULL, 0) == 0) { > dev->rte_xstats_ids[xstats_no] = id; > xstats_no++; > } > @@ -1666,6 +1685,7 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) > } > > free(rte_xstats); > + regfree(&r); > } > } > } else { > diff --git a/lib/util.c b/lib/util.c > index 1195c79821..34755a4dad 100644 > --- a/lib/util.c > +++ b/lib/util.c > @@ -405,19 +405,6 @@ ovs_strzcpy(char *dst, const char *src, size_t size) > } > } > > -/* > - * Returns true if 'str' ends with given 'suffix'. > - */ > -int > -string_ends_with(const char *str, const char *suffix) > -{ > - int str_len = strlen(str); > - int suffix_len = strlen(suffix); > - > - return (str_len >= suffix_len) && > - (0 == strcmp(str + (str_len - suffix_len), suffix)); > -} > - > /* Prints 'format' on stderr, formatting it like printf() does. If 'err_no' is > * nonzero, then it is formatted with ovs_retval_to_string() and appended to > * the message inside parentheses. Then, terminates with abort(). > diff --git a/lib/util.h b/lib/util.h > index aea19d45fa..4ad6e28274 100644 > --- a/lib/util.h > +++ b/lib/util.h > @@ -183,8 +183,6 @@ void free_cacheline(void *); > void ovs_strlcpy(char *dst, const char *src, size_t size); > void ovs_strzcpy(char *dst, const char *src, size_t size); > > -int string_ends_with(const char *str, const char *suffix); > - > void *xmalloc_pagealign(size_t) MALLOC_LIKE; > void free_pagealign(void *); > void *xmalloc_size_align(size_t, size_t) MALLOC_LIKE; >
Hi Kevin, David, On 11/18/21 16:31, Kevin Traynor wrote: > On 15/10/2021 16:04, David Marchand wrote: >> When troubleshooting multiqueue setups, having per queue statistics helps >> checking packets repartition in rx and tx queues. >> >> Per queue statistics are exported by most DPDK drivers (with capability >> RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS). But since OVS only filters statistics >> it exposes, there is nothing to request in DPDK API. >> >> Extend existing filter with a regular expression. >> string_ends_with() helper is not used anymore, and removed as a >> consequence. >> >> Querying statistics with >> $ ovs-vsctl get interface dpdk0 statistics | \ >> sed -e 's#[{}]##g' -e 's#, #\n#g' >> >> and comparing gives: >> @@ -13,7 +13,12 @@ >> rx_phy_crc_errors=0 >> rx_phy_in_range_len_errors=0 >> rx_phy_symbol_errors=0 >> +rx_q0_bytes=0 >> rx_q0_errors=0 >> +rx_q0_packets=0 >> +rx_q1_bytes=0 >> +rx_q1_errors=0 >> +rx_q1_packets=0 >> rx_wqe_errors=0 >> tx_broadcast_packets=0 >> tx_bytes=0 >> @@ -27,3 +32,13 @@ >> tx_pp_rearm_queue_errors=0 >> tx_pp_timestamp_future_errors=0 >> tx_pp_timestamp_past_errors=0 >> +tx_q0_bytes=0 >> +tx_q0_packets=0 >> +tx_q1_bytes=0 >> +tx_q1_packets=0 >> +tx_q2_bytes=0 >> +tx_q2_packets=0 >> +tx_q3_bytes=0 >> +tx_q3_packets=0 >> +tx_q4_bytes=0 >> +tx_q4_packets=0 >> >> Signed-off-by: David Marchand <david.marchand@redhat.com> > > The patch looks good but I tested with with ixgbe, and seeing something > I am unsure of. I changed the number of rxqs to 11 but I am only seeing > stats for q0. i'm not sure if this because there was no traffic yet > (even though there is also no traffic on q0). i will set up a traffic > gen and see what happens when the q1-q10 receives traffic. I think the problem is that when port reconfiguration is called because the number of queues changed, netdev_dpdk_clear_xstats() is not called. So the next time netdev_dpdk_configure_xstats() is called, it returns true early as it is already configured. So old configuration is keeped. When increasing the number of queues, I think we get your behavior, i.e. only previous number of queues counters are disaplayed. But when decreasing the number of queues, I think rte_eth_xstats_get_by_id() will fail. I think calling netdev_dpdk_clear_xstats() in netdev_dpdk_reconfigure() should fix your issue. Maxime
On Fri, Nov 19, 2021 at 10:40 AM Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > > Hi Kevin, David, > > On 11/18/21 16:31, Kevin Traynor wrote: > > On 15/10/2021 16:04, David Marchand wrote: > >> When troubleshooting multiqueue setups, having per queue statistics helps > >> checking packets repartition in rx and tx queues. > >> > >> Per queue statistics are exported by most DPDK drivers (with capability > >> RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS). But since OVS only filters statistics > >> it exposes, there is nothing to request in DPDK API. > >> > >> Extend existing filter with a regular expression. > >> string_ends_with() helper is not used anymore, and removed as a > >> consequence. > >> > >> Querying statistics with > >> $ ovs-vsctl get interface dpdk0 statistics | \ > >> sed -e 's#[{}]##g' -e 's#, #\n#g' > >> > >> and comparing gives: > >> @@ -13,7 +13,12 @@ > >> rx_phy_crc_errors=0 > >> rx_phy_in_range_len_errors=0 > >> rx_phy_symbol_errors=0 > >> +rx_q0_bytes=0 > >> rx_q0_errors=0 > >> +rx_q0_packets=0 > >> +rx_q1_bytes=0 > >> +rx_q1_errors=0 > >> +rx_q1_packets=0 > >> rx_wqe_errors=0 > >> tx_broadcast_packets=0 > >> tx_bytes=0 > >> @@ -27,3 +32,13 @@ > >> tx_pp_rearm_queue_errors=0 > >> tx_pp_timestamp_future_errors=0 > >> tx_pp_timestamp_past_errors=0 > >> +tx_q0_bytes=0 > >> +tx_q0_packets=0 > >> +tx_q1_bytes=0 > >> +tx_q1_packets=0 > >> +tx_q2_bytes=0 > >> +tx_q2_packets=0 > >> +tx_q3_bytes=0 > >> +tx_q3_packets=0 > >> +tx_q4_bytes=0 > >> +tx_q4_packets=0 > >> > >> Signed-off-by: David Marchand <david.marchand@redhat.com> > > > > The patch looks good but I tested with with ixgbe, and seeing something > > I am unsure of. I changed the number of rxqs to 11 but I am only seeing > > stats for q0. i'm not sure if this because there was no traffic yet > > (even though there is also no traffic on q0). i will set up a traffic > > gen and see what happens when the q1-q10 receives traffic. > > I think the problem is that when port reconfiguration is called because > the number of queues changed, netdev_dpdk_clear_xstats() is not called. > > So the next time netdev_dpdk_configure_xstats() is called, it returns > true early as it is already configured. So old configuration is keeped. > > When increasing the number of queues, I think we get your behavior, i.e. > only previous number of queues counters are disaplayed. But when > decreasing the number of queues, I think rte_eth_xstats_get_by_id() will > fail. > > I think calling netdev_dpdk_clear_xstats() in netdev_dpdk_reconfigure() > should fix your issue. Thanks for the analysis. I'll need a bit more time to dig in (I don't understand the logic behind the configure_xstats/clear_xstats stuff). In any case, it seems this is a bug predating my patch.
On 19/11/2021 10:31, David Marchand wrote: > On Fri, Nov 19, 2021 at 10:40 AM Maxime Coquelin > <maxime.coquelin@redhat.com> wrote: >> >> Hi Kevin, David, >> >> On 11/18/21 16:31, Kevin Traynor wrote: >>> On 15/10/2021 16:04, David Marchand wrote: >>>> When troubleshooting multiqueue setups, having per queue statistics helps >>>> checking packets repartition in rx and tx queues. >>>> >>>> Per queue statistics are exported by most DPDK drivers (with capability >>>> RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS). But since OVS only filters statistics >>>> it exposes, there is nothing to request in DPDK API. >>>> >>>> Extend existing filter with a regular expression. >>>> string_ends_with() helper is not used anymore, and removed as a >>>> consequence. >>>> >>>> Querying statistics with >>>> $ ovs-vsctl get interface dpdk0 statistics | \ >>>> sed -e 's#[{}]##g' -e 's#, #\n#g' >>>> >>>> and comparing gives: >>>> @@ -13,7 +13,12 @@ >>>> rx_phy_crc_errors=0 >>>> rx_phy_in_range_len_errors=0 >>>> rx_phy_symbol_errors=0 >>>> +rx_q0_bytes=0 >>>> rx_q0_errors=0 >>>> +rx_q0_packets=0 >>>> +rx_q1_bytes=0 >>>> +rx_q1_errors=0 >>>> +rx_q1_packets=0 >>>> rx_wqe_errors=0 >>>> tx_broadcast_packets=0 >>>> tx_bytes=0 >>>> @@ -27,3 +32,13 @@ >>>> tx_pp_rearm_queue_errors=0 >>>> tx_pp_timestamp_future_errors=0 >>>> tx_pp_timestamp_past_errors=0 >>>> +tx_q0_bytes=0 >>>> +tx_q0_packets=0 >>>> +tx_q1_bytes=0 >>>> +tx_q1_packets=0 >>>> +tx_q2_bytes=0 >>>> +tx_q2_packets=0 >>>> +tx_q3_bytes=0 >>>> +tx_q3_packets=0 >>>> +tx_q4_bytes=0 >>>> +tx_q4_packets=0 >>>> >>>> Signed-off-by: David Marchand <david.marchand@redhat.com> >>> >>> The patch looks good but I tested with with ixgbe, and seeing something >>> I am unsure of. I changed the number of rxqs to 11 but I am only seeing >>> stats for q0. i'm not sure if this because there was no traffic yet >>> (even though there is also no traffic on q0). i will set up a traffic >>> gen and see what happens when the q1-q10 receives traffic. >> >> I think the problem is that when port reconfiguration is called because >> the number of queues changed, netdev_dpdk_clear_xstats() is not called. >> >> So the next time netdev_dpdk_configure_xstats() is called, it returns >> true early as it is already configured. So old configuration is keeped. >> >> When increasing the number of queues, I think we get your behavior, i.e. >> only previous number of queues counters are disaplayed. But when >> decreasing the number of queues, I think rte_eth_xstats_get_by_id() will >> fail. >> >> I think calling netdev_dpdk_clear_xstats() in netdev_dpdk_reconfigure() >> should fix your issue. > > Thanks for the analysis. > > I'll need a bit more time to dig in (I don't understand the logic > behind the configure_xstats/clear_xstats stuff). > In any case, it seems this is a bug predating my patch. > > Thanks Maxime. Yes, that seems to be explanation for it. The issue was also present with the txqs when the number of PMDs was increased/decreased, hence changing requested txqs. I added below and it is showing the right amount of rx/tx queues now. I'm not sure it's an existing bug that we'd backport a fix for as the num queues changing wouldn't have previously impacted because of the filter. It seems more like a short cut that we now can't keep :-) diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index ca92c947a..720041c34 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1876,4 +1876,5 @@ dpdk_set_rxq_config(struct netdev_dpdk *dev, const struct smap *args) dev->requested_n_rxq = new_n_rxq; netdev_request_reconfigure(&dev->up); + netdev_dpdk_clear_xstats(dev); } } @@ -2109,4 +2110,5 @@ netdev_dpdk_set_tx_multiq(struct netdev *netdev, unsigned int n_txq) dev->requested_n_txq = n_txq; netdev_request_reconfigure(netdev); + netdev_dpdk_clear_xstats(dev);
On 11/19/21 13:08, Kevin Traynor wrote: > On 19/11/2021 10:31, David Marchand wrote: >> On Fri, Nov 19, 2021 at 10:40 AM Maxime Coquelin >> <maxime.coquelin@redhat.com> wrote: >>> >>> Hi Kevin, David, >>> >>> On 11/18/21 16:31, Kevin Traynor wrote: >>>> On 15/10/2021 16:04, David Marchand wrote: >>>>> When troubleshooting multiqueue setups, having per queue statistics >>>>> helps >>>>> checking packets repartition in rx and tx queues. >>>>> >>>>> Per queue statistics are exported by most DPDK drivers (with >>>>> capability >>>>> RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS). But since OVS only filters >>>>> statistics >>>>> it exposes, there is nothing to request in DPDK API. >>>>> >>>>> Extend existing filter with a regular expression. >>>>> string_ends_with() helper is not used anymore, and removed as a >>>>> consequence. >>>>> >>>>> Querying statistics with >>>>> $ ovs-vsctl get interface dpdk0 statistics | \ >>>>> sed -e 's#[{}]##g' -e 's#, #\n#g' >>>>> >>>>> and comparing gives: >>>>> @@ -13,7 +13,12 @@ >>>>> rx_phy_crc_errors=0 >>>>> rx_phy_in_range_len_errors=0 >>>>> rx_phy_symbol_errors=0 >>>>> +rx_q0_bytes=0 >>>>> rx_q0_errors=0 >>>>> +rx_q0_packets=0 >>>>> +rx_q1_bytes=0 >>>>> +rx_q1_errors=0 >>>>> +rx_q1_packets=0 >>>>> rx_wqe_errors=0 >>>>> tx_broadcast_packets=0 >>>>> tx_bytes=0 >>>>> @@ -27,3 +32,13 @@ >>>>> tx_pp_rearm_queue_errors=0 >>>>> tx_pp_timestamp_future_errors=0 >>>>> tx_pp_timestamp_past_errors=0 >>>>> +tx_q0_bytes=0 >>>>> +tx_q0_packets=0 >>>>> +tx_q1_bytes=0 >>>>> +tx_q1_packets=0 >>>>> +tx_q2_bytes=0 >>>>> +tx_q2_packets=0 >>>>> +tx_q3_bytes=0 >>>>> +tx_q3_packets=0 >>>>> +tx_q4_bytes=0 >>>>> +tx_q4_packets=0 >>>>> >>>>> Signed-off-by: David Marchand <david.marchand@redhat.com> >>>> >>>> The patch looks good but I tested with with ixgbe, and seeing something >>>> I am unsure of. I changed the number of rxqs to 11 but I am only seeing >>>> stats for q0. i'm not sure if this because there was no traffic yet >>>> (even though there is also no traffic on q0). i will set up a traffic >>>> gen and see what happens when the q1-q10 receives traffic. >>> >>> I think the problem is that when port reconfiguration is called because >>> the number of queues changed, netdev_dpdk_clear_xstats() is not called. >>> >>> So the next time netdev_dpdk_configure_xstats() is called, it returns >>> true early as it is already configured. So old configuration is keeped. >>> >>> When increasing the number of queues, I think we get your behavior, i.e. >>> only previous number of queues counters are disaplayed. But when >>> decreasing the number of queues, I think rte_eth_xstats_get_by_id() will >>> fail. >>> >>> I think calling netdev_dpdk_clear_xstats() in netdev_dpdk_reconfigure() >>> should fix your issue. >> >> Thanks for the analysis. >> >> I'll need a bit more time to dig in (I don't understand the logic >> behind the configure_xstats/clear_xstats stuff). >> In any case, it seems this is a bug predating my patch. >> >> > > Thanks Maxime. Yes, that seems to be explanation for it. > > The issue was also present with the txqs when the number of PMDs was > increased/decreased, hence changing requested txqs. I added below and it > is showing the right amount of rx/tx queues now. > > I'm not sure it's an existing bug that we'd backport a fix for as the > num queues changing wouldn't have previously impacted because of the > filter. It seems more like a short cut that we now can't keep :-) As per David's commit log above, don't we need to backport it because we already dumped rxqN_error counter before? > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index ca92c947a..720041c34 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1876,4 +1876,5 @@ dpdk_set_rxq_config(struct netdev_dpdk *dev, const > struct smap *args) > dev->requested_n_rxq = new_n_rxq; > netdev_request_reconfigure(&dev->up); > + netdev_dpdk_clear_xstats(dev); > } > } > @@ -2109,4 +2110,5 @@ netdev_dpdk_set_tx_multiq(struct netdev *netdev, > unsigned int n_txq) > dev->requested_n_txq = n_txq; > netdev_request_reconfigure(netdev); > + netdev_dpdk_clear_xstats(dev); >
On 19/11/2021 12:45, Maxime Coquelin wrote: > > > On 11/19/21 13:08, Kevin Traynor wrote: >> On 19/11/2021 10:31, David Marchand wrote: >>> On Fri, Nov 19, 2021 at 10:40 AM Maxime Coquelin >>> <maxime.coquelin@redhat.com> wrote: >>>> >>>> Hi Kevin, David, >>>> >>>> On 11/18/21 16:31, Kevin Traynor wrote: >>>>> On 15/10/2021 16:04, David Marchand wrote: >>>>>> When troubleshooting multiqueue setups, having per queue statistics >>>>>> helps >>>>>> checking packets repartition in rx and tx queues. >>>>>> >>>>>> Per queue statistics are exported by most DPDK drivers (with >>>>>> capability >>>>>> RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS). But since OVS only filters >>>>>> statistics >>>>>> it exposes, there is nothing to request in DPDK API. >>>>>> >>>>>> Extend existing filter with a regular expression. >>>>>> string_ends_with() helper is not used anymore, and removed as a >>>>>> consequence. >>>>>> >>>>>> Querying statistics with >>>>>> $ ovs-vsctl get interface dpdk0 statistics | \ >>>>>> sed -e 's#[{}]##g' -e 's#, #\n#g' >>>>>> >>>>>> and comparing gives: >>>>>> @@ -13,7 +13,12 @@ >>>>>> rx_phy_crc_errors=0 >>>>>> rx_phy_in_range_len_errors=0 >>>>>> rx_phy_symbol_errors=0 >>>>>> +rx_q0_bytes=0 >>>>>> rx_q0_errors=0 >>>>>> +rx_q0_packets=0 >>>>>> +rx_q1_bytes=0 >>>>>> +rx_q1_errors=0 >>>>>> +rx_q1_packets=0 >>>>>> rx_wqe_errors=0 >>>>>> tx_broadcast_packets=0 >>>>>> tx_bytes=0 >>>>>> @@ -27,3 +32,13 @@ >>>>>> tx_pp_rearm_queue_errors=0 >>>>>> tx_pp_timestamp_future_errors=0 >>>>>> tx_pp_timestamp_past_errors=0 >>>>>> +tx_q0_bytes=0 >>>>>> +tx_q0_packets=0 >>>>>> +tx_q1_bytes=0 >>>>>> +tx_q1_packets=0 >>>>>> +tx_q2_bytes=0 >>>>>> +tx_q2_packets=0 >>>>>> +tx_q3_bytes=0 >>>>>> +tx_q3_packets=0 >>>>>> +tx_q4_bytes=0 >>>>>> +tx_q4_packets=0 >>>>>> >>>>>> Signed-off-by: David Marchand <david.marchand@redhat.com> >>>>> >>>>> The patch looks good but I tested with with ixgbe, and seeing something >>>>> I am unsure of. I changed the number of rxqs to 11 but I am only seeing >>>>> stats for q0. i'm not sure if this because there was no traffic yet >>>>> (even though there is also no traffic on q0). i will set up a traffic >>>>> gen and see what happens when the q1-q10 receives traffic. >>>> >>>> I think the problem is that when port reconfiguration is called because >>>> the number of queues changed, netdev_dpdk_clear_xstats() is not called. >>>> >>>> So the next time netdev_dpdk_configure_xstats() is called, it returns >>>> true early as it is already configured. So old configuration is keeped. >>>> >>>> When increasing the number of queues, I think we get your behavior, i.e. >>>> only previous number of queues counters are disaplayed. But when >>>> decreasing the number of queues, I think rte_eth_xstats_get_by_id() will >>>> fail. >>>> >>>> I think calling netdev_dpdk_clear_xstats() in netdev_dpdk_reconfigure() >>>> should fix your issue. >>> >>> Thanks for the analysis. >>> >>> I'll need a bit more time to dig in (I don't understand the logic >>> behind the configure_xstats/clear_xstats stuff). >>> In any case, it seems this is a bug predating my patch. >>> >>> >> >> Thanks Maxime. Yes, that seems to be explanation for it. >> >> The issue was also present with the txqs when the number of PMDs was >> increased/decreased, hence changing requested txqs. I added below and it >> is showing the right amount of rx/tx queues now. >> >> I'm not sure it's an existing bug that we'd backport a fix for as the >> num queues changing wouldn't have previously impacted because of the >> filter. It seems more like a short cut that we now can't keep :-) > > As per David's commit log above, don't we need to backport it because we > already dumped rxqN_error counter before? > Yes, I forgot that the filter would catch the rx_qN_error already, so you're both right, it's an existing bug. >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index ca92c947a..720041c34 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -1876,4 +1876,5 @@ dpdk_set_rxq_config(struct netdev_dpdk *dev, const >> struct smap *args) >> dev->requested_n_rxq = new_n_rxq; >> netdev_request_reconfigure(&dev->up); >> + netdev_dpdk_clear_xstats(dev); >> } >> } >> @@ -2109,4 +2110,5 @@ netdev_dpdk_set_tx_multiq(struct netdev *netdev, >> unsigned int n_txq) >> dev->requested_n_txq = n_txq; >> netdev_request_reconfigure(netdev); >> + netdev_dpdk_clear_xstats(dev); >> >
On Thu, Nov 18, 2021 at 4:31 PM Kevin Traynor <ktraynor@redhat.com> wrote: > > @@ -1583,6 +1584,13 @@ netdev_dpdk_get_xstat_name(struct netdev_dpdk *dev, uint64_t id) > > return dev->rte_xstats_names[id].name; > > } > > > > +/* We filter out everything except per rxq/txq basic stats, and dropped, > > + * error and management counters. > > + * Note: rx_qX_errors is handled by the _errors$ pattern. > > + */ > > +#define DPDK_STATS_REGEX_FILTER \ > > + "(^(rx_q|tx_q)[0-9]*_(packets|bytes)$|_errors$|_dropped$|_management_)" > > + > > iirc, there was some pmds that were not conforming correctly with this > rx_q syntax, but I *think* they were fixed a few releases ago in DPDK. > Just mentioning because if there was still some, they could be fixed or > the regex could be expanded to capture them. In theory, those per q stats are part of the "basic" stats handled at the ethdev level. There should be no issue for them. I remember an issue with incorrect stats, but it was a conflict between OVS and i40e stats. Is this what you had in mind?
On 23/11/2021 15:13, David Marchand wrote: > On Thu, Nov 18, 2021 at 4:31 PM Kevin Traynor <ktraynor@redhat.com> wrote: >>> @@ -1583,6 +1584,13 @@ netdev_dpdk_get_xstat_name(struct netdev_dpdk *dev, uint64_t id) >>> return dev->rte_xstats_names[id].name; >>> } >>> >>> +/* We filter out everything except per rxq/txq basic stats, and dropped, >>> + * error and management counters. >>> + * Note: rx_qX_errors is handled by the _errors$ pattern. >>> + */ >>> +#define DPDK_STATS_REGEX_FILTER \ >>> + "(^(rx_q|tx_q)[0-9]*_(packets|bytes)$|_errors$|_dropped$|_management_)" >>> + >> >> iirc, there was some pmds that were not conforming correctly with this >> rx_q syntax, but I *think* they were fixed a few releases ago in DPDK. >> Just mentioning because if there was still some, they could be fixed or >> the regex could be expanded to capture them. > > In theory, those per q stats are part of the "basic" stats handled at > the ethdev level. > There should be no issue for them. > > > I remember an issue with incorrect stats, but it was a conflict > between OVS and i40e stats. > Is this what you had in mind? > > No, I had a thought that some driver for maybe Broadcom or Mellanox was reporting say 'rxq' instead of 'rx_q' and it was noticed and fixed while some other work on xstats. But it's hazy and I can't find it now, so best just to ignore unless someone hits it.
On 10/15/21 17:04, David Marchand wrote: > When troubleshooting multiqueue setups, having per queue statistics helps > checking packets repartition in rx and tx queues. > > Per queue statistics are exported by most DPDK drivers (with capability > RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS). But since OVS only filters statistics > it exposes, there is nothing to request in DPDK API. > > Extend existing filter with a regular expression. > string_ends_with() helper is not used anymore, and removed as a > consequence. > > Querying statistics with > $ ovs-vsctl get interface dpdk0 statistics | \ > sed -e 's#[{}]##g' -e 's#, #\n#g' > > and comparing gives: > @@ -13,7 +13,12 @@ > rx_phy_crc_errors=0 > rx_phy_in_range_len_errors=0 > rx_phy_symbol_errors=0 > +rx_q0_bytes=0 > rx_q0_errors=0 > +rx_q0_packets=0 > +rx_q1_bytes=0 > +rx_q1_errors=0 > +rx_q1_packets=0 > rx_wqe_errors=0 > tx_broadcast_packets=0 > tx_bytes=0 > @@ -27,3 +32,13 @@ > tx_pp_rearm_queue_errors=0 > tx_pp_timestamp_future_errors=0 > tx_pp_timestamp_past_errors=0 > +tx_q0_bytes=0 > +tx_q0_packets=0 > +tx_q1_bytes=0 > +tx_q1_packets=0 > +tx_q2_bytes=0 > +tx_q2_packets=0 > +tx_q3_bytes=0 > +tx_q3_packets=0 > +tx_q4_bytes=0 > +tx_q4_packets=0 > > Signed-off-by: David Marchand <david.marchand@redhat.com> > --- > Sending this as a RFC for feedback. > vhost user ports are left untouched for now, but could be added in the > future. > > --- > lib/netdev-dpdk.c | 32 ++++++++++++++++++++++++++------ > lib/util.c | 13 ------------- > lib/util.h | 2 -- > 3 files changed, 26 insertions(+), 21 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index ca92c947a2..12447cc4b2 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -18,6 +18,7 @@ > #include "netdev-dpdk.h" > > #include <errno.h> > +#include <regex.h> > #include <signal.h> > #include <stdlib.h> > #include <string.h> > @@ -1583,6 +1584,13 @@ netdev_dpdk_get_xstat_name(struct netdev_dpdk *dev, uint64_t id) > return dev->rte_xstats_names[id].name; > } > > +/* We filter out everything except per rxq/txq basic stats, and dropped, > + * error and management counters. > + * Note: rx_qX_errors is handled by the _errors$ pattern. > + */ > +#define DPDK_STATS_REGEX_FILTER \ > + "(^(rx_q|tx_q)[0-9]*_(packets|bytes)$|_errors$|_dropped$|_management_)" > + Hi, David. The idea seems OK to me, but I'm a bit concerned about using the regex. It seems that not every libc implements this API and it might become a problem with attempts to enable OVS with DPDK on Windows, AFAICT. Can we replace this regex with a usual string checking function? For example, we could keep the current checks and add something like this: static bool is_queue_stat(const char *s) { return (s[0] == 'r' || s[1] == 't') && (ovs_scan(s + 1, "x_q%"SCNu16"_packets", &tmp) || ovs_scan(s + 1, "x_q%"SCNu16"_bytes", &tmp)); } Should be equal to your regex, if I didn't mess up something. What do you think? Best regards, Ilya Maximets.
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index ca92c947a2..12447cc4b2 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -18,6 +18,7 @@ #include "netdev-dpdk.h" #include <errno.h> +#include <regex.h> #include <signal.h> #include <stdlib.h> #include <string.h> @@ -1583,6 +1584,13 @@ netdev_dpdk_get_xstat_name(struct netdev_dpdk *dev, uint64_t id) return dev->rte_xstats_names[id].name; } +/* We filter out everything except per rxq/txq basic stats, and dropped, + * error and management counters. + * Note: rx_qX_errors is handled by the _errors$ pattern. + */ +#define DPDK_STATS_REGEX_FILTER \ + "(^(rx_q|tx_q)[0-9]*_(packets|bytes)$|_errors$|_dropped$|_management_)" + static bool netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex) @@ -1617,6 +1625,20 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) sizeof *dev->rte_xstats_names); if (dev->rte_xstats_names) { + int err; + regex_t r; + + err = regcomp(&r, DPDK_STATS_REGEX_FILTER, REG_EXTENDED); + if (err != 0) { + size_t errbuf_size = regerror(err, &r, NULL, 0); + char *errbuf = xcalloc(1, errbuf_size); + + regerror(err, &r, errbuf, errbuf_size); + VLOG_DBG("Could not setup stats regexp: %s", errbuf); + free(errbuf); + goto out; + } + /* Retreive xstats names */ rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, @@ -1626,10 +1648,12 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) if (rte_xstats_len < 0) { VLOG_WARN("Cannot get XSTATS names for port: " DPDK_PORT_ID_FMT, dev->port_id); + regfree(&r); goto out; } else if (rte_xstats_len != dev->rte_xstats_names_size) { VLOG_WARN("XSTATS size doesn't match for port: " DPDK_PORT_ID_FMT, dev->port_id); + regfree(&r); goto out; } @@ -1648,12 +1672,7 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) for (uint32_t i = 0; i < rte_xstats_len; i++) { id = rte_xstats[i].id; name = netdev_dpdk_get_xstat_name(dev, id); - /* We need to filter out everything except - * dropped, error and management counters */ - if (string_ends_with(name, "_errors") || - strstr(name, "_management_") || - string_ends_with(name, "_dropped")) { - + if (regexec(&r, name, 0, NULL, 0) == 0) { dev->rte_xstats_ids[xstats_no] = id; xstats_no++; } @@ -1666,6 +1685,7 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) } free(rte_xstats); + regfree(&r); } } } else { diff --git a/lib/util.c b/lib/util.c index 1195c79821..34755a4dad 100644 --- a/lib/util.c +++ b/lib/util.c @@ -405,19 +405,6 @@ ovs_strzcpy(char *dst, const char *src, size_t size) } } -/* - * Returns true if 'str' ends with given 'suffix'. - */ -int -string_ends_with(const char *str, const char *suffix) -{ - int str_len = strlen(str); - int suffix_len = strlen(suffix); - - return (str_len >= suffix_len) && - (0 == strcmp(str + (str_len - suffix_len), suffix)); -} - /* Prints 'format' on stderr, formatting it like printf() does. If 'err_no' is * nonzero, then it is formatted with ovs_retval_to_string() and appended to * the message inside parentheses. Then, terminates with abort(). diff --git a/lib/util.h b/lib/util.h index aea19d45fa..4ad6e28274 100644 --- a/lib/util.h +++ b/lib/util.h @@ -183,8 +183,6 @@ void free_cacheline(void *); void ovs_strlcpy(char *dst, const char *src, size_t size); void ovs_strzcpy(char *dst, const char *src, size_t size); -int string_ends_with(const char *str, const char *suffix); - void *xmalloc_pagealign(size_t) MALLOC_LIKE; void free_pagealign(void *); void *xmalloc_size_align(size_t, size_t) MALLOC_LIKE;
When troubleshooting multiqueue setups, having per queue statistics helps checking packets repartition in rx and tx queues. Per queue statistics are exported by most DPDK drivers (with capability RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS). But since OVS only filters statistics it exposes, there is nothing to request in DPDK API. Extend existing filter with a regular expression. string_ends_with() helper is not used anymore, and removed as a consequence. Querying statistics with $ ovs-vsctl get interface dpdk0 statistics | \ sed -e 's#[{}]##g' -e 's#, #\n#g' and comparing gives: @@ -13,7 +13,12 @@ rx_phy_crc_errors=0 rx_phy_in_range_len_errors=0 rx_phy_symbol_errors=0 +rx_q0_bytes=0 rx_q0_errors=0 +rx_q0_packets=0 +rx_q1_bytes=0 +rx_q1_errors=0 +rx_q1_packets=0 rx_wqe_errors=0 tx_broadcast_packets=0 tx_bytes=0 @@ -27,3 +32,13 @@ tx_pp_rearm_queue_errors=0 tx_pp_timestamp_future_errors=0 tx_pp_timestamp_past_errors=0 +tx_q0_bytes=0 +tx_q0_packets=0 +tx_q1_bytes=0 +tx_q1_packets=0 +tx_q2_bytes=0 +tx_q2_packets=0 +tx_q3_bytes=0 +tx_q3_packets=0 +tx_q4_bytes=0 +tx_q4_packets=0 Signed-off-by: David Marchand <david.marchand@redhat.com> --- Sending this as a RFC for feedback. vhost user ports are left untouched for now, but could be added in the future. --- lib/netdev-dpdk.c | 32 ++++++++++++++++++++++++++------ lib/util.c | 13 ------------- lib/util.h | 2 -- 3 files changed, 26 insertions(+), 21 deletions(-)