DPDK patches and discussions
 help / color / mirror / Atom feed
From: Tom Barbette <barbette@kth.se>
To: Shahaf Shuler <shahafs@mellanox.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: Yongseok Koh <yskoh@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH] mlx5: Report imissed stat
Date: Wed, 14 Nov 2018 16:18:20 +0000
Message-ID: <1542212300053.31332@kth.se> (raw)
In-Reply-To: <DB7PR05MB44269CFE9DDA79B740D138A1C3C30@DB7PR05MB4426.eurprd05.prod.outlook.com>

Hi Shahaf,

Yes we learned this distinction with rx_discards_phy the hard way. I would expect imissed to be only rx_out_of_buffer actually.  I'd say people look at imissed to see if they consume packets fast enough. If the problem is pure CPU power. And that is more the definition of imissed, it is "ie RX queues are full", not "eg".
The *_phy counters are somehow unusual, 82599, xl710 and others would simply never receive other packets. But I'll follow your lead.

If documentation of the limitations is okay for you, I can propose a v2, also adding rx_discards_phy no matter what in xstats as it is needed for the full picture. Then the documentation can mention that.

Tom



________________________________________
De : Shahaf Shuler <shahafs@mellanox.com>
Envoyé : mercredi 14 novembre 2018 16:17
À : Tom Barbette; dev@dpdk.org
Cc : Yongseok Koh
Objet : RE: [PATCH] mlx5: Report imissed stat

Hi Again Tom,

Tuesday, November 13, 2018 12:17 PM, Tom Barbette:
> Subject: [PATCH] mlx5: Report imissed stat
>
> The imissed counters (number of packets dropped because the queues were
> full) were actually reported through xstats as "rx_out_of_buffer"
> but was not reported through stats.
>
> Following a recent discussion on the ML, as there is no way to tell the user if a
> counter is implemented or not, this should be considered a bug. Eg, user
> looking at imissed will think the packets are lost before reaching the device.
>
> As for xstats, I added a base counter to be able to "reset" imissed.
>

Yes, a bit of extra work is needed here.
the full definition of the missed is
"
uint64_t imissed;
/**< Total of RX packets dropped by the HW,
 * because there are no available buffer (i.e. RX queues are full).
 */
"

It is not well defined (this is other topic), because it assumes the only reason to drop packets is because there is no avail buffer on the Rx queue.
It is not entirely correct, for example if you have large latency on your system it is possible that packets would be dropped on the physical port, not because the application is slow and not replenish the buffers fast enough, rather because the NIC is not processing them on the needed rate.

I guess what application seek on imissed is the sum of two. It can be done by summing up two counters: the out_of_buffer and the rx_discard_phy (which exists on ethtool -S <netdev> and need to be added to mlx5_counters_init array).

Still, the output will be incorrect on the following cases:
1. from VF, since VF cannot read the phy port counters
2. in the presence of representors, as they all share the same phy port counter.

I guess if we want to get such patch in, we need first to make the calculation correct, and document the limitations.


> Signed-off-by: Tom Barbette <barbette@kth.se>
> ---
>  drivers/net/mlx5/mlx5.h       |  2 ++
>  drivers/net/mlx5/mlx5_stats.c | 36 +++++++++++++++++++++++-------------
>  2 files changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> a3a34cf..61054a8 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -77,6 +77,8 @@ struct mlx5_xstats_ctrl {
>       /* Index in the device counters table. */
>       uint16_t dev_table_idx[MLX5_MAX_XSTATS];
>       uint64_t base[MLX5_MAX_XSTATS];
> +     /* Base for imissed counter. */
> +     uint64_t imissed_base;
>  };
>
>  /* Flow list . */
> diff --git a/drivers/net/mlx5/mlx5_stats.c b/drivers/net/mlx5/mlx5_stats.c
> index 91f3d47..1e75e85 100644
> --- a/drivers/net/mlx5/mlx5_stats.c
> +++ b/drivers/net/mlx5/mlx5_stats.c
> @@ -119,6 +119,24 @@ static const struct mlx5_counter_ctrl
> mlx5_counters_init[] = {
>
>  static const unsigned int xstats_n = RTE_DIM(mlx5_counters_init);
>
> +static inline void
> +mlx5_read_ib_stat(struct priv *priv, unsigned int idx, uint64_t *stat)
> +{
> +     FILE *file;
> +     MKSTR(path, "%s/ports/1/hw_counters/%s",
> +               priv->ibdev_path,
> +               mlx5_counters_init[idx].ctr_name);
> +
> +     file = fopen(path, "rb");
> +     if (file) {
> +             int n = fscanf(file, "%" SCNu64, stat);
> +
> +             fclose(file);
> +             if (n != 1)
> +                     stat = 0;
> +     }
> +}
> +
>  /**
>   * Read device counters table.
>   *
> @@ -155,19 +173,7 @@ mlx5_read_dev_counters(struct rte_eth_dev *dev,
> uint64_t *stats)
>       }
>       for (i = 0; i != xstats_n; ++i) {
>               if (mlx5_counters_init[i].ib) {
> -                     FILE *file;
> -                     MKSTR(path, "%s/ports/1/hw_counters/%s",
> -                           priv->ibdev_path,
> -                           mlx5_counters_init[i].ctr_name);
> -
> -                     file = fopen(path, "rb");
> -                     if (file) {
> -                             int n = fscanf(file, "%" SCNu64, &stats[i]);
> -
> -                             fclose(file);
> -                             if (n != 1)
> -                                     stats[i] = 0;
> -                     }
> +                     mlx5_read_ib_stat(priv, i, &stats[i]);
>               } else {
>                       stats[i] = (uint64_t)
>                               et_stats->data[xstats_ctrl-
> >dev_table_idx[i]];
> @@ -281,6 +287,7 @@ mlx5_xstats_init(struct rte_eth_dev *dev)
>       if (ret)
>               DRV_LOG(ERR, "port %u cannot read device counters: %s",
>                       dev->data->port_id, strerror(rte_errno));
> +     mlx5_read_ib_stat(priv, 17, &xstats_ctrl->imissed_base);
>  free:
>       rte_free(strings);
>  }
> @@ -389,6 +396,8 @@ mlx5_stats_get(struct rte_eth_dev *dev, struct
> rte_eth_stats *stats)  #endif
>               tmp.oerrors += txq->stats.oerrors;
>       }
> +     mlx5_read_ib_stat(priv, 17, &tmp.imissed);
> +     tmp.imissed -= priv->xstats_ctrl.imissed_base;
>  #ifndef MLX5_PMD_SOFT_COUNTERS
>       /* FIXME: retrieve and add hardware counters. */  #endif @@ -461,6
> +470,7 @@ mlx5_xstats_reset(struct rte_eth_dev *dev)
>       }
>       for (i = 0; i != n; ++i)
>               xstats_ctrl->base[i] = counters[i];
> +     mlx5_read_ib_stat(priv, 17, &xstats_ctrl->imissed_base);
>  }
>
>  /**
> --
> 2.7.4


  reply	other threads:[~2018-11-14 16:18 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-13 10:16 Tom Barbette
2018-11-14 15:17 ` Shahaf Shuler
2018-11-14 16:18   ` Tom Barbette [this message]
2018-11-15  7:39     ` Shahaf Shuler

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1542212300053.31332@kth.se \
    --to=barbette@kth.se \
    --cc=dev@dpdk.org \
    --cc=shahafs@mellanox.com \
    --cc=yskoh@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git