diff mbox series

net/nfp: set the appropriate initialized value of flbufsz

Message ID 1665384495-24990-1-git-send-email-chaoyong.he@corigine.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series net/nfp: set the appropriate initialized value of flbufsz |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/intel-Testing success Testing PASS

Commit Message

Chaoyong He Oct. 10, 2022, 6:48 a.m. UTC 
  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.

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

Comments

Ferruh Yigit Oct. 13, 2022, noon UTC | #1 
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?

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?


> 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)
Nole Zhang Oct. 15, 2022, 7:38 a.m. UTC | #2 
> 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.

> 
> 
> > 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)
Ferruh Yigit Oct. 17, 2022, 12:11 p.m. UTC | #3 
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.

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?


>>
>>
>>> 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)
>
Nole Zhang Oct. 18, 2022, 1:41 a.m. UTC | #4 
> 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)
> >
Ferruh Yigit Oct. 18, 2022, 7:51 a.m. UTC | #5 
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)
>>>
>
Nole Zhang Oct. 18, 2022, 8:51 a.m. UTC | #6 
> 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.
> >>
> >> 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()`.

> > 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)
> >>>
> >
Ferruh Yigit Oct. 18, 2022, 9:36 a.m. UTC | #7 
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)
>>>>>
>>>
>
Nole Zhang Oct. 18, 2022, 9:50 a.m. UTC | #8 
> -----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)
> >>>>>
> >>>
> >
Ferruh Yigit Oct. 18, 2022, 10:42 a.m. UTC | #9 
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)
>>>>>>>
>>>>>
>>>
>
diff mbox series

Patch

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)