On Fri, Dec 17, 2021 at 4:10 PM Maxime Coquelin
<[email protected]> wrote:
>
> Hash-based Tx steering feature will enable steering Tx
> packets on transmit 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 <[email protected]>
> Signed-off-by: Maxime Coquelin <[email protected]>

A couple of nits about a comment and coding style, but otherwise it lgtm.

Reviewed-by: David Marchand <[email protected]>


> @@ -479,9 +486,12 @@ struct netdev_dpdk {
>      PADDED_MEMBERS(CACHE_LINE_SIZE,
>          struct netdev_stats stats;
>          struct netdev_dpdk_sw_stats *sw_stats;
> +        /* Per-queue vhost Tx stats */

Nit: this kind of comment is not really helpful as the field name is
already descriptive.
I would remove it.


> +        struct netdev_dpdk_vhost_q_stats *vhost_txq_stats;
> +        /* Per-queue vhost Rx stats */
> +        struct netdev_dpdk_vhost_q_stats *vhost_rxq_stats;
>          /* Protects stats */
>          rte_spinlock_t stats_lock;
> -        /* 36 pad bytes here. */
>      );
>
>      PADDED_MEMBERS(CACHE_LINE_SIZE,
> @@ -1276,6 +1286,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->vhost_txq_stats = xcalloc(netdev->n_txq,
> +                                     sizeof *dev->vhost_txq_stats);
> +        dev->vhost_rxq_stats = xcalloc(netdev->n_rxq,
> +                                     sizeof *dev->vhost_rxq_stats);
> +    }
> +

Nit: indent is off.


> @@ -3287,6 +3316,76 @@ 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);
> +
> +#define VHOST_Q_STATS \
> +    VHOST_Q_STAT(bytes) \
> +    VHOST_Q_STAT(packets) \
> +    VHOST_Q_STAT(errors)

Nit: I did not find a recommendation in the coding style, but other
macros in this file try to align the trailing \


> +
> +    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++) {
> +#define VHOST_Q_STAT(NAME) \
> +        snprintf(custom_stats->counters[sw_stats_size + j++].name, \
> +                NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_"#NAME, i);

Nit: Indent is off.


> +        VHOST_Q_STATS
> +#undef VHOST_Q_STAT
> +    }
> +
> +    for (i = 0; i < netdev->n_txq; 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);

Nit: Indent is off.

> +        VHOST_Q_STATS
> +#undef VHOST_Q_STAT
> +    }
> +
> +    rte_spinlock_lock(&dev->stats_lock);
> +
> +    j = 0;
> +    for (i = 0; i < netdev->n_rxq; i++) {
> +#define VHOST_Q_STAT(NAME) \
> +        custom_stats->counters[sw_stats_size + j++].value = \
> +                dev->vhost_rxq_stats[i].NAME;

Nit: Indent is off.


> +        VHOST_Q_STATS
> +#undef VHOST_Q_STAT
> +    }
> +
> +    for (i = 0; i < netdev->n_txq; i++) {
> +#define VHOST_Q_STAT(NAME) \
> +        custom_stats->counters[sw_stats_size + j++].value = \
> +                dev->vhost_txq_stats[i].NAME;

Nit: Indent is off.

> +        VHOST_Q_STATS
> +#undef VHOST_Q_STAT
> +    }
> +
> +    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,
> @@ -3556,6 +3655,11 @@ netdev_dpdk_update_flags__(struct netdev_dpdk *dev,
>              if (NETDEV_UP & on) {
>                  rte_spinlock_lock(&dev->stats_lock);
>                  memset(&dev->stats, 0, sizeof dev->stats);
> +                memset(dev->sw_stats, 0, sizeof *dev->sw_stats);
> +                memset(dev->vhost_rxq_stats, 0,
> +                        dev->up.n_rxq * sizeof *dev->vhost_rxq_stats);

Nit: indent is off.


> +                memset(dev->vhost_txq_stats, 0,
> +                        dev->up.n_txq * sizeof *dev->vhost_txq_stats);
>                  rte_spinlock_unlock(&dev->stats_lock);
>              }
>          }



-- 
David Marchand

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to