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 31AEE438FF; Fri, 19 Jan 2024 14:37:35 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F0C7F4029F; Fri, 19 Jan 2024 14:37:34 +0100 (CET) Received: from mail-ed1-f43.google.com (mail-ed1-f43.google.com [209.85.208.43]) by mails.dpdk.org (Postfix) with ESMTP id B5C4B40279 for ; Fri, 19 Jan 2024 14:37:33 +0100 (CET) Received: by mail-ed1-f43.google.com with SMTP id 4fb4d7f45d1cf-559d3fbed6bso879940a12.1 for ; Fri, 19 Jan 2024 05:37:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1705671453; x=1706276253; 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=PHu2KV9AtvR4FrKuNZR/4q6CZmNh+I5Nz6h1gVxzGFc=; b=0UooV+Vdy+vn9j8cuVbdc5WBXSusIUxmUqJ8hbvNR7hbfPyUsx0vVq24Vb8Opa9sx1 fGTfeIbx6ZhQFfXgxzyJslK4NjYgserhRK2Nz/1JP/AunZP6aV6+U39p1KrVgfVmHVcP BF0nktaPmOarsqFKLSP4IDigZ8WU+l5bbMn4+A7p2iKjn5X4zMW295ld5lrzN7PQ7wvt ncEUx7ZdEjmAaejmoV7JiIypQd0/rzDr8B6OtlF/flITPhdfM+c0YIcxJh/vT7XKRBgG cDnRpu+Q6o4YdVL4D+Nz6EWY2F7Bv0YEtVswAy2Kl6JR35h5X1b+ud5XD64RiDdnBeAb Bj4A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705671453; x=1706276253; 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=PHu2KV9AtvR4FrKuNZR/4q6CZmNh+I5Nz6h1gVxzGFc=; b=T6JX9/xtYdugN+XstJJU8RMVgay+LZJIxjw+DPS1uO+XypnhB12VY83cCusgqky/az qCv0TWYZ8eMElrlKdJsJVqU4WsnKVVWYpcxdnUNQYrEKv8ciBqDvKN8KYq8YfL+pR6sV 3NanfsuYlR+I+BMx/iPgQR96V8NMpePoAArODFEhfAmGD3652fa900CJv71RWTcFShiN MFC6Fcs8uTFTJF1+EF2SjN8D8yMg6DF+H9d5sUfHbFYScJ4oSsTHnS6jB/IFUKAaZhng 8xAN1BDJtfhfUt4H53vXq3HQwBEz+x6OnNkW1mx+O0qRPB+TmDeLnL8bRvjcE+tWgq03 C/sQ== X-Gm-Message-State: AOJu0Yz8rUousJtJZHSTpAeGtS/Ntvw6LNsiCSl0/IWV6gOze1qOtb7Z YblX19/W+qg2FJUCCwcF9/hHio8rVukPwAZwUI9a6cTV4g5acZKsGgaVajPTDY22reBdAGQhaDs w3+aB4lepE8UsWEyrwD7zzumya1Eda2ocHJg/ X-Google-Smtp-Source: AGHT+IHafwJK7rd2PuYuH0yy5BU/di2pwjUcELNd3E6Br5VVWHJHq60wFW51dpT0fhz0V9FgJXu1H0ceJ1Md8uMfgSE= X-Received: by 2002:a17:906:d104:b0:a28:e6d8:dd15 with SMTP id b4-20020a170906d10400b00a28e6d8dd15mr970993ejz.33.1705671453150; Fri, 19 Jan 2024 05:37:33 -0800 (PST) MIME-Version: 1.0 References: <20231222153953.1266615-1-rushilg@google.com> <9a9e3d4c-f0f1-4192-bd92-1b4a7a2c9c20@amd.com> <1289a4f1-cdde-41f3-a034-2e325680a5d4@amd.com> In-Reply-To: <1289a4f1-cdde-41f3-a034-2e325680a5d4@amd.com> From: Rushil Gupta Date: Fri, 19 Jan 2024 19:07:20 +0530 Message-ID: Subject: Re: [PATCH] net/gve: Enable stats reporting for GQ format To: Ferruh Yigit Cc: junfeng.guo@intel.com, jeroendb@google.com, joshwash@google.com, dev@dpdk.org Content-Type: multipart/alternative; boundary="000000000000124261060f4c95e7" 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 --000000000000124261060f4c95e7 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Those are fair points. I'll fix this by simply calling gve_get_imissed_from_nic from gve_xstats_get in the v3 patch. On Wed, Jan 17, 2024 at 3:10=E2=80=AFPM Ferruh Yigit = wrote: > On 1/16/2024 6:18 AM, 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 fro= m > > 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 an= y > > 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 > > *stats) > > > { > > > 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; > > calling gve_dev_stats_get is the right way to get this stat. > > > > I don't think it is expected. > xstats contains the basic stats too, if users calls xstats API, > expectation is to get correct values. > > --000000000000124261060f4c95e7 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Those are fair points. I'll fix this by simply calling= =C2=A0gve_get_imissed_from_nic from=C2=A0gve_xstats_get in the v3 patch.
On = Wed, Jan 17, 2024 at 3:10=E2=80=AFPM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
On 1/16/2024 6:18 AM, Rushil Gupta w= rote:
>
>
> On Fri, Jan 12, 2024 at 8:36=E2=80=AFPM Ferruh Yigit <ferruh.yigit@amd.com
> <mailto:f= erruh.yigit@amd.com>> wrote:
>
>=C2=A0 =C2=A0 =C2=A0On 12/22/2023 3:39 PM, Rushil Gupta wrote:
>=C2=A0 =C2=A0 =C2=A0> Read from shared region to retrieve imissed st= atistics for GQ from
>=C2=A0 =C2=A0 =C2=A0device.
>=C2=A0 =C2=A0 =C2=A0> Tested using `show port xstats <port-id>= ` in interactive mode.
>=C2=A0 =C2=A0 =C2=A0> This metric can be triggered by using queues &= gt; cores.
>=C2=A0 =C2=A0 =C2=A0>
>
>=C2=A0 =C2=A0 =C2=A0Looks good but please check following comments:
>
>=C2=A0 =C2=A0 =C2=A0Checkpatch gives warning on the patch title, and th= is patch adds
>=C2=A0 =C2=A0 =C2=A0'imissed' support so it can be added to the= patch title, something like:
>=C2=A0 =C2=A0 =C2=A0"net/gve: enable imissed stats for GQ format&q= uot;
>
>=C2=A0 =C2=A0 =C2=A0<...>
>
>=C2=A0 =C2=A0 =C2=A0> +static int gve_alloc_stats_report(struct gve_= priv *priv,
>=C2=A0 =C2=A0 =C2=A0> +=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=A0> +{
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0char z_name[RTE_MEMZONE_N= AMESIZE];
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0int tx_stats_cnt;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0int rx_stats_cnt;
>=C2=A0 =C2=A0 =C2=A0> +
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0tx_stats_cnt =3D (GVE_TX_= STATS_REPORT_NUM +
>=C2=A0 =C2=A0 =C2=A0NIC_TX_STATS_REPORT_NUM) *
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0nb_tx_queues;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0rx_stats_cnt =3D (GVE_RX_= STATS_REPORT_NUM +
>=C2=A0 =C2=A0 =C2=A0NIC_RX_STATS_REPORT_NUM) *
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0nb_rx_queues;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0priv->stats_report_len= =3D sizeof(struct gve_stats_report) +
>=C2=A0 =C2=A0 =C2=A0> +=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=A0> +
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0snprintf(z_name, sizeof(z= _name), "stats_report_%s",
>=C2=A0 =C2=A0 =C2=A0priv->pci_dev->device.name <http://device.name&g= t;);
>=C2=A0 =C2=A0 =C2=A0>
>
>=C2=A0 =C2=A0 =C2=A0Can you please add 'gve_' prefix to the mem= zone name, to prevent any
>=C2=A0 =C2=A0 =C2=A0possible collision.
>
> Done.=C2=A0
>
>
>=C2=A0 =C2=A0 =C2=A0<...>
>
>=C2=A0 =C2=A0 =C2=A0> +static void gve_free_stats_report(struct rte_= eth_dev *dev)
>=C2=A0 =C2=A0 =C2=A0> +{
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0struct gve_priv *priv =3D= dev->data->dev_private;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0rte_memzone_free(priv->= ;stats_report_mem);
>=C2=A0 =C2=A0 =C2=A0>
>
>=C2=A0 =C2=A0 =C2=A0What will happen if user asks stats/xstats after po= rt stopped?
>
> Good catch. I have added a null check so that the driver doesn't t= ry to
> read stats from memory region that doesn't exist.=C2=A0
>
>
>=C2=A0 =C2=A0 =C2=A0<...>
>
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 gve_dev_stats_get(struct rte_eth_dev *de= v, struct rte_eth_stats
>=C2=A0 =C2=A0 =C2=A0*stats)
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0 =C2=A0uint16_t i;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0if (gve_is_gqi(dev->da= ta->dev_private))
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0gve_get_imissed_from_nic(dev);
>=C2=A0 =C2=A0 =C2=A0>=C2=A0
>
>=C2=A0 =C2=A0 =C2=A0This updates imissed in RxQ struct for all queues f= or basic stats, but
>=C2=A0 =C2=A0 =C2=A0what if user only calls xstats, I guess in that cas= e stat won't be
>=C2=A0 =C2=A0 =C2=A0updated.
>
> =C2=A0
> Yes; that is expected. Since imissed is a member of rte_eth_stats;
> calling gve_dev_stats_get is the right way to get this stat.
>

I don't think it is expected.
xstats contains the basic stats too, if users calls xstats API,
expectation is to get correct values.

--000000000000124261060f4c95e7--