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

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

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.

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

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