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 8356043B53; Tue, 20 Feb 2024 10:41:01 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D0EE2410E7; Tue, 20 Feb 2024 10:41:00 +0100 (CET) Received: from mail-ej1-f50.google.com (mail-ej1-f50.google.com [209.85.218.50]) by mails.dpdk.org (Postfix) with ESMTP id 3290A402D1 for ; Tue, 20 Feb 2024 10:40:59 +0100 (CET) Received: by mail-ej1-f50.google.com with SMTP id a640c23a62f3a-a3e5d82ad86so286699766b.2 for ; Tue, 20 Feb 2024 01:40:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1708422059; x=1709026859; 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=8g54210pbxdsddsBYGRYnnzrXLYgpnAMaxj3xuPGV/s=; b=tikFj/SpgJuKsehkRXUDvl8MST7z1wWoFDz1rLNiXxRoCWxmL4wCEM4ymwGs04qANl o8GwjrEV3Xo/i6Y5ldftPTJUjtNpxsXyE+050im9ioGbqK2WX/mhBLhTpy4TSKDf30t5 1MZ4LLP8Id+fjEnRBJh3qHezVejJ/ZVNYCwrnmRcrHrqW6+VNQp2KEbxR/YXJiguBBuw /g3n1NNtgmhfGZfzGvy2zxXOMVSlM2s88oneUy/kq2i/BgziBwdozwWKdvaxKXvOoIdo jTT+iYaG+tj8JgcOfiTEuSf9CFgKr4tLHEXZYpx5L2lCZpG7ZsLsXXcm4wUtWxKpwLuK 7m1w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708422059; x=1709026859; 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=8g54210pbxdsddsBYGRYnnzrXLYgpnAMaxj3xuPGV/s=; b=uK+GIXHfJc4r8G2P/eJxC248bT+sfijWCkIAd30b2GO9LufEuRdah4Lqyworegu7cP ylmdFfnA+rAJ4oJzFy1iAWW5gQNG1sCPU3zcma/QDHm25F/GqOd1TYvAr7RzrY04IxWf iVldhZB5c8FyQpspWDvXfoy4gx7XqpOEyrH26NeHGus/W064cTZYQL1t4JTgYxqUOvA9 zfCA8q1r3e05k/iV/6hucnfTgoTWWLxOMisCE34BTP73LZKgB1TC27f7Ok7ttvc+iNfI H3PyUQFmJjrnQ3fYSEsHGxqOCnWMbJuGuuWfdxfkNtcDCkmZhlmPtVH4nhsd8GoM48vS bc7g== X-Forwarded-Encrypted: i=1; AJvYcCUiDQ3NvMVTV4IfAvEvEe9HJ8pOE/EIIJwk1h8YnHbjXUOuDlUleMW5Cuyl55KFw4tYEkejoRILONhHuFI= X-Gm-Message-State: AOJu0YyEOsqPbtI5P4bXwwQKLgiLjb8p4tO3gOyl/2U0TlFZl4/qU2Hw NBpiCjZsdnrQZpdKfJaiL3zPvbv6+ERd/G+JcxpSla9kfRwVq1TY0SYecHIJUIxu9ZJh4KJAqB6 IVzjQ81S0OWNjQPEOegCMzSwWGavyEAF2W7bV X-Google-Smtp-Source: AGHT+IHov5UJivP5rYMA/eN69fw8sg1jG8y0XoX3KTlSI8ZbcEHSyEC7QN4F206yvt09NaP8I/B0QQHwIExzHTybGXY= X-Received: by 2002:a17:906:af8d:b0:a3e:cf42:bc68 with SMTP id mj13-20020a170906af8d00b00a3ecf42bc68mr3132530ejb.30.1708422058599; Tue, 20 Feb 2024 01:40:58 -0800 (PST) MIME-Version: 1.0 References: <20240219024436.1010010-1-rushilg@google.com> <5295dc14-7b01-4855-86b5-6039adbdac1c@amd.com> In-Reply-To: <5295dc14-7b01-4855-86b5-6039adbdac1c@amd.com> From: Rushil Gupta Date: Tue, 20 Feb 2024 15:10:46 +0530 Message-ID: Subject: Re: [PATCH] net/gve: Change ERR to DEBUG to prevent flooding of logs for Tx-Dqo. To: Ferruh Yigit Cc: "Guo, Junfeng" , Jeroen de Borst , Joshua Washington , dev@dpdk.org, stable@dpdk.org Content-Type: multipart/alternative; boundary="000000000000ee42fd0611cd019b" 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 --000000000000ee42fd0611cd019b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable These are very useful insights Ferruh. I think RTE_LOG_DP() is something that would have been more suitable here. Also, the DEBUG statement combined with a statistic will be more useful than ERR from developer perspective if they see a potential memory leak in their program. On Mon, Feb 19, 2024, 3:03=E2=80=AFPM Ferruh Yigit w= rote: > On 2/19/2024 2:44 AM, Rushil Gupta wrote: > > This was causing failure for testpmd runs (for queues >=3D15) > > presumably due to flooding of logs due to descriptor ring being > > overwritten. > > > > Fixes: a01854 ("net/gve: fix dqo bug for chained descriptors") > > Cc: stable@dpdk.org > > > > Signed-off-by: Rushil Gupta > > Reviewed-by: Joshua Washington > > --- > > drivers/net/gve/gve_tx_dqo.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/gve/gve_tx_dqo.c b/drivers/net/gve/gve_tx_dqo.= c > > index 1a8eb96ea9..30a1455b20 100644 > > --- a/drivers/net/gve/gve_tx_dqo.c > > +++ b/drivers/net/gve/gve_tx_dqo.c > > @@ -116,7 +116,7 @@ gve_tx_burst_dqo(void *tx_queue, struct rte_mbuf > **tx_pkts, uint16_t nb_pkts) > > first_sw_id =3D sw_id; > > do { > > if (sw_ring[sw_id] !=3D NULL) > > - PMD_DRV_LOG(ERR, "Overwriting an entry in > sw_ring"); > > + PMD_DRV_LOG(DEBUG, "Overwriting an entry > in sw_ring"); > > > > txd =3D &txr[tx_id]; > > sw_ring[sw_id] =3D tx_pkt; > > Hi Rushil, > > I will continue with this patch, BUT > logging in the datapath has potential problems like this, also it may > have performance impact even log is not printed, because of additional > check in the log function. > > For datapath, you can prefer: > - Add log macros controlled by RTE_ETHDEV_DEBUG_RX & RTE_ETHDEV_DEBUG_TX > flags > - Or use RTE_LOG_DP() macro which is compiled out if default log level > is less than the log uses > > > Also another perspective for the logs, when end-user observes this bloat > of logs, what she can do? > Can she change some driver arguments or environment variables to fix the > issue, if not what is the point of the log? > If this is a condition that can occur dynamically based on received > traffic and user doesn't have control on it, maybe driver should handle > the error without log? > And if this is a log for driver developer, perhaps it should be assert > or similar that is disabled in the release build? > --000000000000ee42fd0611cd019b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
These are very useful insights Ferruh. I think RTE_L= OG_DP() is something that would have been more suitable here.
Also, the DEBUG statement combined with a statistic will be more = useful than ERR from developer perspective if they see a potential memory l= eak in their program.

On Mon, Feb 19, 2024, 3:03=E2=80=AFPM Ferru= h Yigit <ferruh.yigit@amd.com> wrote:
On 2/19/2024 2:44 AM, Rushil Gupta wrote:
> This was causing failure for testpmd runs (for queues >=3D15)
> presumably due to flooding of logs due to descriptor ring being
> overwritten.
>
> Fixes: a01854 ("net/gve: fix dqo bug for chained descriptors"= ;)
> Cc: stable@dpdk.org
>
> Signed-off-by: Rushil Gupta <rushilg@google.com><= br> > Reviewed-by: Joshua Washington <joshwash@google.com= >
> ---
>=C2=A0 drivers/net/gve/gve_tx_dqo.c | 2 +-
>=C2=A0 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/gve/gve_tx_dqo.c b/drivers/net/gve/gve_tx_dqo= .c
> index 1a8eb96ea9..30a1455b20 100644
> --- a/drivers/net/gve/gve_tx_dqo.c
> +++ b/drivers/net/gve/gve_tx_dqo.c
> @@ -116,7 +116,7 @@ gve_tx_burst_dqo(void *tx_queue, struct rte_mbuf *= *tx_pkts, uint16_t nb_pkts)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0first_sw_id =3D = sw_id;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0do {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0if (sw_ring[sw_id] !=3D NULL)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0PMD_DRV_LOG(ERR, "Overwriting an en= try in sw_ring");
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0PMD_DRV_LOG(DEBUG, "Overwriting an = entry in sw_ring");
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0txd =3D &txr[tx_id];
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0sw_ring[sw_id] =3D tx_pkt;

Hi Rushil,

I will continue with this patch, BUT
logging in the datapath has potential problems like this, also it may
have performance impact even log is not printed, because of additional
check in the log function.

For datapath, you can prefer:
- Add log macros controlled by RTE_ETHDEV_DEBUG_RX & RTE_ETHDEV_DEBUG_T= X
flags
- Or use RTE_LOG_DP() macro which is compiled out if default log level
is less than the log uses


Also another perspective for the logs, when end-user observes this bloat of logs, what she can do?
Can she change some driver arguments or environment variables to fix the issue, if not what is the point of the log?
If this is a condition that can occur dynamically based on received
traffic and user doesn't have control on it, maybe driver should handle=
the error without log?
And if this is a log for driver developer, perhaps it should be assert
or similar that is disabled in the release build?
--000000000000ee42fd0611cd019b--