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 8AC35438D1; Tue, 16 Jan 2024 07:18:22 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0FF0A4029F; Tue, 16 Jan 2024 07:18:22 +0100 (CET) Received: from mail-lf1-f42.google.com (mail-lf1-f42.google.com [209.85.167.42]) by mails.dpdk.org (Postfix) with ESMTP id 84C794027D for ; Tue, 16 Jan 2024 07:18:20 +0100 (CET) Received: by mail-lf1-f42.google.com with SMTP id 2adb3069b0e04-50ea9daac4cso9666515e87.3 for ; Mon, 15 Jan 2024 22:18:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1705385900; x=1705990700; 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=SaJD1jRDLZ9UoUQlrEvlZjiSMRyWeAblInxGGzQkRg8=; b=L5NZHdAlqN6kSXMr6ZC9dHGTAxHNKuNGtv1fzVRj1dEksUFgztH1Cv6LAinWRWcEEI iJGrnbzjSOvTggQjlqXhx5SNSJwLxY/KPcifP++2txrtTHdXwHsJHXL2dBlc237M+bsA 2BjWMASTNVH2Ltmgj/vV33L2p6vIC4JpI6OAbRsnEzG+eJbs6wZz7a66SWZ9pN9H1WDH losbepzpu2Rc44suCvjX/7E/UiToqXeoy/74SfF92RKCFlicAzdsvaMEAj/mwIn4NNAv JDXCbr/HBW09/TvdS7ggr2ZJO0j1sxNEg7lRqvtkbsk7vhrd1lsXkGjooyLf8mZvrHCt TaVQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705385900; x=1705990700; 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=SaJD1jRDLZ9UoUQlrEvlZjiSMRyWeAblInxGGzQkRg8=; b=fShDFg9r1zs+opJtVqta7TFcQWv/qP2WGpcqeV0pTW18WbjQ3OQ5YEG6LNlxYTFe5t qMjh1At3V4TBTu3GXVTHWCb9ks9I9VR1tMiq3IIAjZxsZjR2OnKipvne08cGgUM1eCfo ERBdjtuJCVq7Plg2RfZyNQqAB92DmFl+aJjQM56rKpf20dOmy6agAVmbiMzQSxSv0Qx7 WDY5BtbbHZeus77psxxDAQ/IuCPwlUmkwTtMBvN8tR6qwHwVnnwjcVNd5CQMCvTe39FS gr81QWWqk3phXWOThm2St8hwGXAhjaGM7IOddYuYy4llsHB+P8m5qiiPGkmQFCYy/6S+ GTlw== X-Gm-Message-State: AOJu0YyZSPoxEHi0bdK93jrGtsPF1gpCNHAV8GXXGe7oBH3WbAFweTI2 ejhTOxTPCS3tcDULBcrkTLnK7/td6vroMUksVBNYazjWdVyc X-Google-Smtp-Source: AGHT+IEEAM89Xu0MTWpZHLmMGg9j7SW0xO/m5zCDENm4hdcjiU+QkbJZrmGWLj5AcE5OblWc8X4qG67NcT4GCoFJ048= X-Received: by 2002:a05:6512:2256:b0:50e:76e7:b1fc with SMTP id i22-20020a056512225600b0050e76e7b1fcmr3623442lfu.0.1705385899658; Mon, 15 Jan 2024 22:18:19 -0800 (PST) MIME-Version: 1.0 References: <20231222153953.1266615-1-rushilg@google.com> <9a9e3d4c-f0f1-4192-bd92-1b4a7a2c9c20@amd.com> In-Reply-To: <9a9e3d4c-f0f1-4192-bd92-1b4a7a2c9c20@amd.com> From: Rushil Gupta Date: Tue, 16 Jan 2024 11:48:07 +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="000000000000c1b408060f0a18d7" 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 --000000000000c1b408060f0a18d7 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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_N= UM) > * > > + nb_tx_queues; > > + rx_stats_cnt =3D (GVE_RX_STATS_REPORT_NUM + NIC_RX_STATS_REPORT_N= UM) > * > > + 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 *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 update= d. > 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. --000000000000c1b408060f0a18d7 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Fri, Jan 12, 2024 at 8:36=E2=80=AF= PM Ferruh Yigit <ferruh.yigit@am= d.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.
--000000000000c1b408060f0a18d7--