app/testpmd: don't allow to dynamic change nbcore
diff mbox series

Message ID 20201027064252.9624-1-zhenghuax.zhou@intel.com
State Accepted, archived
Delegated to: Ferruh Yigit
Headers show
Series
  • app/testpmd: don't allow to dynamic change nbcore
Related show

Checks

Context Check Description
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation fail Compilation issues
ci/travis-robot success Travis build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/checkpatch warning coding style issues

Commit Message

Zhou Zhenghua Oct. 27, 2020, 6:42 a.m. UTC
When changing the number of forwarding cores in runtime, two issues may be
encountered:

  - If the setting nbcore little than current nbcore, the forwarding
    thread will still running on the extra cores. Therefore, trying to
    stop forwarding will hang testpmd, since it will wait for the extra
    cores to stop.

  - If the setting nbcore greate than actual nbcore, the newly added
    cores are not allocated resources.

This will face the problem of dynamic resource allocation, so it's not
allow to changes nbcore number when forwarding is running.

Fixes: 0c0db76f42ed ("app/testpmd: separate forward config setup from display")
Cc: bernard.iremonger@intel.com
Cc: stable@dpdk.org

Signed-off-by: Zhou Zhenghua <zhenghuax.zhou@intel.com>
---
 app/test-pmd/config.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Dharmik Thakkar Oct. 29, 2020, 4:43 a.m. UTC | #1
Hi Zhou,

Although, I see that a few of the configuration options are checking for ’test_done’, I am wondering if it is truly required.
Since, as per the documentation guidelines:
'Configuration changes only become active when forwarding is started/restarted.’

> On Oct 27, 2020, at 1:42 AM, Zhou Zhenghua <zhenghuax.zhou@intel.com> wrote:
> 
> When changing the number of forwarding cores in runtime, two issues may be
> encountered:
> 
>  - If the setting nbcore little than current nbcore, the forwarding
>    thread will still running on the extra cores. Therefore, trying to
>    stop forwarding will hang testpmd, since it will wait for the extra
>    cores to stop.
> 
>  - If the setting nbcore greate than actual nbcore, the newly added
>    cores are not allocated resources.
> 
> This will face the problem of dynamic resource allocation, so it's not
> allow to changes nbcore number when forwarding is running.
> 
> Fixes: 0c0db76f42ed ("app/testpmd: separate forward config setup from display")
> Cc: bernard.iremonger@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Zhou Zhenghua <zhenghuax.zhou@intel.com>
Ferruh Yigit Oct. 30, 2020, 10:46 a.m. UTC | #2
On 10/29/2020 4:43 AM, Dharmik Thakkar wrote:

<...>

>> On Oct 27, 2020, at 1:42 AM, Zhou Zhenghua <zhenghuax.zhou@intel.com> wrote:
>>
>> When changing the number of forwarding cores in runtime, two issues may be
>> encountered:
>>
>>   - If the setting nbcore little than current nbcore, the forwarding
>>     thread will still running on the extra cores. Therefore, trying to
>>     stop forwarding will hang testpmd, since it will wait for the extra
>>     cores to stop.
>>
>>   - If the setting nbcore greate than actual nbcore, the newly added
>>     cores are not allocated resources.
>>
>> This will face the problem of dynamic resource allocation, so it's not
>> allow to changes nbcore number when forwarding is running.
>>
>> Fixes: 0c0db76f42ed ("app/testpmd: separate forward config setup from display")
>> Cc: bernard.iremonger@intel.com
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Zhou Zhenghua <zhenghuax.zhou@intel.com>
> 
 > Hi Zhou,
 >
 > Although, I see that a few of the configuration options are checking for 
’test_done’, I am wondering if it is truly required.
 > Since, as per the documentation guidelines:
 > 'Configuration changes only become active when forwarding is started/restarted.’
 >
Yes documentation has it and I think that is what happens in below bullet 2 
(increase core count).

But it is easy to reproduce the hung mentioned in the first bullet, I am for 
having this fix.
And this won't contradict with the above document, the config options you can 
set will be taken account when forwarding is started/restarted, but this one 
won't be set because of added check, hence:
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

@Zhou,
do you have any objection to remove bullet 2 while merging?
Ferruh Yigit Nov. 2, 2020, 3:20 p.m. UTC | #3
On 10/30/2020 10:46 AM, Ferruh Yigit wrote:
> On 10/29/2020 4:43 AM, Dharmik Thakkar wrote:
> 
> <...>
> 
>>> On Oct 27, 2020, at 1:42 AM, Zhou Zhenghua <zhenghuax.zhou@intel.com> wrote:
>>>
>>> When changing the number of forwarding cores in runtime, two issues may be
>>> encountered:
>>>
>>>   - If the setting nbcore little than current nbcore, the forwarding
>>>     thread will still running on the extra cores. Therefore, trying to
>>>     stop forwarding will hang testpmd, since it will wait for the extra
>>>     cores to stop.
>>>
>>>   - If the setting nbcore greate than actual nbcore, the newly added
>>>     cores are not allocated resources.
>>>
>>> This will face the problem of dynamic resource allocation, so it's not
>>> allow to changes nbcore number when forwarding is running.
>>>
>>> Fixes: 0c0db76f42ed ("app/testpmd: separate forward config setup from display")
>>> Cc: bernard.iremonger@intel.com
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Zhou Zhenghua <zhenghuax.zhou@intel.com>
>>
>  > Hi Zhou,
>  >
>  > Although, I see that a few of the configuration options are checking for 
> ’test_done’, I am wondering if it is truly required.
>  > Since, as per the documentation guidelines:
>  > 'Configuration changes only become active when forwarding is started/restarted.’
>  >
> Yes documentation has it and I think that is what happens in below bullet 2 
> (increase core count).
> 
> But it is easy to reproduce the hung mentioned in the first bullet, I am for 
> having this fix.
> And this won't contradict with the above document, the config options you can 
> set will be taken account when forwarding is started/restarted, but this one 
> won't be set because of added check, hence:
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> @Zhou,
> do you have any objection to remove bullet 2 while merging?

Applied to dpdk-next-net/main, thanks.

Updated the commit log as following:

     app/testpmd: do not allow dynamic change of nbcore

     When the number of forwarding cores changed in runtime, the issue may
     be encountered:
     If the nbcore set little than current nbcore, the forwarding thread
     will still running on the extra cores. Therefore, trying to stop
     forwarding will hang testpmd, since it will wait for the extra cores to
     stop.

     So do not allow to change nbcore number when forwarding is running.

     Fixes: 0c0db76f42ed ("app/testpmd: separate forward config setup from display")
     Cc: stable@dpdk.org

Patch
diff mbox series

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 1668ae323..e9245053d 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3504,6 +3504,10 @@  set_fwd_lcores_mask(uint64_t lcoremask)
 void
 set_fwd_lcores_number(uint16_t nb_lc)
 {
+	if (test_done == 0) {
+		printf("Please stop forwarding first\n");
+		return;
+	}
 	if (nb_lc > nb_cfg_lcores) {
 		printf("nb fwd cores %u > %u (max. number of configured "
 		       "lcores) - ignored\n",