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 66BD14331F; Mon, 13 Nov 2023 23:47:40 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EEAB74026C; Mon, 13 Nov 2023 23:47:39 +0100 (CET) Received: from mail-qt1-f173.google.com (mail-qt1-f173.google.com [209.85.160.173]) by mails.dpdk.org (Postfix) with ESMTP id C93DA4026C for ; Mon, 13 Nov 2023 23:47:37 +0100 (CET) Received: by mail-qt1-f173.google.com with SMTP id d75a77b69052e-421ae866bc2so116251cf.0 for ; Mon, 13 Nov 2023 14:47:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1699915657; x=1700520457; 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=PEMXEu41PN5oMoeay1DFz47yaROkswoi7PwM4aq8IY0=; b=xs5ZJamOwbx+QpfhTeL7Vmm0SD+WGmAjYplORJFcFCe2ce2nRWjmSeRVF3ENARZ2cZ Bxuif0UKeinw7PM57phjls8E3CCcgU2qf8MVTpnLSLv7IFleK2UaUsYRY1pVs0UHQmu+ ayt3tnM3jgrl+c2+C2etCOnmFNnmhbH2wOi4H+zrub2x/MtYsbP1cm/3x4PPZVkcgR7j p17V4ClLG8bGLG9/0gNQdt/dxSdJ+jNxidRou3VXQDO2XU67wWcV82aBlNu5Y9G4dlAa ljc0d43KNfI4mKyvEcSQmjs4GQlU7OMBXYvT+DltcO6N6csZboolKdM/QfRJPETkBWUD n9GA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699915657; x=1700520457; 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=PEMXEu41PN5oMoeay1DFz47yaROkswoi7PwM4aq8IY0=; b=n1tWOaihXSdHdBRG0upXVhOV9bjXHpQFLvZodfJYOzs+J3zkq8k8a26BDAFmD1iBaj 7XX8V24vfe2XspNs8+aQd5mJ3eGKSANqccyAPBjdXF9chcQTX0Iivs5pkLtuPFXZjBTI j1n1LLrFsm5JrDrtp9YLSjeEIeFNZd7i5AnEA7iQSqFNnDS2kt3FnDfyVoTVNS4Y1q7k KjxccvSv7iD/I1kKUCVXxvyzFaX0NiAwQX0KaJ6Nsx8QfasiHkD1Uj8rETbT1Mf88eRt qo4w1BU1x79v4mQp61HQi3yoX8wF/vTwKnnW2vyKiBCxYQTFHQk03EgTdCHKNT+IZqJ3 w31A== X-Gm-Message-State: AOJu0YwhsptWj19uIyPH95rpncccdMY4V+QLmxJx0jxZd4GKRkmpqaUu ejm4QNAq0slj5SW3C7PbFP/TF45lUk0OKgSgws/Stw== X-Google-Smtp-Source: AGHT+IHyC4RqJovDyGMh5Yl8ZfTA5a419wiAKPqO9SopoNp19ye/m1hx9fgRAmAcFM8SEGkXvpVDh17k7ryIVSKpfRA= X-Received: by 2002:a05:622a:34a:b0:421:b3b5:7589 with SMTP id r10-20020a05622a034a00b00421b3b57589mr34366qtw.19.1699915656880; Mon, 13 Nov 2023 14:47:36 -0800 (PST) MIME-Version: 1.0 References: <20231111003410.2950594-1-joshwash@google.com> <31e8f62f-c0f9-4d16-92b0-cec08d73cee0@amd.com> In-Reply-To: <31e8f62f-c0f9-4d16-92b0-cec08d73cee0@amd.com> From: Joshua Washington Date: Mon, 13 Nov 2023 14:47:24 -0800 Message-ID: Subject: Re: [PATCH] net/gve: fix RX buffer size alignment To: Ferruh Yigit Cc: Junfeng Guo , Jeroen de Borst , Rushil Gupta , Xiaoyun Li , dev@dpdk.org, stable@dpdk.org Content-Type: multipart/alternative; boundary="000000000000e0c122060a10748f" 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 --000000000000e0c122060a10748f Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hello Ferruh, But I am not clear with what is "minimum required by DPDK", since > application can provide smaller mbufs. > Also not clear why this alignment cause problem only with mbuf size > bigger than 2048 + 128 bytes. Can you please clarify? > My apologies, the statement "minimum required by DPDK" is a typo; it should probably say "minimum recommended", per https://doc.dpdk.org/api/rte__mbuf__core_8h.html#a185c46bcdbfa90f6c50a4b037= a93313f. The GVE GQ driver is one which requires a packet buffer size of at least 2K. This alignment issue manifests in a different way when the mbuf size is smaller than the minimum supported by the device, but it is an issue nonetheless. I will fix the wording in the commit description in an updated patch. When 'dev_info->min_rx_bufsize' set correctly, above check should be > done in ethdev level, can you please check 'rte_eth_check_rx_mempool()'. > This validation path does seem to be hit when running testpmd: # dpdk-testpmd -- -a --stats-period=3D1 --forward-mode=3Dtxonly --rxq=3D$N --txq=3D$N --nb-cores=3D$(($N + 1)) --mbuf-size=3D10 ... mb_pool_0 mbuf_data_room_size 10 < 1152 (128 + 1024) Fail to configure port 0 rx queues Port 0 is closed EAL: Error - exiting with code: 1 Cause: Start ports failed I can remove this check from the driver, as it is redundant. Just for your info, this release 'dev_info.max_rx_bufsize' and ethdev > layer note added [1] if user provides mbuf size bigger than this value. > Ethdev layer not is mainly for memmory optimization, but above check is > required for driver. > > [1] > > https://git.dpdk.org/dpdk/commit/?id=3D75c7849a9dcca356985fdb87f2d11cae13= 5dfb1a If I were to add GVE support for max buffer size to this patch, how would that interact with backports? Is it possible to include only parts of a patch in a backport? On Fri, Nov 10, 2023 at 8:18=E2=80=AFPM Ferruh Yigit = wrote: > On 11/11/2023 12:34 AM, Joshua Washington wrote: > > In GVE, both queue formats have RX buffer size alignment requirements > > which are not respected whenever the mbuf size is greater than the > > minimum required by DPDK (2048 + 128). > > > > Hi Joshua, > > We don't have a way to inform application about the alignment > requirement, so drivers enforces these as you are doing in this patch. > > But I am not clear with what is "minimum required by DPDK", since > application can provide smaller mbufs. > Also not clear why this alignment cause problem only with mbuf size > bigger than 2048 + 128 bytes. Can you please clarify? > > > This causes the driver to break > > silently in initialization, and no queues are created, leading to no > > network traffic. > > > > This change aims to remedy this by restricting the RX receive buffer > > sizes to valid sizes for their respective queue formats. > > > > Fixes: 4bec2d0b5572 ("net/gve: support queue operations") > > Fixes: 1dc00f4fc74b ("net/gve: add Rx queue setup for DQO") > > Cc: junfeng.guo@intel.com > > Cc: stable@dpdk.org > > > > Signed-off-by: Joshua Washington > > Reviewed-by: Rushil Gupta > > <...> > > > @@ -337,6 +343,20 @@ gve_clear_device_rings_ok(struct gve_priv *priv) > > &priv->state_flags); > > } > > > > +static inline int > > +gve_validate_rx_buffer_size(struct gve_priv *priv, uint16_t > rx_buffer_size) > > +{ > > + uint16_t min_rx_buffer_size =3D gve_is_gqi(priv) ? > > + GVE_RX_MIN_BUF_SIZE_GQI : GVE_RX_MIN_BUF_SIZE_DQO; > > + if (rx_buffer_size < min_rx_buffer_size) { > > + PMD_DRV_LOG(ERR, "mbuf size must be at least %hu bytes", > > + min_rx_buffer_size); > > + return -EINVAL; > > + } > > + > > > > When 'dev_info->min_rx_bufsize' set correctly, above check should be > done in ethdev level, can you please check 'rte_eth_check_rx_mempool()'. > > > > + return 0; > > +} > > + > > int > > gve_rx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_id, uint16_= t > nb_desc, > > unsigned int socket_id, const struct rte_eth_rxconf > *conf, > > diff --git a/drivers/net/gve/gve_rx.c b/drivers/net/gve/gve_rx.c > > index b8c92ccda0..0049c6428d 100644 > > --- a/drivers/net/gve/gve_rx.c > > +++ b/drivers/net/gve/gve_rx.c > > @@ -301,6 +301,7 @@ gve_rx_queue_setup(struct rte_eth_dev *dev, uint16_= t > queue_id, > > const struct rte_memzone *mz; > > struct gve_rx_queue *rxq; > > uint16_t free_thresh; > > + uint32_t mbuf_len; > > int err =3D 0; > > > > if (nb_desc !=3D hw->rx_desc_cnt) { > > @@ -344,7 +345,14 @@ gve_rx_queue_setup(struct rte_eth_dev *dev, > uint16_t queue_id, > > rxq->hw =3D hw; > > rxq->ntfy_addr =3D > &hw->db_bar2[rte_be_to_cpu_32(hw->irq_dbs[rxq->ntfy_id].id)]; > > > > - rxq->rx_buf_len =3D rte_pktmbuf_data_room_size(rxq->mpool) - > RTE_PKTMBUF_HEADROOM; > > + mbuf_len =3D > > + rte_pktmbuf_data_room_size(rxq->mpool) - > RTE_PKTMBUF_HEADROOM; > > + err =3D gve_validate_rx_buffer_size(hw, mbuf_len); > > + if (err) > > + goto err_rxq; > > + rxq->rx_buf_len =3D > > + RTE_MIN((uint16_t)GVE_RX_MAX_BUF_SIZE_GQI, > > + RTE_ALIGN_FLOOR(mbuf_len, GVE_RX_BUF_ALIGN_GQI)); > > > > Just for your info, this release 'dev_info.max_rx_bufsize' and ethdev > layer note added [1] if user provides mbuf size bigger than this value. > Ethdev layer not is mainly for memmory optimization, but above check is > required for driver. > > [1] > > https://git.dpdk.org/dpdk/commit/?id=3D75c7849a9dcca356985fdb87f2d11cae13= 5dfb1a > > --=20 Joshua Washington | Software Engineer | joshwash@google.com | (414) 366-442= 3 --000000000000e0c122060a10748f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello Ferruh,

But I am not clear with what is "minimum required = by DPDK", since
application can provide smaller mbufs.
Also not = clear why this alignment cause problem only with mbuf size
bigger than 2= 048 + 128 bytes. Can you please clarify?

My apologies, the statement "minimum required by DPDK" is a typ= o; it should probably say "minimum recommended", per=C2=A0https://doc.dpdk.org/api/rte__mbuf__core_8h.html#a185c46bcdbfa= 90f6c50a4b037a93313f. The GVE=C2=A0GQ driver is one which requires a pa= cket buffer size of at least 2K. This alignment issue manifests in a differ= ent way when the mbuf size is smaller than the minimum supported by the dev= ice, but it is an issue nonetheless. I will fix the wording in the commit d= escription in an updated patch.

When 'dev_info->min_rx_bufsize' set c= orrectly, above check should be
done in ethdev level, can you please che= ck 'rte_eth_check_rx_mempool()'.

This validation path does seem to be hit when running testpmd:
=
# dpdk-testpmd -- -a --stats-period=3D1 = --forward-mode=3Dtxonly --rxq=3D$N --txq=3D$N --nb-cores=3D$(($N + 1)) --mb= uf-size=3D10
...
mb_pool_0 mbuf_data_room_size 10 < 1152 (128 + 1024)=
Fail to configure port 0 rx queues=
Port 0 is closed
EAL: Error - exiting with code: 1
=C2=A0 Cause: = Start ports failed

I can remove this check from t= he driver, as it is redundant.

Just for your info, this release 'dev_= info.max_rx_bufsize' and ethdev
layer note added [1] if user provide= s mbuf size bigger than this value.
Ethdev layer not is mainly for memmo= ry optimization, but above check is
required for driver.

[1]
<= a href=3D"https://git.dpdk.org/dpdk/commit/?id=3D75c7849a9dcca356985fdb87f2= d11cae135dfb1a" rel=3D"noreferrer" target=3D"_blank">https://git.dpdk.org/d= pdk/commit/?id=3D75c7849a9dcca356985fdb87f2d11cae135dfb1a
<= div>
If I were to add GVE support for max buffer size to this= patch, how would that interact=C2=A0with backports? Is it possible to incl= ude only parts of a patch in a backport?



On Fri, Nov 10, 2023 at 8:18=E2=80=AFPM Ferruh Yigit <ferruh.yigit@amd.com>= wrote:
On 11/11= /2023 12:34 AM, Joshua Washington wrote:
> In GVE, both queue formats have RX buffer size alignment requirements<= br> > which are not respected whenever the mbuf size is greater than the
> minimum required by DPDK (2048 + 128).
>

Hi Joshua,

We don't have a way to inform application about the alignment
requirement, so drivers enforces these as you are doing in this patch.

But I am not clear with what is "minimum required by DPDK", since=
application can provide smaller mbufs.
Also not clear why this alignment cause problem only with mbuf size
bigger than 2048 + 128 bytes. Can you please clarify?

> This causes the driver to break
> silently in initialization, and no queues are created, leading to no > network traffic.
>
> This change aims to remedy this by restricting the RX receive buffer > sizes to valid sizes for their respective queue formats.
>
> Fixes: 4bec2d0b5572 ("net/gve: support queue operations") > Fixes: 1dc00f4fc74b ("net/gve: add Rx queue setup for DQO")<= br> > Cc: junfeng= .guo@intel.com
> Cc: stable@dpdk.o= rg
>
> Signed-off-by: Joshua Washington <joshwash@google.com>
> Reviewed-by: Rushil Gupta <rushilg@google.com>

<...>

> @@ -337,6 +343,20 @@ gve_clear_device_rings_ok(struct gve_priv *priv)<= br> >=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=A0 =C2=A0&priv->state_flags);
>=C2=A0 }
>=C2=A0
> +static inline int
> +gve_validate_rx_buffer_size(struct gve_priv *priv, uint16_t rx_buffer= _size)
> +{
> +=C2=A0 =C2=A0 =C2=A0uint16_t min_rx_buffer_size =3D gve_is_gqi(priv) = ?
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0GVE_RX_MIN_BUF_SIZE_G= QI : GVE_RX_MIN_BUF_SIZE_DQO;
> +=C2=A0 =C2=A0 =C2=A0if (rx_buffer_size < min_rx_buffer_size) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0PMD_DRV_LOG(ERR, &quo= t;mbuf size must be at least %hu bytes",
> +=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=A0min_rx_buffer_size);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -EINVAL;
> +=C2=A0 =C2=A0 =C2=A0}
> +
>

When 'dev_info->min_rx_bufsize' set correctly, above check shoul= d be
done in ethdev level, can you please check 'rte_eth_check_rx_mempool()&= #39;.


> +=C2=A0 =C2=A0 =C2=A0return 0;
> +}
> +
>=C2=A0 int
>=C2=A0 gve_rx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_id, u= int16_t nb_desc,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 unsigned= int socket_id, const struct rte_eth_rxconf *conf,
> diff --git a/drivers/net/gve/gve_rx.c b/drivers/net/gve/gve_rx.c
> index b8c92ccda0..0049c6428d 100644
> --- a/drivers/net/gve/gve_rx.c
> +++ b/drivers/net/gve/gve_rx.c
> @@ -301,6 +301,7 @@ gve_rx_queue_setup(struct rte_eth_dev *dev, uint16= _t queue_id,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0const struct rte_memzone *mz;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0struct gve_rx_queue *rxq;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0uint16_t free_thresh;
> +=C2=A0 =C2=A0 =C2=A0uint32_t mbuf_len;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0int err =3D 0;
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (nb_desc !=3D hw->rx_desc_cnt) {
> @@ -344,7 +345,14 @@ gve_rx_queue_setup(struct rte_eth_dev *dev, uint1= 6_t queue_id,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0rxq->hw =3D hw;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0rxq->ntfy_addr =3D &hw->db_bar2[rt= e_be_to_cpu_32(hw->irq_dbs[rxq->ntfy_id].id)];
>=C2=A0
> -=C2=A0 =C2=A0 =C2=A0rxq->rx_buf_len =3D rte_pktmbuf_data_room_size= (rxq->mpool) - RTE_PKTMBUF_HEADROOM;
> +=C2=A0 =C2=A0 =C2=A0mbuf_len =3D
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0rte_pktmbuf_data_room= _size(rxq->mpool) - RTE_PKTMBUF_HEADROOM;
> +=C2=A0 =C2=A0 =C2=A0err =3D gve_validate_rx_buffer_size(hw, mbuf_len)= ;
> +=C2=A0 =C2=A0 =C2=A0if (err)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto err_rxq;
> +=C2=A0 =C2=A0 =C2=A0rxq->rx_buf_len =3D
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0RTE_MIN((uint16_t)GVE= _RX_MAX_BUF_SIZE_GQI,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0RTE_ALIGN_FLOOR(mbuf_len, GVE_RX_BUF_ALIGN_GQI));
>

Just for your info, this release 'dev_info.max_rx_bufsize' and ethd= ev
layer note added [1] if user provides mbuf size bigger than this value.
Ethdev layer not is mainly for memmory optimization, but above check is
required for driver.

[1]
https://git.dpdk.org/= dpdk/commit/?id=3D75c7849a9dcca356985fdb87f2d11cae135dfb1a



--

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