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

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

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: 2022年10月18日 17:36
> To: Nole Zhang <peng.zhang@corigine.com>; Chaoyong He
> <chaoyong.he@corigine.com>; dev@dpdk.org
> Cc: oss-drivers <oss-drivers@corigine.com>; Niklas Soderlund
> <niklas.soderlund@corigine.com>; stable@dpdk.org
> Subject: Re: [PATCH] net/nfp: set the appropriate initialized value of flbufsz
> 
> On 10/18/2022 9:51 AM, Nole Zhang wrote:
> >> On 10/18/2022 2:41 AM, Nole Zhang wrote:
> >>>
> >>>> 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.
> >>
> >> I think it should be 'mbuf-size', since 'hw->flbufsz' is the buffer size.
> >> 'max-pkt-len' is the max Rx frame accepted by NIC, for some devices
> >> 'max- pkt-len' can be bigger than buffer size but it will work fine
> >> because of segmented Rx, but it seems this config doesn't work for nfp.
> >>
> > As you said, maybe the ` NFP_FRAME_SIZE_MAX ` is right, for our nic,
> > the max_rx_pktlen is NFP_FRAME_SIZE_MAX.
> 
> If 'NFP_FRAME_SIZE_MAX' is your HW limit, agree to set it if it will be a
> hardcoded value, please see below.
> 
> >>>>
> >>>> 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()`.
> >>>
> >>
> >> Instead of setting 'hw->flbufsz' in 'nfp_net_init()' and check it in
> >> 'nfp_net_configure()', why not set 'hw->flbufsz' in 'nfp_net_configure()'?
> >>
> >> Because in 'nfp_net_configure()', 'mtu' is a configuration parameter.
> >> It can be possible to convert 'mtu' to frame size and set it. If
> >> there is a HW limitation on buffer size, it can fail in 'nfp_net_configure()'
> when that limit hit.
> >>
> > You mean in the 'nfp_net_configure()', set the value of 'hw->flbufsz',
> > Like `if (rxmode->mtu > hw->flbufsz) {
> > 		hw->flbufsz = rxmode->mtu;
> > 	}`
> > Because the 'nfp_net_configure()' also isn't only called once.
> > If it will be set the values every times, so I first just set the
> > Initial value in the `nfp_net_inital()`.
> >
> 
> 
> But during init, 'nfp_net_init()', you don't know the buffer size, that is why
> you are just setting a hardcoded value, which can be wrong, as you are
> observing now.
> 
> Also thinking twice setting "hw->flbufsz = rxmode->mtu" is not correct, since
> it is not frame size either.
> 
> 
> What about:
> - in 'nfp_net_configure()' change check as:
> if (rxmode->mtu > NFP_FRAME_SIZE_MAX)
> 	return error
> 
> And you may want to save 'mtu' in driver or in device via
> nn_cfg_writel(hw, NFP_NET_CFG_MTU, mtu);
> 
> 
> - in start() add check for buffer size:
> if (mtu > hw->flbufsz)
> 	return error
> 
> - in init() you can either remove setting 'hw->flbufsz' or set it to max
> (NFP_FRAME_SIZE_MAX) as you suggested.
> If you remove setting it in init, you can have logic in configure() as
> `
> bufsize = hw->flbufsz ? hw->flbufsz : NFP_FRAME_SIZE_MAX;
> if (rxmode->mtu > bufsize)
> 	return error
> `
Yes, thanks for your advice.

I will add the 
 - in start() add check for buffer size:
`if (mtu > hw->flbufsz)
	return error`
-in the init() keep the value
`
hw->flbufsz = NFP_FRAME_SIZE_MAX;
`
-in the conigdure() use this check
`
if (rxmode->mtu > NFP_FRAME_SIZE_MAX)
 	return error
`
I will send the v2 after the testing.

Thanks for your review and help again.

> 
> >>> Thanks for your advice.
> >>>>
> >>>>>>
> >>>>>>
> >>>>>>> Fixes: 5c305e218f15 ("net/nfp: fix initialization")
> >>>>>>> Cc: stable@dpdk.org
> > ...
> >>>>>>>          /* VLAN insertion is incompatible with LSOv2 */
> >>>>>>>          if (hw->cap & NFP_NET_CFG_CTRL_LSO2)
> >>>>>
> >>>
> >