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#a185c46bcdbfa90f6c50a4b037a93313f. 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=1 --forward-mode=txonly --rxq=$N --txq=$N --nb-cores=$(($N + 1)) --mbuf-size=10 ... 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=75c7849a9dcca356985fdb87f2d11cae135dfb1a 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 PM 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 = 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 = 0; > > > > if (nb_desc != hw->rx_desc_cnt) { > > @@ -344,7 +345,14 @@ gve_rx_queue_setup(struct rte_eth_dev *dev, > uint16_t queue_id, > > rxq->hw = hw; > > rxq->ntfy_addr = > &hw->db_bar2[rte_be_to_cpu_32(hw->irq_dbs[rxq->ntfy_id].id)]; > > > > - rxq->rx_buf_len = rte_pktmbuf_data_room_size(rxq->mpool) - > RTE_PKTMBUF_HEADROOM; > > + mbuf_len = > > + rte_pktmbuf_data_room_size(rxq->mpool) - > RTE_PKTMBUF_HEADROOM; > > + err = gve_validate_rx_buffer_size(hw, mbuf_len); > > + if (err) > > + goto err_rxq; > > + rxq->rx_buf_len = > > + 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=75c7849a9dcca356985fdb87f2d11cae135dfb1a > > -- Joshua Washington | Software Engineer | joshwash@google.com | (414) 366-4423