DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@amd.com>
To: Nole Zhang <peng.zhang@corigine.com>,
	Chaoyong He <chaoyong.he@corigine.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Cc: oss-drivers <oss-drivers@corigine.com>,
	Niklas Soderlund <niklas.soderlund@corigine.com>,
	"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [PATCH] net/nfp: set the appropriate initialized value of flbufsz
Date: Tue, 18 Oct 2022 11:42:39 +0100	[thread overview]
Message-ID: <a66fbd1f-9544-c7ec-b2de-5618bd7da48a@amd.com> (raw)
In-Reply-To: <BL0PR13MB4450FBC1EF605CD702E53784EF289@BL0PR13MB4450.namprd13.prod.outlook.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)
>>>>>>>
>>>>>
>>>
> 


  reply	other threads:[~2022-10-18 10:42 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-10  6:48 Chaoyong He
2022-10-13 12:00 ` Ferruh Yigit
2022-10-15  7:38   ` Nole Zhang
2022-10-17 12:11     ` Ferruh Yigit
2022-10-18  1:41       ` Nole Zhang
2022-10-18  7:51         ` Ferruh Yigit
2022-10-18  8:51           ` Nole Zhang
2022-10-18  9:36             ` Ferruh Yigit
2022-10-18  9:50               ` Nole Zhang
2022-10-18 10:42                 ` Ferruh Yigit [this message]
2022-10-21  6:27 ` [PATCH v2] net/nfp: ensure the MTU can work Chaoyong He
2022-10-21  9:59   ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a66fbd1f-9544-c7ec-b2de-5618bd7da48a@amd.com \
    --to=ferruh.yigit@amd.com \
    --cc=chaoyong.he@corigine.com \
    --cc=dev@dpdk.org \
    --cc=niklas.soderlund@corigine.com \
    --cc=oss-drivers@corigine.com \
    --cc=peng.zhang@corigine.com \
    --cc=stable@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).