[PATCH] net/gve: fix RX buffer size alignment

[Joshua Washington]

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 <ferruh.yigit at amd.com> 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 at intel.com
> > Cc: stable at dpdk.org
> >
> > Signed-off-by: Joshua Washington <joshwash at google.com>
> > Reviewed-by: Rushil Gupta <rushilg at google.com>
>
> <...>
>
> > @@ -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 at google.com | (414) 366-4423
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mails.dpdk.org/archives/stable/attachments/20231113/69c690f6/attachment.htm>