[V4,5/5] app/testpmd: stop forwarding in new or destroy event

  When testpmd receives the new or destroy event, the port related
information will be updated. Testpmd must stop packet forwarding
before updating the information to avoid some serious problems.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 app/test-pmd/testpmd.c | 4 ++++
 1 file changed, 4 insertions(+)
  

On 12/6/2022 9:26 AM, Huisong Li wrote:
> When testpmd receives the new or destroy event, the port related
> information will be updated. Testpmd must stop packet forwarding
> before updating the information to avoid some serious problems.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  app/test-pmd/testpmd.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 2e6329c853..746f07652a 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3806,6 +3806,8 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
>  
>  	switch (type) {
>  	case RTE_ETH_EVENT_NEW:
> +		if (test_done == 0)
> +			stop_packet_forwarding();

testpmd is test application, why not prevent user to issue attach /
detach commands when packet forwarding is going on, and force user to
stop forwarding explicitly instead of doing this implicitly and silently?

Similar to previous comments, as we make things more complex for
specific use cases it will be very difficult to update testpmd without
hitting unexpected side effects everywhere, at least this is my concern.

>  		if (setup_on_probe_event)
>  			setup_attached_port(port_id);
>  		break;
> @@ -3816,6 +3818,8 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
>  				"Could not set up deferred device removal\n");
>  		break;
>  	case RTE_ETH_EVENT_DESTROY:
> +		if (test_done == 0)
> +			stop_packet_forwarding();
>  		ports[port_id].port_status = RTE_PORT_CLOSED;
>  		printf("Port %u is closed\n", port_id);
>  		if (rte_eal_alarm_set(100000, remove_invalid_ports_callback,
  

在 2023/1/11 20:52, Ferruh Yigit 写道:
> On 12/6/2022 9:26 AM, Huisong Li wrote:
>> When testpmd receives the new or destroy event, the port related
>> information will be updated. Testpmd must stop packet forwarding
>> before updating the information to avoid some serious problems.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   app/test-pmd/testpmd.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index 2e6329c853..746f07652a 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -3806,6 +3806,8 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
>>   
>>   	switch (type) {
>>   	case RTE_ETH_EVENT_NEW:
>> +		if (test_done == 0)
>> +			stop_packet_forwarding();
> testpmd is test application, why not prevent user to issue attach /
> detach commands when packet forwarding is going on, and force user to
> stop forwarding explicitly instead of doing this implicitly and silently?
For primary process, maybe it is ok by forcing user to stop forwarding 
explicitly.
But for multiple process, it is better to do this before removing or adding.
otherwise there'll be a lot of weird prints.
>
> Similar to previous comments, as we make things more complex for
> specific use cases it will be very difficult to update testpmd without
> hitting unexpected side effects everywhere, at least this is my concern.
Understand your concern. But it's a problem.
>
>>   		if (setup_on_probe_event)
>>   			setup_attached_port(port_id);
>>   		break;
>> @@ -3816,6 +3818,8 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
>>   				"Could not set up deferred device removal\n");
>>   		break;
>>   	case RTE_ETH_EVENT_DESTROY:
>> +		if (test_done == 0)
>> +			stop_packet_forwarding();
>>   		ports[port_id].port_status = RTE_PORT_CLOSED;
>>   		printf("Port %u is closed\n", port_id);
>>   		if (rte_eal_alarm_set(100000, remove_invalid_ports_callback,
> .