<div dir="ltr">Hello Ferruh,<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">But I am not clear with what is "minimum required by DPDK", since<br>application can provide smaller mbufs.<br>Also not clear why this alignment cause problem only with mbuf size<br>bigger than 2048 + 128 bytes. Can you please clarify?<br></blockquote><div><br></div><div>My apologies, the statement "minimum required by DPDK" is a typo; it should probably say "minimum recommended", per <a href="https://doc.dpdk.org/api/rte__mbuf__core_8h.html#a185c46bcdbfa90f6c50a4b037a93313f">https://doc.dpdk.org/api/rte__mbuf__core_8h.html#a185c46bcdbfa90f6c50a4b037a93313f</a>. 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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">When 'dev_info->min_rx_bufsize' set correctly, above check should be<br>done in ethdev level, can you please check 'rte_eth_check_rx_mempool()'.<br></blockquote><div><br></div><div>This validation path does seem to be hit when running testpmd:</div><div><br></div><font face="monospace"># dpdk-testpmd -- -a --stats-period=1 --forward-mode=txonly --rxq=$N --txq=$N --nb-cores=$(($N + 1)) --mbuf-size=10</font><div><font face="monospace">...</font></div><div><font face="monospace">mb_pool_0 mbuf_data_room_size 10 < 1152 (128 + 1024)</font><br><div><font face="monospace">Fail to configure port 0 rx queues<br>Port 0 is closed<br>EAL: Error - exiting with code: 1<br> Cause: Start ports failed</font><br></div><div><font face="monospace"><br></font></div><div><font face="arial, sans-serif">I can remove this check from the driver, as it is redundant.</font></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Just for your info, this release 'dev_info.max_rx_bufsize' and ethdev<br>layer note added [1] if user provides mbuf size bigger than this value.<br>Ethdev layer not is mainly for memmory optimization, but above check is<br>required for driver.<br><br>[1]<br><a href="https://git.dpdk.org/dpdk/commit/?id=75c7849a9dcca356985fdb87f2d11cae135dfb1a" rel="noreferrer" target="_blank">https://git.dpdk.org/dpdk/commit/?id=75c7849a9dcca356985fdb87f2d11cae135dfb1a</a></blockquote><div><br></div><div>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?</div><div><br></div><div><br></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Nov 10, 2023 at 8:18 PM Ferruh Yigit <<a href="mailto:ferruh.yigit@amd.com" target="_blank">ferruh.yigit@amd.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 11/11/2023 12:34 AM, Joshua Washington wrote:<br> > In GVE, both queue formats have RX buffer size alignment requirements<br> > which are not respected whenever the mbuf size is greater than the<br> > minimum required by DPDK (2048 + 128).<br> ><br> <br> Hi Joshua,<br> <br> We don't have a way to inform application about the alignment<br> requirement, so drivers enforces these as you are doing in this patch.<br> <br> But I am not clear with what is "minimum required by DPDK", since<br> application can provide smaller mbufs.<br> Also not clear why this alignment cause problem only with mbuf size<br> bigger than 2048 + 128 bytes. Can you please clarify?<br> <br> > This causes the driver to break<br> > silently in initialization, and no queues are created, leading to no<br> > network traffic.<br> > <br> > This change aims to remedy this by restricting the RX receive buffer<br> > sizes to valid sizes for their respective queue formats.<br> > <br> > Fixes: 4bec2d0b5572 ("net/gve: support queue operations")<br> > Fixes: 1dc00f4fc74b ("net/gve: add Rx queue setup for DQO")<br> > Cc: <a href="mailto:junfeng.guo@intel.com" target="_blank">junfeng.guo@intel.com</a><br> > Cc: <a href="mailto:stable@dpdk.org" target="_blank">stable@dpdk.org</a><br> > <br> > Signed-off-by: Joshua Washington <<a href="mailto:joshwash@google.com" target="_blank">joshwash@google.com</a>><br> > Reviewed-by: Rushil Gupta <<a href="mailto:rushilg@google.com" target="_blank">rushilg@google.com</a>><br> <br> <...><br> <br> > @@ -337,6 +343,20 @@ gve_clear_device_rings_ok(struct gve_priv *priv)<br> > &priv->state_flags);<br> > }<br> > <br> > +static inline int<br> > +gve_validate_rx_buffer_size(struct gve_priv *priv, uint16_t rx_buffer_size)<br> > +{<br> > + uint16_t min_rx_buffer_size = gve_is_gqi(priv) ?<br> > + GVE_RX_MIN_BUF_SIZE_GQI : GVE_RX_MIN_BUF_SIZE_DQO;<br> > + if (rx_buffer_size < min_rx_buffer_size) {<br> > + PMD_DRV_LOG(ERR, "mbuf size must be at least %hu bytes",<br> > + min_rx_buffer_size);<br> > + return -EINVAL;<br> > + }<br> > +<br> ><br> <br> When 'dev_info->min_rx_bufsize' set correctly, above check should be<br> done in ethdev level, can you please check 'rte_eth_check_rx_mempool()'.<br> <br> <br> > + return 0;<br> > +}<br> > +<br> > int<br> > gve_rx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_id, uint16_t nb_desc,<br> > unsigned int socket_id, const struct rte_eth_rxconf *conf,<br> > diff --git a/drivers/net/gve/gve_rx.c b/drivers/net/gve/gve_rx.c<br> > index b8c92ccda0..0049c6428d 100644<br> > --- a/drivers/net/gve/gve_rx.c<br> > +++ b/drivers/net/gve/gve_rx.c<br> > @@ -301,6 +301,7 @@ gve_rx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_id,<br> > const struct rte_memzone *mz;<br> > struct gve_rx_queue *rxq;<br> > uint16_t free_thresh;<br> > + uint32_t mbuf_len;<br> > int err = 0;<br> > <br> > if (nb_desc != hw->rx_desc_cnt) {<br> > @@ -344,7 +345,14 @@ gve_rx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_id,<br> > rxq->hw = hw;<br> > rxq->ntfy_addr = &hw->db_bar2[rte_be_to_cpu_32(hw->irq_dbs[rxq->ntfy_id].id)];<br> > <br> > - rxq->rx_buf_len = rte_pktmbuf_data_room_size(rxq->mpool) - RTE_PKTMBUF_HEADROOM;<br> > + mbuf_len =<br> > + rte_pktmbuf_data_room_size(rxq->mpool) - RTE_PKTMBUF_HEADROOM;<br> > + err = gve_validate_rx_buffer_size(hw, mbuf_len);<br> > + if (err)<br> > + goto err_rxq;<br> > + rxq->rx_buf_len =<br> > + RTE_MIN((uint16_t)GVE_RX_MAX_BUF_SIZE_GQI,<br> > + RTE_ALIGN_FLOOR(mbuf_len, GVE_RX_BUF_ALIGN_GQI));<br> ><br> <br> Just for your info, this release 'dev_info.max_rx_bufsize' and ethdev<br> layer note added [1] if user provides mbuf size bigger than this value.<br> Ethdev layer not is mainly for memmory optimization, but above check is<br> required for driver.<br> <br> [1]<br> <a href="https://git.dpdk.org/dpdk/commit/?id=75c7849a9dcca356985fdb87f2d11cae135dfb1a" rel="noreferrer" target="_blank">https://git.dpdk.org/dpdk/commit/?id=75c7849a9dcca356985fdb87f2d11cae135dfb1a</a><br> <br> </blockquote></div><br clear="all"><div><br></div><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature"><div dir="ltr"><br><div style="line-height:1.5em;padding-top:10px;margin-top:10px;color:rgb(85,85,85);font-family:sans-serif"><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(213,15,37);padding-top:2px;margin-top:2px">Joshua Washington |</span><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(51,105,232);padding-top:2px;margin-top:2px"> Software Engineer |</span><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(0,153,57);padding-top:2px;margin-top:2px"> <a href="mailto:joshwash@google.com" target="_blank">joshwash@google.com</a> |</span><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(238,178,17);padding-top:2px;margin-top:2px"> <a href="tel:(414)%20366-4423" value="+14143664423" target="_blank">(414) 366-4423</a></span></div><span style="color:rgb(0,0,0);font-family:Tinos;font-size:medium"> </span><br></div></div>