| Message ID | 20211124212400.70613-2-maxime.coquelin@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dpif-netdev: Hash-based Tx packet steering | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Hey Maxime, On Wed, Nov 24, 2021 at 10:24 PM Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > > HXPS feature will enable steering Tx packets on transmist transmit* > queues based on their hashes. In order to test the feature, "their hashes" is ambiguous. > it is needed to be able to get the per-queue statistics for > Vhost-user ports. This is a general comment, for consistency, I'd write vhost, all lowercase. > > This patch introduces "bytes", "packets" and "error" > per-queue custom statistics for Vhost-user ports. > > Suggested-by David Marchand <david.marchand@redhat.com> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > lib/netdev-dpdk.c | 143 +++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 135 insertions(+), 8 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index ca92c947a..e80d5b4ab 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -192,6 +192,13 @@ static const struct vhost_device_ops virtio_net_device_ops = > .guest_notified = vhost_guest_notified, > }; > > +/* Custome software per-queue stats for Vhost ports */ Custom* > +struct netdev_dpdk_vhost_q_stats { > + uint64_t bytes; > + uint64_t packets; > + uint64_t errors; > +}; > + > /* Custom software stats for dpdk ports */ > struct netdev_dpdk_sw_stats { > /* No. of retries when unable to transmit. */ > @@ -206,6 +213,10 @@ struct netdev_dpdk_sw_stats { > uint64_t rx_qos_drops; > /* Packet drops in HWOL processing. */ > uint64_t tx_invalid_hwol_drops; > + /* Per-queue Vhost Tx stats */ > + struct netdev_dpdk_vhost_q_stats *txq; > + /* Per-queue Vhost Rx stats */ > + struct netdev_dpdk_vhost_q_stats *rxq; Here, we add "driver" specific stats, while netdev_dpdk_sw_stats struct carries OVS "own" stats. This netdev_dpdk_sw_stats struct is converted by netdev_dpdk_get_sw_custom_stats and there is a small framework on adding custom OVS stats (using some macros "trick"). I'd rather leave netdev_dpdk_sw_stats struct untouched for consistency. Pointers to vhost specific stats can be added to the netdev_dpdk struct (we have some spare space after the pointer to netdev_dpdk_sw_stats). > }; > > enum dpdk_dev_type { > @@ -1276,6 +1287,13 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, > dev->sw_stats = xzalloc(sizeof *dev->sw_stats); > dev->sw_stats->tx_retries = (dev->type == DPDK_DEV_VHOST) ? 0 : UINT64_MAX; > > + if (dev->type == DPDK_DEV_VHOST) { > + dev->sw_stats->txq = xcalloc(netdev->n_txq, > + sizeof *dev->sw_stats->txq); > + dev->sw_stats->rxq = xcalloc(netdev->n_rxq, > + sizeof *dev->sw_stats->rxq); > + } > + > return 0; > } > > @@ -2353,17 +2371,21 @@ netdev_dpdk_vhost_update_rx_size_counters(struct netdev_stats *stats, > } > > static inline void > -netdev_dpdk_vhost_update_rx_counters(struct netdev_dpdk *dev, > +netdev_dpdk_vhost_update_rx_counters(struct netdev_dpdk *dev, int qid, > struct dp_packet **packets, int count, > int qos_drops) > { > struct netdev_stats *stats = &dev->stats; > + struct netdev_dpdk_vhost_q_stats *q_stats = &dev->sw_stats->rxq[qid]; reverse xmas tree? > struct dp_packet *packet; > unsigned int packet_size; > int i; > > stats->rx_packets += count; > + q_stats->packets += count; > stats->rx_dropped += qos_drops; > + q_stats->errors += qos_drops; > + > for (i = 0; i < count; i++) { > packet = packets[i]; > packet_size = dp_packet_size(packet); > @@ -2374,6 +2396,7 @@ netdev_dpdk_vhost_update_rx_counters(struct netdev_dpdk *dev, > * further processing. */ > stats->rx_errors++; > stats->rx_length_errors++; > + q_stats->errors++; > continue; > } > > @@ -2385,6 +2408,7 @@ netdev_dpdk_vhost_update_rx_counters(struct netdev_dpdk *dev, > } > > stats->rx_bytes += packet_size; > + q_stats->bytes += packet_size; > } > > if (OVS_UNLIKELY(qos_drops)) { > @@ -2437,7 +2461,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq, > } > > rte_spinlock_lock(&dev->stats_lock); > - netdev_dpdk_vhost_update_rx_counters(dev, batch->packets, > + netdev_dpdk_vhost_update_rx_counters(dev, qid, batch->packets, > nb_rx, qos_drops); > rte_spinlock_unlock(&dev->stats_lock); > > @@ -2551,7 +2575,7 @@ netdev_dpdk_filter_packet_len(struct netdev_dpdk *dev, struct rte_mbuf **pkts, > } > > static inline void > -netdev_dpdk_vhost_update_tx_counters(struct netdev_dpdk *dev, > +netdev_dpdk_vhost_update_tx_counters(struct netdev_dpdk *dev, int qid, > struct dp_packet **packets, > int attempted, > struct netdev_dpdk_sw_stats *sw_stats_add) > @@ -2561,14 +2585,20 @@ netdev_dpdk_vhost_update_tx_counters(struct netdev_dpdk *dev, > sw_stats_add->tx_failure_drops + > sw_stats_add->tx_invalid_hwol_drops; > struct netdev_stats *stats = &dev->stats; > + struct netdev_dpdk_vhost_q_stats *q_stats = &dev->sw_stats->txq[qid]; reverse xmas tree? > int sent = attempted - dropped; > int i; > > stats->tx_packets += sent; > + q_stats->packets += sent; > stats->tx_dropped += dropped; > + q_stats->errors += dropped; > > for (i = 0; i < sent; i++) { > - stats->tx_bytes += dp_packet_size(packets[i]); > + uint64_t bytes = dp_packet_size(packets[i]); > + > + stats->tx_bytes += bytes; > + q_stats->bytes += bytes; > } > > if (OVS_UNLIKELY(dropped || sw_stats_add->tx_retries)) { > @@ -2656,7 +2686,7 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, > sw_stats_add.tx_retries = MIN(retries, max_retries); > > rte_spinlock_lock(&dev->stats_lock); > - netdev_dpdk_vhost_update_tx_counters(dev, pkts, total_packets, > + netdev_dpdk_vhost_update_tx_counters(dev, qid, pkts, total_packets, > &sw_stats_add); > rte_spinlock_unlock(&dev->stats_lock); > > @@ -3286,6 +3316,72 @@ netdev_dpdk_get_sw_custom_stats(const struct netdev *netdev, > return 0; > } > > +static int > +netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev, > + struct netdev_custom_stats *custom_stats) > +{ > + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > + int sw_stats_size, i, j; > + > + netdev_dpdk_get_sw_custom_stats(netdev, custom_stats); > + > + ovs_mutex_lock(&dev->mutex); > + > + sw_stats_size = custom_stats->size; > + custom_stats->size += netdev->n_rxq * sizeof(*dev->sw_stats->rxq) / > + sizeof(uint64_t); > + custom_stats->size += netdev->n_txq * sizeof(*dev->sw_stats->txq) / > + sizeof(uint64_t); > + > + custom_stats->counters = xrealloc(custom_stats->counters, > + custom_stats->size * > + sizeof *custom_stats->counters); Names and values must be synced in the code below and this might get broken later. We could reuse the same kind of "trick" than for sw_stats. This should make adding stats less error prone. WDYT of (just build tested) diff: diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index e80d5b4ab5..8ad9b785f7 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -3327,52 +3327,53 @@ netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev, ovs_mutex_lock(&dev->mutex); - sw_stats_size = custom_stats->size; - custom_stats->size += netdev->n_rxq * sizeof(*dev->sw_stats->rxq) / - sizeof(uint64_t); - custom_stats->size += netdev->n_txq * sizeof(*dev->sw_stats->txq) / - sizeof(uint64_t); +#define VHOST_Q_STATS \ + VHOST_Q_STAT(bytes) \ + VHOST_Q_STAT(packets) \ + VHOST_Q_STAT(errors) + sw_stats_size = custom_stats->size; +#define VHOST_Q_STAT(NAME) + netdev->n_rxq + custom_stats->size += VHOST_Q_STATS; +#undef VHOST_Q_STAT +#define VHOST_Q_STAT(NAME) + netdev->n_txq + custom_stats->size += VHOST_Q_STATS; +#undef VHOST_Q_STAT custom_stats->counters = xrealloc(custom_stats->counters, custom_stats->size * sizeof *custom_stats->counters); j = 0; for (i = 0; i < netdev->n_rxq; i++) { - snprintf(custom_stats->counters[sw_stats_size + j++].name, - NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_bytes", i); - snprintf(custom_stats->counters[sw_stats_size + j++].name, - NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_packets", i); - snprintf(custom_stats->counters[sw_stats_size + j++].name, - NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_errors", i); +#define VHOST_Q_STAT(NAME) \ + snprintf(custom_stats->counters[sw_stats_size + j++].name, \ + NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_"#NAME, i); + VHOST_Q_STATS +#undef VHOST_Q_STAT } - for (i = 0; i < netdev->n_txq; i++) { - snprintf(custom_stats->counters[sw_stats_size + j++].name, - NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_bytes", i); - snprintf(custom_stats->counters[sw_stats_size + j++].name, - NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_packets", i); - snprintf(custom_stats->counters[sw_stats_size + j++].name, - NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_errors", i); +#define VHOST_Q_STAT(NAME) \ + snprintf(custom_stats->counters[sw_stats_size + j++].name, \ + NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_"#NAME, i); + VHOST_Q_STATS +#undef VHOST_Q_STAT } rte_spinlock_lock(&dev->stats_lock); j = 0; for (i = 0; i < netdev->n_rxq; i++) { - struct netdev_dpdk_vhost_q_stats *rxq_stats = &dev->sw_stats->rxq[i]; - - custom_stats->counters[sw_stats_size + j++].value = rxq_stats->bytes; - custom_stats->counters[sw_stats_size + j++].value = rxq_stats->packets; - custom_stats->counters[sw_stats_size + j++].value = rxq_stats->errors; +#define VHOST_Q_STAT(NAME) \ + custom_stats->counters[sw_stats_size + j++].value = dev->sw_stats->rxq[i].NAME; + VHOST_Q_STATS +#undef VHOST_Q_STAT } for (i = 0; i < netdev->n_txq; i++) { - struct netdev_dpdk_vhost_q_stats *txq_stats = &dev->sw_stats->txq[i]; - - custom_stats->counters[sw_stats_size + j++].value = txq_stats->bytes; - custom_stats->counters[sw_stats_size + j++].value = txq_stats->packets; - custom_stats->counters[sw_stats_size + j++].value = txq_stats->errors; +#define VHOST_Q_STAT(NAME) \ + custom_stats->counters[sw_stats_size + j++].value = dev->sw_stats->txq[i].NAME; + VHOST_Q_STATS +#undef VHOST_Q_STAT } rte_spinlock_unlock(&dev->stats_lock); > + > + j = 0; > + for (i = 0; i < netdev->n_rxq; i++) { > + snprintf(custom_stats->counters[sw_stats_size + j++].name, > + NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_bytes", i); > + snprintf(custom_stats->counters[sw_stats_size + j++].name, > + NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_packets", i); > + snprintf(custom_stats->counters[sw_stats_size + j++].name, > + NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_errors", i); > + } > + > + for (i = 0; i < netdev->n_txq; i++) { > + snprintf(custom_stats->counters[sw_stats_size + j++].name, > + NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_bytes", i); > + snprintf(custom_stats->counters[sw_stats_size + j++].name, > + NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_packets", i); > + snprintf(custom_stats->counters[sw_stats_size + j++].name, > + NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_errors", i); > + } > + > + rte_spinlock_lock(&dev->stats_lock); > + > + j = 0; > + for (i = 0; i < netdev->n_rxq; i++) { > + struct netdev_dpdk_vhost_q_stats *rxq_stats = &dev->sw_stats->rxq[i]; > + > + custom_stats->counters[sw_stats_size + j++].value = rxq_stats->bytes; > + custom_stats->counters[sw_stats_size + j++].value = rxq_stats->packets; > + custom_stats->counters[sw_stats_size + j++].value = rxq_stats->errors; > + } > + > + for (i = 0; i < netdev->n_txq; i++) { > + struct netdev_dpdk_vhost_q_stats *txq_stats = &dev->sw_stats->txq[i]; > + > + custom_stats->counters[sw_stats_size + j++].value = txq_stats->bytes; > + custom_stats->counters[sw_stats_size + j++].value = txq_stats->packets; > + custom_stats->counters[sw_stats_size + j++].value = txq_stats->errors; > + } > + > + rte_spinlock_unlock(&dev->stats_lock); > + > + ovs_mutex_unlock(&dev->mutex); > + > + return 0; > +} > + > static int > netdev_dpdk_get_features(const struct netdev *netdev, > enum netdev_features *current, > @@ -5043,9 +5139,12 @@ static int > dpdk_vhost_reconfigure_helper(struct netdev_dpdk *dev) > OVS_REQUIRES(dev->mutex) > { > + int old_n_txq = dev->up.n_txq; > + int old_n_rxq = dev->up.n_rxq; > + int err; > + > dev->up.n_txq = dev->requested_n_txq; > dev->up.n_rxq = dev->requested_n_rxq; > - int err; > > /* Always keep RX queue 0 enabled for implementations that won't > * report vring states. */ > @@ -5063,6 +5162,34 @@ dpdk_vhost_reconfigure_helper(struct netdev_dpdk *dev) > > netdev_dpdk_remap_txqs(dev); > > + if (dev->up.n_txq != old_n_txq) { > + struct netdev_dpdk_vhost_q_stats *old_txq_stats, *new_txq_stats; > + > + new_txq_stats = xcalloc(dev->up.n_txq, sizeof *dev->sw_stats->txq); > + rte_spinlock_lock(&dev->stats_lock); > + old_txq_stats = dev->sw_stats->txq; > + memcpy(new_txq_stats, old_txq_stats, > + MIN(dev->up.n_txq, old_n_txq) * sizeof *dev->sw_stats->txq); > + dev->sw_stats->txq = new_txq_stats; I prefer realloc and setting extra space to 0, but it is equivalent, no strong opinion on the implementation. > + rte_spinlock_unlock(&dev->stats_lock); > + free(old_txq_stats); > + > + } > + > + if (dev->up.n_rxq != old_n_rxq) { > + struct netdev_dpdk_vhost_q_stats *old_rxq_stats, *new_rxq_stats; > + > + new_rxq_stats = xcalloc(dev->up.n_rxq, sizeof *dev->sw_stats->rxq); > + rte_spinlock_lock(&dev->stats_lock); > + old_rxq_stats = dev->sw_stats->rxq; > + memcpy(new_rxq_stats, old_rxq_stats, > + MIN(dev->up.n_rxq, old_n_rxq) * sizeof *dev->sw_stats->rxq); > + dev->sw_stats->rxq = new_rxq_stats; > + rte_spinlock_unlock(&dev->stats_lock); > + free(old_rxq_stats); > + > + } > + I did not test your patch yet (will do later this week once I get to the end of this series). However I think we are missing some reset to 0 for per q stats in netdev_dpdk_update_flags__. > err = netdev_dpdk_mempool_configure(dev); > if (!err) { > /* A new mempool was created or re-used. */ > @@ -5467,7 +5594,7 @@ static const struct netdev_class dpdk_vhost_class = { > .send = netdev_dpdk_vhost_send, > .get_carrier = netdev_dpdk_vhost_get_carrier, > .get_stats = netdev_dpdk_vhost_get_stats, > - .get_custom_stats = netdev_dpdk_get_sw_custom_stats, > + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, > .get_status = netdev_dpdk_vhost_user_get_status, > .reconfigure = netdev_dpdk_vhost_reconfigure, > .rxq_recv = netdev_dpdk_vhost_rxq_recv, > @@ -5483,7 +5610,7 @@ static const struct netdev_class dpdk_vhost_client_class = { > .send = netdev_dpdk_vhost_send, > .get_carrier = netdev_dpdk_vhost_get_carrier, > .get_stats = netdev_dpdk_vhost_get_stats, > - .get_custom_stats = netdev_dpdk_get_sw_custom_stats, > + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, > .get_status = netdev_dpdk_vhost_user_get_status, > .reconfigure = netdev_dpdk_vhost_client_reconfigure, > .rxq_recv = netdev_dpdk_vhost_rxq_recv, > -- > 2.31.1 >
Hi David, On 12/7/21 21:37, David Marchand wrote: > Hey Maxime, > > On Wed, Nov 24, 2021 at 10:24 PM Maxime Coquelin > <maxime.coquelin@redhat.com> wrote: >> >> HXPS feature will enable steering Tx packets on transmist > > transmit* > >> queues based on their hashes. In order to test the feature, > > "their hashes" is ambiguous. s/their hashes/the packets hashes/ ? >> it is needed to be able to get the per-queue statistics for >> Vhost-user ports. > > This is a general comment, for consistency, I'd write vhost, all lowercase. Ok, I will make it consistent in next revision. > >> >> This patch introduces "bytes", "packets" and "error" >> per-queue custom statistics for Vhost-user ports. >> >> Suggested-by David Marchand <david.marchand@redhat.com> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >> --- >> lib/netdev-dpdk.c | 143 +++++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 135 insertions(+), 8 deletions(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index ca92c947a..e80d5b4ab 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -192,6 +192,13 @@ static const struct vhost_device_ops virtio_net_device_ops = >> .guest_notified = vhost_guest_notified, >> }; >> >> +/* Custome software per-queue stats for Vhost ports */ > > Custom* > >> +struct netdev_dpdk_vhost_q_stats { >> + uint64_t bytes; >> + uint64_t packets; >> + uint64_t errors; >> +}; >> + >> /* Custom software stats for dpdk ports */ >> struct netdev_dpdk_sw_stats { >> /* No. of retries when unable to transmit. */ >> @@ -206,6 +213,10 @@ struct netdev_dpdk_sw_stats { >> uint64_t rx_qos_drops; >> /* Packet drops in HWOL processing. */ >> uint64_t tx_invalid_hwol_drops; >> + /* Per-queue Vhost Tx stats */ >> + struct netdev_dpdk_vhost_q_stats *txq; >> + /* Per-queue Vhost Rx stats */ >> + struct netdev_dpdk_vhost_q_stats *rxq; > > Here, we add "driver" specific stats, while netdev_dpdk_sw_stats > struct carries OVS "own" stats. > This netdev_dpdk_sw_stats struct is converted by > netdev_dpdk_get_sw_custom_stats and there is a small framework on > adding custom OVS stats (using some macros "trick"). > > I'd rather leave netdev_dpdk_sw_stats struct untouched for consistency. > Pointers to vhost specific stats can be added to the netdev_dpdk > struct (we have some spare space after the pointer to > netdev_dpdk_sw_stats). That makes sense, it is indeed better to have it out of netdev_dpdk_sw_stats struct. > >> }; >> >> enum dpdk_dev_type { >> @@ -1276,6 +1287,13 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, >> dev->sw_stats = xzalloc(sizeof *dev->sw_stats); >> dev->sw_stats->tx_retries = (dev->type == DPDK_DEV_VHOST) ? 0 : UINT64_MAX; >> >> + if (dev->type == DPDK_DEV_VHOST) { >> + dev->sw_stats->txq = xcalloc(netdev->n_txq, >> + sizeof *dev->sw_stats->txq); >> + dev->sw_stats->rxq = xcalloc(netdev->n_rxq, >> + sizeof *dev->sw_stats->rxq); >> + } >> + >> return 0; >> } >> >> @@ -2353,17 +2371,21 @@ netdev_dpdk_vhost_update_rx_size_counters(struct netdev_stats *stats, >> } >> >> static inline void >> -netdev_dpdk_vhost_update_rx_counters(struct netdev_dpdk *dev, >> +netdev_dpdk_vhost_update_rx_counters(struct netdev_dpdk *dev, int qid, >> struct dp_packet **packets, int count, >> int qos_drops) >> { >> struct netdev_stats *stats = &dev->stats; >> + struct netdev_dpdk_vhost_q_stats *q_stats = &dev->sw_stats->rxq[qid]; > > reverse xmas tree? Will fix it here and elsewhere. > >> struct dp_packet *packet; >> unsigned int packet_size; >> int i; >> >> stats->rx_packets += count; >> + q_stats->packets += count; >> stats->rx_dropped += qos_drops; >> + q_stats->errors += qos_drops; >> + >> for (i = 0; i < count; i++) { >> packet = packets[i]; >> packet_size = dp_packet_size(packet); >> @@ -2374,6 +2396,7 @@ netdev_dpdk_vhost_update_rx_counters(struct netdev_dpdk *dev, >> * further processing. */ >> stats->rx_errors++; >> stats->rx_length_errors++; >> + q_stats->errors++; >> continue; >> } >> >> @@ -2385,6 +2408,7 @@ netdev_dpdk_vhost_update_rx_counters(struct netdev_dpdk *dev, >> } >> >> stats->rx_bytes += packet_size; >> + q_stats->bytes += packet_size; >> } >> >> if (OVS_UNLIKELY(qos_drops)) { >> @@ -2437,7 +2461,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq, >> } >> >> rte_spinlock_lock(&dev->stats_lock); >> - netdev_dpdk_vhost_update_rx_counters(dev, batch->packets, >> + netdev_dpdk_vhost_update_rx_counters(dev, qid, batch->packets, >> nb_rx, qos_drops); >> rte_spinlock_unlock(&dev->stats_lock); >> >> @@ -2551,7 +2575,7 @@ netdev_dpdk_filter_packet_len(struct netdev_dpdk *dev, struct rte_mbuf **pkts, >> } >> >> static inline void >> -netdev_dpdk_vhost_update_tx_counters(struct netdev_dpdk *dev, >> +netdev_dpdk_vhost_update_tx_counters(struct netdev_dpdk *dev, int qid, >> struct dp_packet **packets, >> int attempted, >> struct netdev_dpdk_sw_stats *sw_stats_add) >> @@ -2561,14 +2585,20 @@ netdev_dpdk_vhost_update_tx_counters(struct netdev_dpdk *dev, >> sw_stats_add->tx_failure_drops + >> sw_stats_add->tx_invalid_hwol_drops; >> struct netdev_stats *stats = &dev->stats; >> + struct netdev_dpdk_vhost_q_stats *q_stats = &dev->sw_stats->txq[qid]; > > reverse xmas tree? > > >> int sent = attempted - dropped; >> int i; >> >> stats->tx_packets += sent; >> + q_stats->packets += sent; >> stats->tx_dropped += dropped; >> + q_stats->errors += dropped; >> >> for (i = 0; i < sent; i++) { >> - stats->tx_bytes += dp_packet_size(packets[i]); >> + uint64_t bytes = dp_packet_size(packets[i]); >> + >> + stats->tx_bytes += bytes; >> + q_stats->bytes += bytes; >> } >> >> if (OVS_UNLIKELY(dropped || sw_stats_add->tx_retries)) { >> @@ -2656,7 +2686,7 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, >> sw_stats_add.tx_retries = MIN(retries, max_retries); >> >> rte_spinlock_lock(&dev->stats_lock); >> - netdev_dpdk_vhost_update_tx_counters(dev, pkts, total_packets, >> + netdev_dpdk_vhost_update_tx_counters(dev, qid, pkts, total_packets, >> &sw_stats_add); >> rte_spinlock_unlock(&dev->stats_lock); >> >> @@ -3286,6 +3316,72 @@ netdev_dpdk_get_sw_custom_stats(const struct netdev *netdev, >> return 0; >> } >> >> +static int >> +netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev, >> + struct netdev_custom_stats *custom_stats) >> +{ >> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >> + int sw_stats_size, i, j; >> + >> + netdev_dpdk_get_sw_custom_stats(netdev, custom_stats); >> + >> + ovs_mutex_lock(&dev->mutex); >> + >> + sw_stats_size = custom_stats->size; >> + custom_stats->size += netdev->n_rxq * sizeof(*dev->sw_stats->rxq) / >> + sizeof(uint64_t); >> + custom_stats->size += netdev->n_txq * sizeof(*dev->sw_stats->txq) / >> + sizeof(uint64_t); >> + >> + custom_stats->counters = xrealloc(custom_stats->counters, >> + custom_stats->size * >> + sizeof *custom_stats->counters); > > Names and values must be synced in the code below and this might get > broken later. > > We could reuse the same kind of "trick" than for sw_stats. > This should make adding stats less error prone. > > WDYT of (just build tested) diff: > I think that sounds reasonable. It will bring consistency with sw_stats and I agree that it will be less error prone. > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index e80d5b4ab5..8ad9b785f7 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -3327,52 +3327,53 @@ netdev_dpdk_vhost_get_custom_stats(const > struct netdev *netdev, > > ovs_mutex_lock(&dev->mutex); > > - sw_stats_size = custom_stats->size; > - custom_stats->size += netdev->n_rxq * sizeof(*dev->sw_stats->rxq) / > - sizeof(uint64_t); > - custom_stats->size += netdev->n_txq * sizeof(*dev->sw_stats->txq) / > - sizeof(uint64_t); > +#define VHOST_Q_STATS \ > + VHOST_Q_STAT(bytes) \ > + VHOST_Q_STAT(packets) \ > + VHOST_Q_STAT(errors) > > + sw_stats_size = custom_stats->size; > +#define VHOST_Q_STAT(NAME) + netdev->n_rxq > + custom_stats->size += VHOST_Q_STATS; > +#undef VHOST_Q_STAT > +#define VHOST_Q_STAT(NAME) + netdev->n_txq > + custom_stats->size += VHOST_Q_STATS; > +#undef VHOST_Q_STAT > custom_stats->counters = xrealloc(custom_stats->counters, > custom_stats->size * > sizeof *custom_stats->counters); > > j = 0; > for (i = 0; i < netdev->n_rxq; i++) { > - snprintf(custom_stats->counters[sw_stats_size + j++].name, > - NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_bytes", i); > - snprintf(custom_stats->counters[sw_stats_size + j++].name, > - NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_packets", i); > - snprintf(custom_stats->counters[sw_stats_size + j++].name, > - NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_errors", i); > +#define VHOST_Q_STAT(NAME) \ > + snprintf(custom_stats->counters[sw_stats_size + j++].name, \ > + NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_"#NAME, i); > + VHOST_Q_STATS > +#undef VHOST_Q_STAT > } > - > for (i = 0; i < netdev->n_txq; i++) { > - snprintf(custom_stats->counters[sw_stats_size + j++].name, > - NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_bytes", i); > - snprintf(custom_stats->counters[sw_stats_size + j++].name, > - NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_packets", i); > - snprintf(custom_stats->counters[sw_stats_size + j++].name, > - NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_errors", i); > +#define VHOST_Q_STAT(NAME) \ > + snprintf(custom_stats->counters[sw_stats_size + j++].name, \ > + NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_"#NAME, i); > + VHOST_Q_STATS > +#undef VHOST_Q_STAT > } > > rte_spinlock_lock(&dev->stats_lock); > > j = 0; > for (i = 0; i < netdev->n_rxq; i++) { > - struct netdev_dpdk_vhost_q_stats *rxq_stats = &dev->sw_stats->rxq[i]; > - > - custom_stats->counters[sw_stats_size + j++].value = rxq_stats->bytes; > - custom_stats->counters[sw_stats_size + j++].value = rxq_stats->packets; > - custom_stats->counters[sw_stats_size + j++].value = rxq_stats->errors; > +#define VHOST_Q_STAT(NAME) \ > + custom_stats->counters[sw_stats_size + j++].value = > dev->sw_stats->rxq[i].NAME; > + VHOST_Q_STATS > +#undef VHOST_Q_STAT > } > > for (i = 0; i < netdev->n_txq; i++) { > - struct netdev_dpdk_vhost_q_stats *txq_stats = &dev->sw_stats->txq[i]; > - > - custom_stats->counters[sw_stats_size + j++].value = txq_stats->bytes; > - custom_stats->counters[sw_stats_size + j++].value = txq_stats->packets; > - custom_stats->counters[sw_stats_size + j++].value = txq_stats->errors; > +#define VHOST_Q_STAT(NAME) \ > + custom_stats->counters[sw_stats_size + j++].value = > dev->sw_stats->txq[i].NAME; > + VHOST_Q_STATS > +#undef VHOST_Q_STAT > } > > rte_spinlock_unlock(&dev->stats_lock); > > > >> + >> + j = 0; >> + for (i = 0; i < netdev->n_rxq; i++) { >> + snprintf(custom_stats->counters[sw_stats_size + j++].name, >> + NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_bytes", i); >> + snprintf(custom_stats->counters[sw_stats_size + j++].name, >> + NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_packets", i); >> + snprintf(custom_stats->counters[sw_stats_size + j++].name, >> + NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_errors", i); >> + } >> + >> + for (i = 0; i < netdev->n_txq; i++) { >> + snprintf(custom_stats->counters[sw_stats_size + j++].name, >> + NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_bytes", i); >> + snprintf(custom_stats->counters[sw_stats_size + j++].name, >> + NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_packets", i); >> + snprintf(custom_stats->counters[sw_stats_size + j++].name, >> + NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_errors", i); >> + } >> + >> + rte_spinlock_lock(&dev->stats_lock); >> + >> + j = 0; >> + for (i = 0; i < netdev->n_rxq; i++) { >> + struct netdev_dpdk_vhost_q_stats *rxq_stats = &dev->sw_stats->rxq[i]; >> + >> + custom_stats->counters[sw_stats_size + j++].value = rxq_stats->bytes; >> + custom_stats->counters[sw_stats_size + j++].value = rxq_stats->packets; >> + custom_stats->counters[sw_stats_size + j++].value = rxq_stats->errors; >> + } >> + >> + for (i = 0; i < netdev->n_txq; i++) { >> + struct netdev_dpdk_vhost_q_stats *txq_stats = &dev->sw_stats->txq[i]; >> + >> + custom_stats->counters[sw_stats_size + j++].value = txq_stats->bytes; >> + custom_stats->counters[sw_stats_size + j++].value = txq_stats->packets; >> + custom_stats->counters[sw_stats_size + j++].value = txq_stats->errors; >> + } >> + >> + rte_spinlock_unlock(&dev->stats_lock); >> + >> + ovs_mutex_unlock(&dev->mutex); >> + >> + return 0; >> +} >> + >> static int >> netdev_dpdk_get_features(const struct netdev *netdev, >> enum netdev_features *current, >> @@ -5043,9 +5139,12 @@ static int >> dpdk_vhost_reconfigure_helper(struct netdev_dpdk *dev) >> OVS_REQUIRES(dev->mutex) >> { >> + int old_n_txq = dev->up.n_txq; >> + int old_n_rxq = dev->up.n_rxq; >> + int err; >> + >> dev->up.n_txq = dev->requested_n_txq; >> dev->up.n_rxq = dev->requested_n_rxq; >> - int err; >> >> /* Always keep RX queue 0 enabled for implementations that won't >> * report vring states. */ >> @@ -5063,6 +5162,34 @@ dpdk_vhost_reconfigure_helper(struct netdev_dpdk *dev) >> >> netdev_dpdk_remap_txqs(dev); >> >> + if (dev->up.n_txq != old_n_txq) { >> + struct netdev_dpdk_vhost_q_stats *old_txq_stats, *new_txq_stats; >> + >> + new_txq_stats = xcalloc(dev->up.n_txq, sizeof *dev->sw_stats->txq); >> + rte_spinlock_lock(&dev->stats_lock); >> + old_txq_stats = dev->sw_stats->txq; >> + memcpy(new_txq_stats, old_txq_stats, >> + MIN(dev->up.n_txq, old_n_txq) * sizeof *dev->sw_stats->txq); >> + dev->sw_stats->txq = new_txq_stats; > > I prefer realloc and setting extra space to 0, but it is equivalent, > no strong opinion on the implementation. I initialy did it the same way you suggest, but then thought it would be better to avoid calling xrealloc() with the spinlock held. What do you think? > >> + rte_spinlock_unlock(&dev->stats_lock); >> + free(old_txq_stats); >> + >> + } >> + >> + if (dev->up.n_rxq != old_n_rxq) { >> + struct netdev_dpdk_vhost_q_stats *old_rxq_stats, *new_rxq_stats; >> + >> + new_rxq_stats = xcalloc(dev->up.n_rxq, sizeof *dev->sw_stats->rxq); >> + rte_spinlock_lock(&dev->stats_lock); >> + old_rxq_stats = dev->sw_stats->rxq; >> + memcpy(new_rxq_stats, old_rxq_stats, >> + MIN(dev->up.n_rxq, old_n_rxq) * sizeof *dev->sw_stats->rxq); >> + dev->sw_stats->rxq = new_rxq_stats; >> + rte_spinlock_unlock(&dev->stats_lock); >> + free(old_rxq_stats); >> + >> + } >> + > > I did not test your patch yet (will do later this week once I get to > the end of this series). > However I think we are missing some reset to 0 for per q stats in > netdev_dpdk_update_flags__. Hmm... I think you're right, good catch! Thanks for the review, Maxime
On Tue, Dec 7, 2021 at 11:11 PM Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > On 12/7/21 21:37, David Marchand wrote: > > On Wed, Nov 24, 2021 at 10:24 PM Maxime Coquelin > > <maxime.coquelin@redhat.com> wrote: > >> > >> HXPS feature will enable steering Tx packets on transmist > > > > transmit* > > > >> queues based on their hashes. In order to test the feature, > > > > "their hashes" is ambiguous. > > s/their hashes/the packets hashes/ ? Yep. > >> @@ -5063,6 +5162,34 @@ dpdk_vhost_reconfigure_helper(struct netdev_dpdk *dev) > >> > >> netdev_dpdk_remap_txqs(dev); > >> > >> + if (dev->up.n_txq != old_n_txq) { > >> + struct netdev_dpdk_vhost_q_stats *old_txq_stats, *new_txq_stats; > >> + > >> + new_txq_stats = xcalloc(dev->up.n_txq, sizeof *dev->sw_stats->txq); > >> + rte_spinlock_lock(&dev->stats_lock); > >> + old_txq_stats = dev->sw_stats->txq; > >> + memcpy(new_txq_stats, old_txq_stats, > >> + MIN(dev->up.n_txq, old_n_txq) * sizeof *dev->sw_stats->txq); > >> + dev->sw_stats->txq = new_txq_stats; > > > > I prefer realloc and setting extra space to 0, but it is equivalent, > > no strong opinion on the implementation. > > I initialy did it the same way you suggest, but then thought it would be > better to avoid calling xrealloc() with the spinlock held. What do you > think? Indeed, your implementation is better.
On Wed, Nov 24, 2021 at 10:24 PM Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > @@ -2385,6 +2408,7 @@ netdev_dpdk_vhost_update_rx_counters(struct netdev_dpdk *dev, > } > > stats->rx_bytes += packet_size; > + q_stats->bytes += packet_size; > } > > if (OVS_UNLIKELY(qos_drops)) { > @@ -2437,7 +2461,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq, > } > > rte_spinlock_lock(&dev->stats_lock); > - netdev_dpdk_vhost_update_rx_counters(dev, batch->packets, > + netdev_dpdk_vhost_update_rx_counters(dev, qid, batch->packets, After some testing, instead of qid here, we should use rxq->queue_id :-) The tx part is ok. > nb_rx, qos_drops); > rte_spinlock_unlock(&dev->stats_lock); > [snip] With this fix, I start a vm with a 4 rxq vhost port. This vhost port has a virtio counterpart bound to the vm virtio-net kmod. After a while, I can see some stats for rx and tx packets: # ovs-vsctl get interface vhost4 statistics | sed -e 's#[{}]##g' -e 's#, #\n#g' | grep -v '=0$' rx_1_to_64_packets=15 rx_256_to_511_packets=25 rx_65_to_127_packets=35 rx_bytes=12185 rx_packets=75 rx_q1_bytes=242 rx_q1_packets=3 rx_q2_bytes=242 rx_q2_packets=3 rx_q3_bytes=11701 rx_q3_packets=69 tx_bytes=2414 tx_packets=20 tx_q0_bytes=2414 tx_q0_packets=20 All good, and per q stats nicely sums to the same value than the "global" stats. I then stop the vm and look at stats: # ovs-vsctl get interface vhost4 statistics | sed -e 's#[{}]##g' -e 's#, #\n#g' | grep -v '=0$' rx_1_to_64_packets=15 rx_256_to_511_packets=26 rx_65_to_127_packets=38 rx_bytes=12776 rx_packets=79 tx_bytes=2414 tx_dropped=1 tx_packets=20 tx_q0_bytes=2414 tx_q0_packets=20 So we end up with misalignment of "global" stats and per q stats in this situation. Can we do something about it? I am unsure about resetting all stats to 0, but it could be the simpler solution...
On 12/8/21 21:25, David Marchand wrote: > On Wed, Nov 24, 2021 at 10:24 PM Maxime Coquelin > <maxime.coquelin@redhat.com> wrote: >> @@ -2385,6 +2408,7 @@ netdev_dpdk_vhost_update_rx_counters(struct netdev_dpdk *dev, >> } >> >> stats->rx_bytes += packet_size; >> + q_stats->bytes += packet_size; >> } >> >> if (OVS_UNLIKELY(qos_drops)) { >> @@ -2437,7 +2461,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq, >> } >> >> rte_spinlock_lock(&dev->stats_lock); >> - netdev_dpdk_vhost_update_rx_counters(dev, batch->packets, >> + netdev_dpdk_vhost_update_rx_counters(dev, qid, batch->packets, > > After some testing, instead of qid here, we should use rxq->queue_id :-) > The tx part is ok. Oops. Yes, it needs to be rxq->queue_id. > >> nb_rx, qos_drops); >> rte_spinlock_unlock(&dev->stats_lock); >> > > [snip] > > > With this fix, I start a vm with a 4 rxq vhost port. > This vhost port has a virtio counterpart bound to the vm virtio-net kmod. > > After a while, I can see some stats for rx and tx packets: > # ovs-vsctl get interface vhost4 statistics | sed -e 's#[{}]##g' -e > 's#, #\n#g' | grep -v '=0$' > rx_1_to_64_packets=15 > rx_256_to_511_packets=25 > rx_65_to_127_packets=35 > rx_bytes=12185 > rx_packets=75 > rx_q1_bytes=242 > rx_q1_packets=3 > rx_q2_bytes=242 > rx_q2_packets=3 > rx_q3_bytes=11701 > rx_q3_packets=69 > tx_bytes=2414 > tx_packets=20 > tx_q0_bytes=2414 > tx_q0_packets=20 > > All good, and per q stats nicely sums to the same value than the "global" stats. > > I then stop the vm and look at stats: > # ovs-vsctl get interface vhost4 statistics | sed -e 's#[{}]##g' -e > 's#, #\n#g' | grep -v '=0$' > rx_1_to_64_packets=15 > rx_256_to_511_packets=26 > rx_65_to_127_packets=38 > rx_bytes=12776 > rx_packets=79 > tx_bytes=2414 > tx_dropped=1 > tx_packets=20 > tx_q0_bytes=2414 > tx_q0_packets=20 > > > So we end up with misalignment of "global" stats and per q stats in > this situation. > Can we do something about it? > > I am unsure about resetting all stats to 0, but it could be the > simpler solution... I would vote for resetting all counters on number of queues changes, both per-queue and global, as discussed off-list. That's the only way to keep consistency between global and per-queue counters. Maxime >
On 12/7/21 21:37, David Marchand wrote: >> +struct netdev_dpdk_vhost_q_stats { >> + uint64_t bytes; >> + uint64_t packets; >> + uint64_t errors; >> +}; >> + >> /* Custom software stats for dpdk ports */ >> struct netdev_dpdk_sw_stats { >> /* No. of retries when unable to transmit. */ >> @@ -206,6 +213,10 @@ struct netdev_dpdk_sw_stats { >> uint64_t rx_qos_drops; >> /* Packet drops in HWOL processing. */ >> uint64_t tx_invalid_hwol_drops; >> + /* Per-queue Vhost Tx stats */ >> + struct netdev_dpdk_vhost_q_stats *txq; >> + /* Per-queue Vhost Rx stats */ >> + struct netdev_dpdk_vhost_q_stats *rxq; > |Here, we add "driver" specific stats, while netdev_dpdk_sw_stats struct > carries OVS "own" stats. This netdev_dpdk_sw_stats struct is converted > by netdev_dpdk_get_sw_custom_stats and there is a small framework on > adding custom OVS stats (using some macros "trick"). I'd rather leave > netdev_dpdk_sw_stats struct untouched for consistency. Pointers to vhost > specific stats can be added to the netdev_dpdk struct (we have some > spare space after the pointer to netdev_dpdk_sw_stats). | I checked with pahole, and the comment stating "/*36 pad bytes here.*/" is outdated. Since it is difficult to maintain it given it needs to be updated when struct netdev_stats is modified, I will just remove it in next revision. Maxime
On Thu, Dec 9, 2021 at 3:51 PM Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > On 12/7/21 21:37, David Marchand wrote: > >> +struct netdev_dpdk_vhost_q_stats { > >> + uint64_t bytes; > >> + uint64_t packets; > >> + uint64_t errors; > >> +}; > >> + > >> /* Custom software stats for dpdk ports */ > >> struct netdev_dpdk_sw_stats { > >> /* No. of retries when unable to transmit. */ > >> @@ -206,6 +213,10 @@ struct netdev_dpdk_sw_stats { > >> uint64_t rx_qos_drops; > >> /* Packet drops in HWOL processing. */ > >> uint64_t tx_invalid_hwol_drops; > >> + /* Per-queue Vhost Tx stats */ > >> + struct netdev_dpdk_vhost_q_stats *txq; > >> + /* Per-queue Vhost Rx stats */ > >> + struct netdev_dpdk_vhost_q_stats *rxq; > > |Here, we add "driver" specific stats, while netdev_dpdk_sw_stats struct > > carries OVS "own" stats. This netdev_dpdk_sw_stats struct is converted > > by netdev_dpdk_get_sw_custom_stats and there is a small framework on > > adding custom OVS stats (using some macros "trick"). I'd rather leave > > netdev_dpdk_sw_stats struct untouched for consistency. Pointers to vhost > > specific stats can be added to the netdev_dpdk struct (we have some > > spare space after the pointer to netdev_dpdk_sw_stats). | > > I checked with pahole, and the comment stating "/*36 pad bytes here.*/" > is outdated. Since it is difficult to maintain it given it needs to be > updated when struct netdev_stats is modified, I will just remove it in > next revision. Yeah... I don't like those comments which become obsolete too easily. +1
On 12/9/21 16:04, David Marchand wrote: > On Thu, Dec 9, 2021 at 3:51 PM Maxime Coquelin > <maxime.coquelin@redhat.com> wrote: >> On 12/7/21 21:37, David Marchand wrote: >>>> +struct netdev_dpdk_vhost_q_stats { >>>> + uint64_t bytes; >>>> + uint64_t packets; >>>> + uint64_t errors; >>>> +}; >>>> + >>>> /* Custom software stats for dpdk ports */ >>>> struct netdev_dpdk_sw_stats { >>>> /* No. of retries when unable to transmit. */ >>>> @@ -206,6 +213,10 @@ struct netdev_dpdk_sw_stats { >>>> uint64_t rx_qos_drops; >>>> /* Packet drops in HWOL processing. */ >>>> uint64_t tx_invalid_hwol_drops; >>>> + /* Per-queue Vhost Tx stats */ >>>> + struct netdev_dpdk_vhost_q_stats *txq; >>>> + /* Per-queue Vhost Rx stats */ >>>> + struct netdev_dpdk_vhost_q_stats *rxq; >>> |Here, we add "driver" specific stats, while netdev_dpdk_sw_stats struct >>> carries OVS "own" stats. This netdev_dpdk_sw_stats struct is converted >>> by netdev_dpdk_get_sw_custom_stats and there is a small framework on >>> adding custom OVS stats (using some macros "trick"). I'd rather leave >>> netdev_dpdk_sw_stats struct untouched for consistency. Pointers to vhost >>> specific stats can be added to the netdev_dpdk struct (we have some >>> spare space after the pointer to netdev_dpdk_sw_stats). | >> >> I checked with pahole, and the comment stating "/*36 pad bytes here.*/" >> is outdated. Since it is difficult to maintain it given it needs to be >> updated when struct netdev_stats is modified, I will just remove it in >> next revision. > > Yeah... I don't like those comments which become obsolete too easily. > +1 For the reference: d9d73f84ea22 ("Revert "dpif_netdev: Refactor dp_netdev_pmd_thread structure."") This one needs a follow up. :) Not sure if you want to go into that rabbit hole right now though. Best regards, Ilya Maximets.
On 12/9/21 17:29, Ilya Maximets wrote: > On 12/9/21 16:04, David Marchand wrote: >> On Thu, Dec 9, 2021 at 3:51 PM Maxime Coquelin >> <maxime.coquelin@redhat.com> wrote: >>> On 12/7/21 21:37, David Marchand wrote: >>>>> +struct netdev_dpdk_vhost_q_stats { >>>>> + uint64_t bytes; >>>>> + uint64_t packets; >>>>> + uint64_t errors; >>>>> +}; >>>>> + >>>>> /* Custom software stats for dpdk ports */ >>>>> struct netdev_dpdk_sw_stats { >>>>> /* No. of retries when unable to transmit. */ >>>>> @@ -206,6 +213,10 @@ struct netdev_dpdk_sw_stats { >>>>> uint64_t rx_qos_drops; >>>>> /* Packet drops in HWOL processing. */ >>>>> uint64_t tx_invalid_hwol_drops; >>>>> + /* Per-queue Vhost Tx stats */ >>>>> + struct netdev_dpdk_vhost_q_stats *txq; >>>>> + /* Per-queue Vhost Rx stats */ >>>>> + struct netdev_dpdk_vhost_q_stats *rxq; >>>> |Here, we add "driver" specific stats, while netdev_dpdk_sw_stats struct >>>> carries OVS "own" stats. This netdev_dpdk_sw_stats struct is converted >>>> by netdev_dpdk_get_sw_custom_stats and there is a small framework on >>>> adding custom OVS stats (using some macros "trick"). I'd rather leave >>>> netdev_dpdk_sw_stats struct untouched for consistency. Pointers to vhost >>>> specific stats can be added to the netdev_dpdk struct (we have some >>>> spare space after the pointer to netdev_dpdk_sw_stats). | >>> >>> I checked with pahole, and the comment stating "/*36 pad bytes here.*/" >>> is outdated. Since it is difficult to maintain it given it needs to be >>> updated when struct netdev_stats is modified, I will just remove it in >>> next revision. >> >> Yeah... I don't like those comments which become obsolete too easily. >> +1 > > For the reference: > d9d73f84ea22 ("Revert "dpif_netdev: Refactor dp_netdev_pmd_thread structure."") > > This one needs a follow up. :) > Not sure if you want to go into that rabbit hole right now though. Right now, not really. :) But I can for sure take the action for the next release. Regards, Maxime > Best regards, Ilya Maximets. >
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index ca92c947a..e80d5b4ab 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -192,6 +192,13 @@ static const struct vhost_device_ops virtio_net_device_ops = .guest_notified = vhost_guest_notified, }; +/* Custome software per-queue stats for Vhost ports */ +struct netdev_dpdk_vhost_q_stats { + uint64_t bytes; + uint64_t packets; + uint64_t errors; +}; + /* Custom software stats for dpdk ports */ struct netdev_dpdk_sw_stats { /* No. of retries when unable to transmit. */ @@ -206,6 +213,10 @@ struct netdev_dpdk_sw_stats { uint64_t rx_qos_drops; /* Packet drops in HWOL processing. */ uint64_t tx_invalid_hwol_drops; + /* Per-queue Vhost Tx stats */ + struct netdev_dpdk_vhost_q_stats *txq; + /* Per-queue Vhost Rx stats */ + struct netdev_dpdk_vhost_q_stats *rxq; }; enum dpdk_dev_type { @@ -1276,6 +1287,13 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, dev->sw_stats = xzalloc(sizeof *dev->sw_stats); dev->sw_stats->tx_retries = (dev->type == DPDK_DEV_VHOST) ? 0 : UINT64_MAX; + if (dev->type == DPDK_DEV_VHOST) { + dev->sw_stats->txq = xcalloc(netdev->n_txq, + sizeof *dev->sw_stats->txq); + dev->sw_stats->rxq = xcalloc(netdev->n_rxq, + sizeof *dev->sw_stats->rxq); + } + return 0; } @@ -2353,17 +2371,21 @@ netdev_dpdk_vhost_update_rx_size_counters(struct netdev_stats *stats, } static inline void -netdev_dpdk_vhost_update_rx_counters(struct netdev_dpdk *dev, +netdev_dpdk_vhost_update_rx_counters(struct netdev_dpdk *dev, int qid, struct dp_packet **packets, int count, int qos_drops) { struct netdev_stats *stats = &dev->stats; + struct netdev_dpdk_vhost_q_stats *q_stats = &dev->sw_stats->rxq[qid]; struct dp_packet *packet; unsigned int packet_size; int i; stats->rx_packets += count; + q_stats->packets += count; stats->rx_dropped += qos_drops; + q_stats->errors += qos_drops; + for (i = 0; i < count; i++) { packet = packets[i]; packet_size = dp_packet_size(packet); @@ -2374,6 +2396,7 @@ netdev_dpdk_vhost_update_rx_counters(struct netdev_dpdk *dev, * further processing. */ stats->rx_errors++; stats->rx_length_errors++; + q_stats->errors++; continue; } @@ -2385,6 +2408,7 @@ netdev_dpdk_vhost_update_rx_counters(struct netdev_dpdk *dev, } stats->rx_bytes += packet_size; + q_stats->bytes += packet_size; } if (OVS_UNLIKELY(qos_drops)) { @@ -2437,7 +2461,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq, } rte_spinlock_lock(&dev->stats_lock); - netdev_dpdk_vhost_update_rx_counters(dev, batch->packets, + netdev_dpdk_vhost_update_rx_counters(dev, qid, batch->packets, nb_rx, qos_drops); rte_spinlock_unlock(&dev->stats_lock); @@ -2551,7 +2575,7 @@ netdev_dpdk_filter_packet_len(struct netdev_dpdk *dev, struct rte_mbuf **pkts, } static inline void -netdev_dpdk_vhost_update_tx_counters(struct netdev_dpdk *dev, +netdev_dpdk_vhost_update_tx_counters(struct netdev_dpdk *dev, int qid, struct dp_packet **packets, int attempted, struct netdev_dpdk_sw_stats *sw_stats_add) @@ -2561,14 +2585,20 @@ netdev_dpdk_vhost_update_tx_counters(struct netdev_dpdk *dev, sw_stats_add->tx_failure_drops + sw_stats_add->tx_invalid_hwol_drops; struct netdev_stats *stats = &dev->stats; + struct netdev_dpdk_vhost_q_stats *q_stats = &dev->sw_stats->txq[qid]; int sent = attempted - dropped; int i; stats->tx_packets += sent; + q_stats->packets += sent; stats->tx_dropped += dropped; + q_stats->errors += dropped; for (i = 0; i < sent; i++) { - stats->tx_bytes += dp_packet_size(packets[i]); + uint64_t bytes = dp_packet_size(packets[i]); + + stats->tx_bytes += bytes; + q_stats->bytes += bytes; } if (OVS_UNLIKELY(dropped || sw_stats_add->tx_retries)) { @@ -2656,7 +2686,7 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, sw_stats_add.tx_retries = MIN(retries, max_retries); rte_spinlock_lock(&dev->stats_lock); - netdev_dpdk_vhost_update_tx_counters(dev, pkts, total_packets, + netdev_dpdk_vhost_update_tx_counters(dev, qid, pkts, total_packets, &sw_stats_add); rte_spinlock_unlock(&dev->stats_lock); @@ -3286,6 +3316,72 @@ netdev_dpdk_get_sw_custom_stats(const struct netdev *netdev, return 0; } +static int +netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev, + struct netdev_custom_stats *custom_stats) +{ + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); + int sw_stats_size, i, j; + + netdev_dpdk_get_sw_custom_stats(netdev, custom_stats); + + ovs_mutex_lock(&dev->mutex); + + sw_stats_size = custom_stats->size; + custom_stats->size += netdev->n_rxq * sizeof(*dev->sw_stats->rxq) / + sizeof(uint64_t); + custom_stats->size += netdev->n_txq * sizeof(*dev->sw_stats->txq) / + sizeof(uint64_t); + + custom_stats->counters = xrealloc(custom_stats->counters, + custom_stats->size * + sizeof *custom_stats->counters); + + j = 0; + for (i = 0; i < netdev->n_rxq; i++) { + snprintf(custom_stats->counters[sw_stats_size + j++].name, + NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_bytes", i); + snprintf(custom_stats->counters[sw_stats_size + j++].name, + NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_packets", i); + snprintf(custom_stats->counters[sw_stats_size + j++].name, + NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_errors", i); + } + + for (i = 0; i < netdev->n_txq; i++) { + snprintf(custom_stats->counters[sw_stats_size + j++].name, + NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_bytes", i); + snprintf(custom_stats->counters[sw_stats_size + j++].name, + NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_packets", i); + snprintf(custom_stats->counters[sw_stats_size + j++].name, + NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_errors", i); + } + + rte_spinlock_lock(&dev->stats_lock); + + j = 0; + for (i = 0; i < netdev->n_rxq; i++) { + struct netdev_dpdk_vhost_q_stats *rxq_stats = &dev->sw_stats->rxq[i]; + + custom_stats->counters[sw_stats_size + j++].value = rxq_stats->bytes; + custom_stats->counters[sw_stats_size + j++].value = rxq_stats->packets; + custom_stats->counters[sw_stats_size + j++].value = rxq_stats->errors; + } + + for (i = 0; i < netdev->n_txq; i++) { + struct netdev_dpdk_vhost_q_stats *txq_stats = &dev->sw_stats->txq[i]; + + custom_stats->counters[sw_stats_size + j++].value = txq_stats->bytes; + custom_stats->counters[sw_stats_size + j++].value = txq_stats->packets; + custom_stats->counters[sw_stats_size + j++].value = txq_stats->errors; + } + + rte_spinlock_unlock(&dev->stats_lock); + + ovs_mutex_unlock(&dev->mutex); + + return 0; +} + static int netdev_dpdk_get_features(const struct netdev *netdev, enum netdev_features *current, @@ -5043,9 +5139,12 @@ static int dpdk_vhost_reconfigure_helper(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex) { + int old_n_txq = dev->up.n_txq; + int old_n_rxq = dev->up.n_rxq; + int err; + dev->up.n_txq = dev->requested_n_txq; dev->up.n_rxq = dev->requested_n_rxq; - int err; /* Always keep RX queue 0 enabled for implementations that won't * report vring states. */ @@ -5063,6 +5162,34 @@ dpdk_vhost_reconfigure_helper(struct netdev_dpdk *dev) netdev_dpdk_remap_txqs(dev); + if (dev->up.n_txq != old_n_txq) { + struct netdev_dpdk_vhost_q_stats *old_txq_stats, *new_txq_stats; + + new_txq_stats = xcalloc(dev->up.n_txq, sizeof *dev->sw_stats->txq); + rte_spinlock_lock(&dev->stats_lock); + old_txq_stats = dev->sw_stats->txq; + memcpy(new_txq_stats, old_txq_stats, + MIN(dev->up.n_txq, old_n_txq) * sizeof *dev->sw_stats->txq); + dev->sw_stats->txq = new_txq_stats; + rte_spinlock_unlock(&dev->stats_lock); + free(old_txq_stats); + + } + + if (dev->up.n_rxq != old_n_rxq) { + struct netdev_dpdk_vhost_q_stats *old_rxq_stats, *new_rxq_stats; + + new_rxq_stats = xcalloc(dev->up.n_rxq, sizeof *dev->sw_stats->rxq); + rte_spinlock_lock(&dev->stats_lock); + old_rxq_stats = dev->sw_stats->rxq; + memcpy(new_rxq_stats, old_rxq_stats, + MIN(dev->up.n_rxq, old_n_rxq) * sizeof *dev->sw_stats->rxq); + dev->sw_stats->rxq = new_rxq_stats; + rte_spinlock_unlock(&dev->stats_lock); + free(old_rxq_stats); + + } + err = netdev_dpdk_mempool_configure(dev); if (!err) { /* A new mempool was created or re-used. */ @@ -5467,7 +5594,7 @@ static const struct netdev_class dpdk_vhost_class = { .send = netdev_dpdk_vhost_send, .get_carrier = netdev_dpdk_vhost_get_carrier, .get_stats = netdev_dpdk_vhost_get_stats, - .get_custom_stats = netdev_dpdk_get_sw_custom_stats, + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, .get_status = netdev_dpdk_vhost_user_get_status, .reconfigure = netdev_dpdk_vhost_reconfigure, .rxq_recv = netdev_dpdk_vhost_rxq_recv, @@ -5483,7 +5610,7 @@ static const struct netdev_class dpdk_vhost_client_class = { .send = netdev_dpdk_vhost_send, .get_carrier = netdev_dpdk_vhost_get_carrier, .get_stats = netdev_dpdk_vhost_get_stats, - .get_custom_stats = netdev_dpdk_get_sw_custom_stats, + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, .get_status = netdev_dpdk_vhost_user_get_status, .reconfigure = netdev_dpdk_vhost_client_reconfigure, .rxq_recv = netdev_dpdk_vhost_rxq_recv,
HXPS feature will enable steering Tx packets on transmist queues based on their hashes. In order to test the feature, it is needed to be able to get the per-queue statistics for Vhost-user ports. This patch introduces "bytes", "packets" and "error" per-queue custom statistics for Vhost-user ports. Suggested-by David Marchand <david.marchand@redhat.com> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- lib/netdev-dpdk.c | 143 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 135 insertions(+), 8 deletions(-)