[19/20] app/testpmd: reset port status on close notification
diff mbox series

Message ID 20200913220711.3768597-20-thomas@monjalon.net
State Changes Requested
Delegated to: Ferruh Yigit
Headers show
Series
  • cleanup ethdev close operation
Related show

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Thomas Monjalon Sept. 13, 2020, 10:07 p.m. UTC
Since rte_eth_dev_release_port() is called on all port close operations,
the event RTE_ETH_EVENT_DESTROY can be reliably used for resetting
the port status on the application side.

The intermediate state RTE_PORT_HANDLING is removed in close_port()
because a port can also be closed by a PMD in a device remove operation.

In case multiple ports are closed, calling remove_invalid_ports()
only once is enough.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 app/test-pmd/testpmd.c | 34 ++++++++--------------------------
 1 file changed, 8 insertions(+), 26 deletions(-)

Comments

Ferruh Yigit Sept. 23, 2020, 4:45 p.m. UTC | #1
On 9/13/2020 11:07 PM, Thomas Monjalon wrote:
> Since rte_eth_dev_release_port() is called on all port close operations,
> the event RTE_ETH_EVENT_DESTROY can be reliably used for resetting
> the port status on the application side.
> 
> The intermediate state RTE_PORT_HANDLING is removed in close_port()
> because a port can also be closed by a PMD in a device remove operation.
> 
> In case multiple ports are closed, calling remove_invalid_ports()
> only once is enough.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

<...>

> @@ -3118,6 +3093,13 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
>   				rmv_port_callback, (void *)(intptr_t)port_id))
>   			fprintf(stderr, "Could not set up deferred device removal\n");
>   		break;
> +	case RTE_ETH_EVENT_DESTROY:
> +		if (rte_atomic16_cmpset(&(ports[port_id].port_status),
> +					RTE_PORT_STOPPED,
> +					RTE_PORT_CLOSED) == 0)
> +			printf("Port %d cannot be set to closed\n", port_id);
> +		printf("Port %u is closed\n", port_id);
> +		break;

This is failing if a port closed without application port stop command, 
PMD may be doing port stop within the close function but since 
application didn't give the stop command, the port status is not 
'RTE_PORT_STOPPED', hence 'port_status' is not updated correctly.
Thomas Monjalon Sept. 23, 2020, 8:32 p.m. UTC | #2
23/09/2020 18:45, Ferruh Yigit:
> On 9/13/2020 11:07 PM, Thomas Monjalon wrote:
> > Since rte_eth_dev_release_port() is called on all port close operations,
> > the event RTE_ETH_EVENT_DESTROY can be reliably used for resetting
> > the port status on the application side.
> > 
> > The intermediate state RTE_PORT_HANDLING is removed in close_port()
> > because a port can also be closed by a PMD in a device remove operation.
> > 
> > In case multiple ports are closed, calling remove_invalid_ports()
> > only once is enough.
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> 
> <...>
> 
> > @@ -3118,6 +3093,13 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
> >   				rmv_port_callback, (void *)(intptr_t)port_id))
> >   			fprintf(stderr, "Could not set up deferred device removal\n");
> >   		break;
> > +	case RTE_ETH_EVENT_DESTROY:
> > +		if (rte_atomic16_cmpset(&(ports[port_id].port_status),
> > +					RTE_PORT_STOPPED,
> > +					RTE_PORT_CLOSED) == 0)
> > +			printf("Port %d cannot be set to closed\n", port_id);
> > +		printf("Port %u is closed\n", port_id);
> > +		break;
> 
> This is failing if a port closed without application port stop command, 
> PMD may be doing port stop within the close function but since 
> application didn't give the stop command, the port status is not 
> 'RTE_PORT_STOPPED', hence 'port_status' is not updated correctly.

Do you think we should give up with the atomic state transition,
and just assign the state as closed?
Ferruh Yigit Sept. 24, 2020, 12:07 p.m. UTC | #3
On 9/23/2020 9:32 PM, Thomas Monjalon wrote:
> 23/09/2020 18:45, Ferruh Yigit:
>> On 9/13/2020 11:07 PM, Thomas Monjalon wrote:
>>> Since rte_eth_dev_release_port() is called on all port close operations,
>>> the event RTE_ETH_EVENT_DESTROY can be reliably used for resetting
>>> the port status on the application side.
>>>
>>> The intermediate state RTE_PORT_HANDLING is removed in close_port()
>>> because a port can also be closed by a PMD in a device remove operation.
>>>
>>> In case multiple ports are closed, calling remove_invalid_ports()
>>> only once is enough.
>>>
>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>>
>> <...>
>>
>>> @@ -3118,6 +3093,13 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
>>>    				rmv_port_callback, (void *)(intptr_t)port_id))
>>>    			fprintf(stderr, "Could not set up deferred device removal\n");
>>>    		break;
>>> +	case RTE_ETH_EVENT_DESTROY:
>>> +		if (rte_atomic16_cmpset(&(ports[port_id].port_status),
>>> +					RTE_PORT_STOPPED,
>>> +					RTE_PORT_CLOSED) == 0)
>>> +			printf("Port %d cannot be set to closed\n", port_id);
>>> +		printf("Port %u is closed\n", port_id);
>>> +		break;
>>
>> This is failing if a port closed without application port stop command,
>> PMD may be doing port stop within the close function but since
>> application didn't give the stop command, the port status is not
>> 'RTE_PORT_STOPPED', hence 'port_status' is not updated correctly.
> 
> Do you think we should give up with the atomic state transition,
> and just assign the state as closed?
> 

It can be better, if the DESTROY event received it should be in closed 
state.

Right now device can be hot removed while running, or it may be closed 
before it has been stopped, both cases state transition assumption from 
stopped->closed will be wrong, and final state will be wrong.
Thomas Monjalon Sept. 24, 2020, 12:17 p.m. UTC | #4
24/09/2020 14:07, Ferruh Yigit:
> On 9/23/2020 9:32 PM, Thomas Monjalon wrote:
> > 23/09/2020 18:45, Ferruh Yigit:
> >> On 9/13/2020 11:07 PM, Thomas Monjalon wrote:
> >>> Since rte_eth_dev_release_port() is called on all port close operations,
> >>> the event RTE_ETH_EVENT_DESTROY can be reliably used for resetting
> >>> the port status on the application side.
> >>>
> >>> The intermediate state RTE_PORT_HANDLING is removed in close_port()
> >>> because a port can also be closed by a PMD in a device remove operation.
> >>>
> >>> In case multiple ports are closed, calling remove_invalid_ports()
> >>> only once is enough.
> >>>
> >>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> >>
> >> <...>
> >>
> >>> @@ -3118,6 +3093,13 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
> >>>    				rmv_port_callback, (void *)(intptr_t)port_id))
> >>>    			fprintf(stderr, "Could not set up deferred device removal\n");
> >>>    		break;
> >>> +	case RTE_ETH_EVENT_DESTROY:
> >>> +		if (rte_atomic16_cmpset(&(ports[port_id].port_status),
> >>> +					RTE_PORT_STOPPED,
> >>> +					RTE_PORT_CLOSED) == 0)
> >>> +			printf("Port %d cannot be set to closed\n", port_id);
> >>> +		printf("Port %u is closed\n", port_id);
> >>> +		break;
> >>
> >> This is failing if a port closed without application port stop command,
> >> PMD may be doing port stop within the close function but since
> >> application didn't give the stop command, the port status is not
> >> 'RTE_PORT_STOPPED', hence 'port_status' is not updated correctly.
> > 
> > Do you think we should give up with the atomic state transition,
> > and just assign the state as closed?
> > 
> 
> It can be better, if the DESTROY event received it should be in closed 
> state.

I don't understand your proposal.
Note that we are not managing ethdev states here.
Despite the misleading RTE prefix, it is a testpmd state.

> Right now device can be hot removed while running, or it may be closed 
> before it has been stopped, both cases state transition assumption from 
> stopped->closed will be wrong, and final state will be wrong.

I think we should force the state, not matter what it was before.
Ferruh Yigit Sept. 24, 2020, 1:06 p.m. UTC | #5
On 9/24/2020 1:17 PM, Thomas Monjalon wrote:
> 24/09/2020 14:07, Ferruh Yigit:
>> On 9/23/2020 9:32 PM, Thomas Monjalon wrote:
>>> 23/09/2020 18:45, Ferruh Yigit:
>>>> On 9/13/2020 11:07 PM, Thomas Monjalon wrote:
>>>>> Since rte_eth_dev_release_port() is called on all port close operations,
>>>>> the event RTE_ETH_EVENT_DESTROY can be reliably used for resetting
>>>>> the port status on the application side.
>>>>>
>>>>> The intermediate state RTE_PORT_HANDLING is removed in close_port()
>>>>> because a port can also be closed by a PMD in a device remove operation.
>>>>>
>>>>> In case multiple ports are closed, calling remove_invalid_ports()
>>>>> only once is enough.
>>>>>
>>>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>>>>
>>>> <...>
>>>>
>>>>> @@ -3118,6 +3093,13 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
>>>>>     				rmv_port_callback, (void *)(intptr_t)port_id))
>>>>>     			fprintf(stderr, "Could not set up deferred device removal\n");
>>>>>     		break;
>>>>> +	case RTE_ETH_EVENT_DESTROY:
>>>>> +		if (rte_atomic16_cmpset(&(ports[port_id].port_status),
>>>>> +					RTE_PORT_STOPPED,
>>>>> +					RTE_PORT_CLOSED) == 0)
>>>>> +			printf("Port %d cannot be set to closed\n", port_id);
>>>>> +		printf("Port %u is closed\n", port_id);
>>>>> +		break;
>>>>
>>>> This is failing if a port closed without application port stop command,
>>>> PMD may be doing port stop within the close function but since
>>>> application didn't give the stop command, the port status is not
>>>> 'RTE_PORT_STOPPED', hence 'port_status' is not updated correctly.
>>>
>>> Do you think we should give up with the atomic state transition,
>>> and just assign the state as closed?
>>>
>>
>> It can be better, if the DESTROY event received it should be in closed
>> state.
> 
> I don't understand your proposal.
> Note that we are not managing ethdev states here.
> Despite the misleading RTE prefix, it is a testpmd state.
> 

I am aware this is testpmd state, but basically saying same thing, force 
the 'closed' state.

>> Right now device can be hot removed while running, or it may be closed
>> before it has been stopped, both cases state transition assumption from
>> stopped->closed will be wrong, and final state will be wrong.
> 
> I think we should force the state, not matter what it was before.
> 
>

Patch
diff mbox series

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 7842c3b781..31dc97239b 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2694,23 +2694,12 @@  close_port(portid_t pid)
 			continue;
 		}
 
-		if (rte_atomic16_cmpset(&(port->port_status),
-			RTE_PORT_STOPPED, RTE_PORT_HANDLING) == 0) {
-			printf("Port %d is now not stopped\n", pi);
-			continue;
-		}
-
 		if (port->flow_list)
 			port_flow_flush(pi);
 		rte_eth_dev_close(pi);
-
-		remove_invalid_ports();
-
-		if (rte_atomic16_cmpset(&(port->port_status),
-			RTE_PORT_HANDLING, RTE_PORT_CLOSED) == 0)
-			printf("Port %d cannot be set to closed\n", pi);
 	}
 
+	remove_invalid_ports();
 	printf("Done\n");
 }
 
@@ -2841,12 +2830,7 @@  detach_device(struct rte_device *dev)
 		return;
 	}
 	RTE_ETH_FOREACH_DEV_OF(sibling, dev) {
-		/* reset mapping between old ports and removed device */
-		rte_eth_devices[sibling].device = NULL;
 		if (ports[sibling].port_status != RTE_PORT_CLOSED) {
-			/* sibling ports are forced to be closed */
-			ports[sibling].port_status = RTE_PORT_CLOSED;
-			printf("Port %u is closed\n", sibling);
 		}
 	}
 
@@ -2902,11 +2886,8 @@  detach_devargs(char *identifier)
 				return;
 			}
 
-			/* sibling ports are forced to be closed */
 			if (ports[port_id].flow_list)
 				port_flow_flush(port_id);
-			ports[port_id].port_status = RTE_PORT_CLOSED;
-			printf("Port %u is now closed\n", port_id);
 		}
 	}
 
@@ -3055,12 +3036,6 @@  check_all_ports_link_status(uint32_t port_mask)
 	}
 }
 
-/*
- * This callback is for remove a port for a device. It has limitation because
- * it is not for multiple port removal for a device.
- * TODO: the device detach invoke will plan to be removed from user side to
- * eal. And convert all PMDs to free port resources on ether device closing.
- */
 static void
 rmv_port_callback(void *arg)
 {
@@ -3118,6 +3093,13 @@  eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 				rmv_port_callback, (void *)(intptr_t)port_id))
 			fprintf(stderr, "Could not set up deferred device removal\n");
 		break;
+	case RTE_ETH_EVENT_DESTROY:
+		if (rte_atomic16_cmpset(&(ports[port_id].port_status),
+					RTE_PORT_STOPPED,
+					RTE_PORT_CLOSED) == 0)
+			printf("Port %d cannot be set to closed\n", port_id);
+		printf("Port %u is closed\n", port_id);
+		break;
 	default:
 		break;
 	}