RE: [PATCH] net/nfp: set the appropriate initialized value of flbufsz

[Nole Zhang <peng.zhang@corigine.com>]

> On 10/15/2022 8:38 AM, Nole Zhang wrote:
> >   > On 10/10/2022 7:48 AM, Chaoyong He wrote:
> >>> From: Peng Zhang <peng.zhang@corigine.com>
> >>>
> >>> When the testpmd app start-up with parameter max-pkt-len, it will
> >>> set
> >> MTU.
> >>> But the initialized value of flubfsz is inappropriate, if the value
> >>> of flbufsz is smaller than the valude of max-pkt-len, the testpmd
> >>> app will start fail.
> >>>
> >>
> >> What is the failure in the testpmd?
> >
> > The log is as follows:
> > [root@volstruis ~]# dpdk-testpmd --main-lcore 10 -l 10,11,12 -n 4 -a
> > 0000:81:00.0 --socket-mem 2048,2048 --proc-type auto -- --portmask 0x3
> > --nb-cores 2 --rxq 1 --txq 1 --rxd 1024 --txd 1024 --port-topology
> > loop --forward-mode macswap  --max-pkt-len 9216 --mbuf-size 9600
> > --rss-udp --burst=32
> > EAL: Detected CPU lcores: 40
> > EAL: Detected NUMA nodes: 2
> > EAL: Auto-detected process type: PRIMARY
> > EAL: Detected static linkage of DPDK
> > EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
> > EAL: Selected IOVA mode 'VA'
> > EAL: VFIO support initialized
> > EAL: Using IOMMU type 1 (Type 1)
> > EAL: Probe PCI driver: net_nfp_pf (19ee:4000) device: 0000:81:00.0
> > (socket 1) NFP HWINFO header: 48490200
> > TELEMETRY: No legacy callbacks, legacy socket not created Set macswap
> > packet forwarding mode
> > testpmd: create a new mbuf pool <mb_pool_1>: n=163456, size=9600,
> > socket=1
> > testpmd: preferred mempool ops selected: ring_mp_mc Configuring Port 0
> > (socket 1)
> > Port0 dev_configure = -34
> > Fail to configure port 0
> > EAL: Error - exiting with code: 1
> >    Cause: Start ports failed
> >
> > First  in the `nfp_net_configure()`, we will judge the value of MTU and hw-
> >flbufsz, If MTU > hw->flbufsz, it will have the error.
> >
> > And the `--max-pkt-len` is setting the MTU in the initialize process, the
> initialized  value of  hw->flbufsz is just 1500 at first.
> >
> > So if we set the `max-pkt-len`  bigger than the initialized value of flbufsz, It
> will lead the error.
> >
> > Hence we set the new value of hw->flbufsz, it can large the range max-pkt-
> len in the initialized process.
> >
> >
> >>
> >> This patch is fixing something but it is not clear what is fixed, the
> >> concern is it may be changing driver to make something pass in test
> application (testpmd).
> >>
> >> What is 'flubfsz', is it Hw configured frame buffer size?
> >
> >
> > It is configured in the `nfp_net_rx_queue_setup()`{`hw->flbufsz = rxq-
> >mbuf_size`}.
> > If the rxq->mbuf_size < MTU, the MTU can't work.
> >
> 
> It looks like `hw->flbufsz` holds the Rx buffer size, as you highlighted above.
> 
> And you don't want to accept frames bigger than buffer size, since it seems
> driver doesn't support `RTE_ETH_RX_OFFLOAD_SCATTER`, all looks OK.
> 
> 
> According above logic, I agree "hw->flbufsz = RTE_ETHER_MTU;" is wrong,
> but equally `hw->flbufsz = hw->max_mtu;` seems wrong.
> 
> In above command line, it is safe because "mbuf-size=9600" and
> "max-pkt-len=9216", buffer size is bigger than packet size.
Yes, if I need set the hardcoded value, I should set the max max-pkt-len.
> 
> You should able to set `hw->flbufsz` to current buffer size, instead of
> a hardcoded value.
> 
> In `nfp_net_init()`, most probably you don't know the buffer size yet,
> can't you skip setting this value here and set it in
> `nfp_net_rx_queue_setup()` when you know the buffer size?
> 

But If I just depends on the `nfp_net_rx_queue_setup()`, in the `nfp_net_init()`, it will
Call the  `nfp_net_configure()`, it will lead the testpmd start failed, so I add the hardcoded value
in the initialize process. Or  I can remove the judge about  `hw->flbufsz` in the `nfp_net_init()`.

Thanks for your advice.
> 
> >>
> >>
> >>> Fixes: 5c305e218f15 ("net/nfp: fix initialization")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Peng Zhang <peng.zhang@corigine.com>
> >>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> >>> ---
> >>>    drivers/net/nfp/nfp_ethdev.c    | 2 +-
> >>>    drivers/net/nfp/nfp_ethdev_vf.c | 2 +-
> >>>    2 files changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/nfp/nfp_ethdev.c
> >>> b/drivers/net/nfp/nfp_ethdev.c index 5cdd34e588..b95e623f1f 100644
> >>> --- a/drivers/net/nfp/nfp_ethdev.c
> >>> +++ b/drivers/net/nfp/nfp_ethdev.c
> >>> @@ -517,7 +517,7 @@ nfp_net_init(struct rte_eth_dev *eth_dev)
> >>>        hw->cap = nn_cfg_readl(hw, NFP_NET_CFG_CAP);
> >>>        hw->max_mtu = nn_cfg_readl(hw, NFP_NET_CFG_MAX_MTU);
> >>>        hw->mtu = RTE_ETHER_MTU;
> >>> -     hw->flbufsz = RTE_ETHER_MTU;
> >>> +     hw->flbufsz = hw->max_mtu;
> >>>
> >>>        /* VLAN insertion is incompatible with LSOv2 */
> >>>        if (hw->cap & NFP_NET_CFG_CTRL_LSO2) diff --git
> >>> a/drivers/net/nfp/nfp_ethdev_vf.c b/drivers/net/nfp/nfp_ethdev_vf.c
> >>> index d304d78d34..47acb4c60e 100644
> >>> --- a/drivers/net/nfp/nfp_ethdev_vf.c
> >>> +++ b/drivers/net/nfp/nfp_ethdev_vf.c
> >>> @@ -396,7 +396,7 @@ nfp_netvf_init(struct rte_eth_dev *eth_dev)
> >>>        hw->cap = nn_cfg_readl(hw, NFP_NET_CFG_CAP);
> >>>        hw->max_mtu = nn_cfg_readl(hw, NFP_NET_CFG_MAX_MTU);
> >>>        hw->mtu = RTE_ETHER_MTU;
> >>> -     hw->flbufsz = RTE_ETHER_MTU;
> >>> +     hw->flbufsz = hw->max_mtu;
> >>>
> >>>        /* VLAN insertion is incompatible with LSOv2 */
> >>>        if (hw->cap & NFP_NET_CFG_CTRL_LSO2)
> >