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

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

On 10/18/2022 10:50 AM, Nole Zhang wrote:
> 
> 
>> -----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
> `

If you will initialize 'hw->flbufsz' as 'NFP_FRAME_SIZE_MAX' in the 
init(), you can keep the check in the configure() intact, up to you.

> I will send the v2 after the testing.
> 

ack, thanks.

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