From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 164EA438E6; Wed, 17 Jan 2024 08:07:03 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B0A54402CB; Wed, 17 Jan 2024 08:07:02 +0100 (CET) Received: from mail-il1-f180.google.com (mail-il1-f180.google.com [209.85.166.180]) by mails.dpdk.org (Postfix) with ESMTP id E1C284014F for ; Wed, 17 Jan 2024 08:06:59 +0100 (CET) Received: by mail-il1-f180.google.com with SMTP id e9e14a558f8ab-36192c08ac6so104915ab.0 for ; Tue, 16 Jan 2024 23:06:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1705475219; x=1706080019; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=4Be8fYRC8AS+NxNOyLpkZvCXUGNV/KmKgeaWURrkX5o=; b=molJ1vpi2qpbeTNAyKHTVjjDbqTtSGiCrTzyuqCqDC0HboUO+ScrY6CPGSGi3RA7Ao 0k0If4DHdG2hp0k6WcDzJP/05cutijJZnEVbScR4NnxVMG0T2rg55oOGY1DtWDbksUO5 GxnyZ3b6LnLgWXKkCAEn0TMF8AP9XR3jvytcbAFWx/nnUYkuYm1UEM07GkMkVSc6UBb8 TfELj5r5yGSeIsTca3iVmr/EEzed2Qep0a3bYu8z3IpC6pyaeTPwmXqR2+Yxe/9foS+J KP5kDXc4/UYB+hlBnjY4GbwH4E7w/YI4/NjWLQryw69Vyv5krFelvzNYyeJJKWRpTY/m k7Aw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705475219; x=1706080019; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=4Be8fYRC8AS+NxNOyLpkZvCXUGNV/KmKgeaWURrkX5o=; b=FX5ZhoXIzzHQaSAJw96BC0eispVq4iLBpFfDpALrz+ilH5b/i/jxn6dn63PehEDgzW 0Qkw9wLpSwal+E+XZPft/v2P3AEuhcDP11EJb/HWvVKMhzUC+1pv2y76zFCVyPNpKUzO aq4jSsf8buu7pVG9R68Fjj0uOMx2Mmzre4cqha6Mt8wDeWIt6aMyoCr7C8cg4xXs3Bze KiRGs44Tbq24JdwVPk4po/T4Favo8hDPr4WNnc98cM8A1QQCbYEGM+DufmsTD5Og4NS0 i6UYWPxqhrxXaDu7hLGwUZ7iHURc2twaBP1Hvx2v/gQPDG4SkDd9wAEC+T5kOlPUlC+Y tIXw== X-Gm-Message-State: AOJu0Yy5RvdY1rwm6mSZGfNLeQbgDcaZMqvm96ROZWKptwcGluvTaVSl lMUXvzAJP40FIGACC7z9lqezzK7s3yiUNHUs5qDuMoUR9iXRKVPe8nh4M9ujz1T40pVQpoU6W0N OVIPEkIzN2ZMHxpdsPMjtqfDsoBlO624d0cZW X-Google-Smtp-Source: AGHT+IGmCOk1XwSpFcrDBAw0p4CJa9UMFtyyvo19ID8hOD19GcnQHGd9eJdhNuAVz2Cr1uLeCQbQCyRluvKZRmxV1dI= X-Received: by 2002:a05:6e02:1a2f:b0:360:6b3d:4cdb with SMTP id g15-20020a056e021a2f00b003606b3d4cdbmr77649ile.3.1705475219009; Tue, 16 Jan 2024 23:06:59 -0800 (PST) MIME-Version: 1.0 References: <20231222153953.1266615-1-rushilg@google.com> <9a9e3d4c-f0f1-4192-bd92-1b4a7a2c9c20@amd.com> In-Reply-To: From: Joshua Washington Date: Tue, 16 Jan 2024 23:06:47 -0800 Message-ID: Subject: Re: [PATCH] net/gve: Enable stats reporting for GQ format To: Rushil Gupta Cc: Ferruh Yigit , junfeng.guo@intel.com, jeroendb@google.com, dev@dpdk.org Content-Type: multipart/alternative; boundary="0000000000009aa264060f1ee4b0" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --0000000000009aa264060f1ee4b0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable > > >> <...> >> >> > gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stat= s) >> > { >> > uint16_t i; >> > + if (gve_is_gqi(dev->data->dev_private)) >> > + gve_get_imissed_from_nic(dev); >> > >> >> This updates imissed in RxQ struct for all queues for basic stats, but >> what if user only calls xstats, I guess in that case stat won't be >> updated. >> > > Yes; that is expected. Since imissed is a member of rte_eth_stats; callin= g > gve_dev_stats_get is the right way to get this stat. > I actually think that this should be a counter backed by an xstat as well. As far as I can tell xstats is meant to be a more flexible version of stats, and ultimately should be able to handle a superset of what basic stats can handle, for the purposes of querying, etc. On Mon, Jan 15, 2024 at 10:18=E2=80=AFPM Rushil Gupta = wrote: > > > On Fri, Jan 12, 2024 at 8:36=E2=80=AFPM Ferruh Yigit wrote: > >> On 12/22/2023 3:39 PM, Rushil Gupta wrote: >> > Read from shared region to retrieve imissed statistics for GQ from >> device. >> > Tested using `show port xstats ` in interactive mode. >> > This metric can be triggered by using queues > cores. >> > >> >> Looks good but please check following comments: >> >> Checkpatch gives warning on the patch title, and this patch adds >> 'imissed' support so it can be added to the patch title, something like: >> "net/gve: enable imissed stats for GQ format" >> >> <...> >> >> > +static int gve_alloc_stats_report(struct gve_priv *priv, >> > + uint16_t nb_tx_queues, uint16_t nb_rx_queues) >> > +{ >> > + char z_name[RTE_MEMZONE_NAMESIZE]; >> > + int tx_stats_cnt; >> > + int rx_stats_cnt; >> > + >> > + tx_stats_cnt =3D (GVE_TX_STATS_REPORT_NUM + >> NIC_TX_STATS_REPORT_NUM) * >> > + nb_tx_queues; >> > + rx_stats_cnt =3D (GVE_RX_STATS_REPORT_NUM + >> NIC_RX_STATS_REPORT_NUM) * >> > + nb_rx_queues; >> > + priv->stats_report_len =3D sizeof(struct gve_stats_report) + >> > + sizeof(struct stats) * (tx_stats_cnt + rx_stats_cnt); >> > + >> > + snprintf(z_name, sizeof(z_name), "stats_report_%s", >> priv->pci_dev->device.name); >> > >> >> Can you please add 'gve_' prefix to the memzone name, to prevent any >> possible collision. >> > Done. > >> >> <...> >> >> > +static void gve_free_stats_report(struct rte_eth_dev *dev) >> > +{ >> > + struct gve_priv *priv =3D dev->data->dev_private; >> > + rte_memzone_free(priv->stats_report_mem); >> > >> >> What will happen if user asks stats/xstats after port stopped? >> > Good catch. I have added a null check so that the driver doesn't try to > read stats from memory region that doesn't exist. > >> >> <...> >> >> > gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stat= s) >> > { >> > uint16_t i; >> > + if (gve_is_gqi(dev->data->dev_private)) >> > + gve_get_imissed_from_nic(dev); >> > >> >> This updates imissed in RxQ struct for all queues for basic stats, but >> what if user only calls xstats, I guess in that case stat won't be >> updated. >> > > Yes; that is expected. Since imissed is a member of rte_eth_stats; callin= g > gve_dev_stats_get is the right way to get this stat. > --=20 Joshua Washington | Software Engineer | joshwash@google.com | (414) 366-442= 3 --0000000000009aa264060f1ee4b0 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

<...>

>=C2=A0 gve_dev_stats_get(st= ruct rte_eth_dev *dev, struct rte_eth_stats *stats)
>=C2=A0 {
>= =C2=A0 =C2=A0 =C2=A0 =C2=A0uint16_t i;
> +=C2=A0 =C2=A0 =C2=A0if (gve= _is_gqi(dev->data->dev_private))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0gve_get_imissed_from_nic(dev);
>=C2=A0

Th= is updates imissed in RxQ struct for all queues for basic stats, but
wha= t if user only calls xstats, I guess in that case stat won't be updated= .
=C2=A0
Yes; that is expected. Since= imissed is a member of rte_eth_stats; calling gve_dev_stats_get is the rig= ht way to get this stat.

I actually t= hink that this should be a counter backed by an xstat as well. As far as I = can tell xstats is meant to be a more flexible version of stats, and ultima= tely should be able to handle a superset of what basic stats can handle, fo= r the purposes of querying, etc.

=
On Mon, Jan 15, 2024 at 10:18=E2=80= =AFPM Rushil Gupta <rushilg@google= .com> wrote:


On Fri, Jan 12, 2024 at 8:36=E2= =80=AFPM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
On 12/22/2023 3:39 PM, Rushil Gupta wrote:
> Read from shared region to retrieve imissed statistics for GQ from dev= ice.
> Tested using `show port xstats <port-id>` in interactive mode. > This metric can be triggered by using queues > cores.
>

Looks good but please check following comments:

Checkpatch gives warning on the patch title, and this patch adds
'imissed' support so it can be added to the patch title, something = like:
"net/gve: enable imissed stats for GQ format"

<...>

> +static int gve_alloc_stats_report(struct gve_priv *priv,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0uint16_t nb_tx_queues= , uint16_t nb_rx_queues)
> +{
> +=C2=A0 =C2=A0 =C2=A0char z_name[RTE_MEMZONE_NAMESIZE];
> +=C2=A0 =C2=A0 =C2=A0int tx_stats_cnt;
> +=C2=A0 =C2=A0 =C2=A0int rx_stats_cnt;
> +
> +=C2=A0 =C2=A0 =C2=A0tx_stats_cnt =3D (GVE_TX_STATS_REPORT_NUM + NIC_T= X_STATS_REPORT_NUM) *
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0nb_tx_queues;
> +=C2=A0 =C2=A0 =C2=A0rx_stats_cnt =3D (GVE_RX_STATS_REPORT_NUM + NIC_R= X_STATS_REPORT_NUM) *
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0nb_rx_queues;
> +=C2=A0 =C2=A0 =C2=A0priv->stats_report_len =3D sizeof(struct gve_s= tats_report) +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0sizeof(struct stats) = * (tx_stats_cnt + rx_stats_cnt);
> +
> +=C2=A0 =C2=A0 =C2=A0snprintf(z_name, sizeof(z_name), "stats_repo= rt_%s", priv->pci_dev->device.name);
>

Can you please add 'gve_' prefix to the memzone name, to prevent an= y
possible collision.
Done.=C2=A0

<...>

> +static void gve_free_stats_report(struct rte_eth_dev *dev)
> +{
> +=C2=A0 =C2=A0 =C2=A0struct gve_priv *priv =3D dev->data->dev_pr= ivate;
> +=C2=A0 =C2=A0 =C2=A0rte_memzone_free(priv->stats_report_mem);
>

What will happen if user asks stats/xstats after port stopped?
Good catch. I have added a null check so that the driver doesn'= ;t try to read stats from memory region that doesn't exist.=C2=A0
=

<...>

>=C2=A0 gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats = *stats)
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0uint16_t i;
> +=C2=A0 =C2=A0 =C2=A0if (gve_is_gqi(dev->data->dev_private))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0gve_get_imissed_from_= nic(dev);
>=C2=A0

This updates imissed in RxQ struct for all queues for basic stats, but
what if user only calls xstats, I guess in that case stat won't be upda= ted.
=C2=A0
Yes; that is expected. Since imi= ssed is a member of rte_eth_stats; calling gve_dev_stats_get is the right w= ay to get this stat.


--

Joshua Washington=C2=A0|=C2=A0Software Engineer |=C2=A0joshwash@google.com=C2=A0|=C2=A0(414) 366-4423
=C2=A0
--0000000000009aa264060f1ee4b0--