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

[Ferruh Yigit <ferruh.yigit@amd.com>]

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
`

>>> 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)
>>>>>
>>>
>