[V4,4/5] app/testpmd: add attach and detach port for multiple process
Checks
Commit Message
This patch supports attach and detach port in primary and secondary
process.
Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
---
app/test-pmd/testpmd.c | 38 ++++++++++++++++-----------
app/test-pmd/testpmd.h | 1 -
drivers/net/bonding/bonding_testpmd.c | 1 -
3 files changed, 22 insertions(+), 18 deletions(-)
Comments
On 12/6/2022 9:26 AM, Huisong Li wrote:
> This patch supports attach and detach port in primary and secondary
> process.
>
Hi Huisong,
This patch moves port setup and remove (via alarm callback) to event
callback,
1) I can see it is for MP but can you please give more details, what was
the problem before, how this solves the issue
2) I am concerned about doing more work on the event callback and
observe some unexpected side effects later, can't we handle this out of
event callback
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> ---
> app/test-pmd/testpmd.c | 38 ++++++++++++++++-----------
> app/test-pmd/testpmd.h | 1 -
> drivers/net/bonding/bonding_testpmd.c | 1 -
> 3 files changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index bc25703490..2e6329c853 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3463,15 +3463,12 @@ attach_port(char *identifier)
> return;
> }
>
> - /* first attach mode: event */
> - if (setup_on_probe_event) {
> - /* new ports are detected on RTE_ETH_EVENT_NEW event */
> - for (pi = 0; pi < RTE_MAX_ETHPORTS; pi++)
> - if (ports[pi].port_status == RTE_PORT_HANDLING &&
> - ports[pi].need_setup != 0)
> - setup_attached_port(pi);
> + /*
> + * first attach mode: event, setting up attached port is done in
> + * probing callback.
> + */
> + if (setup_on_probe_event)
> return;
> - }
>
> /* second attach mode: iterator */
> RTE_ETH_FOREACH_MATCHING_DEV(pi, identifier, &iterator) {
> @@ -3502,7 +3499,6 @@ setup_attached_port(portid_t pi)
> ports_ids[nb_ports++] = pi;
> fwd_ports_ids[nb_fwd_ports++] = pi;
> nb_cfg_ports = nb_fwd_ports;
> - ports[pi].need_setup = 0;
> ports[pi].port_status = RTE_PORT_STOPPED;
>
> printf("Port %d is attached. Now total ports is %d\n", pi, nb_ports);
> @@ -3536,10 +3532,8 @@ detach_device(struct rte_device *dev)
> TESTPMD_LOG(ERR, "Failed to detach device %s\n", rte_dev_name(dev));
> return;
> }
> - remove_invalid_ports();
>
> printf("Device is detached\n");
> - printf("Now total ports is %d\n", nb_ports);
> printf("Done\n");
> return;
> }
> @@ -3606,11 +3600,9 @@ detach_devargs(char *identifier)
> return;
> }
>
> - remove_invalid_ports();
> -
> printf("Device %s is detached\n", identifier);
> - printf("Now total ports is %d\n", nb_ports);
> printf("Done\n");
> +
> rte_devargs_reset(&da);
> }
>
> @@ -3774,11 +3766,22 @@ rmv_port_callback(void *arg)
> struct rte_device *device = dev_info.device;
> close_port(port_id);
> detach_device(device); /* might be already removed or have more ports */
> + remove_invalid_ports();
> + printf("Now total ports is %d\n", nb_ports);
> }
> if (need_to_start)
> start_packet_forwarding(0);
> }
>
> +static void
> +remove_invalid_ports_callback(void *arg)
> +{
> + RTE_SET_USED(arg);
> +
> + remove_invalid_ports();
> + printf("Now total ports is %d\n", nb_ports);
> +}
> +
> /* This function is used by the interrupt thread */
> static int
> eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
> @@ -3803,8 +3806,8 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
>
> switch (type) {
> case RTE_ETH_EVENT_NEW:
> - ports[port_id].need_setup = 1;
> - ports[port_id].port_status = RTE_PORT_HANDLING;
> + if (setup_on_probe_event)
> + setup_attached_port(port_id);
> break;
> case RTE_ETH_EVENT_INTR_RMV:
> if (rte_eal_alarm_set(100000,
> @@ -3815,6 +3818,9 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
> case RTE_ETH_EVENT_DESTROY:
> 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,
> + (void *)(intptr_t)port_id))
> + fprintf(stderr, "Could not set up deferred device released\n");
> break;
> case RTE_ETH_EVENT_RX_AVAIL_THRESH: {
> uint16_t rxq_id;
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 7d24d25970..080d3a1139 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -306,7 +306,6 @@ struct rte_port {
> uint16_t tx_vlan_id;/**< The tag ID */
> uint16_t tx_vlan_id_outer;/**< The outer tag ID */
> volatile uint16_t port_status; /**< port started or not */
> - uint8_t need_setup; /**< port just attached */
> uint8_t need_reconfig; /**< need reconfiguring port or not */
> uint8_t need_reconfig_queues; /**< need reconfiguring queues or not */
> uint8_t rss_flag; /**< enable rss or not */
> diff --git a/drivers/net/bonding/bonding_testpmd.c b/drivers/net/bonding/bonding_testpmd.c
> index 9529e16fb6..9216271314 100644
> --- a/drivers/net/bonding/bonding_testpmd.c
> +++ b/drivers/net/bonding/bonding_testpmd.c
> @@ -765,7 +765,6 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,
>
> ports[port_id].update_conf = 1;
> ports[port_id].bond_flag = 1;
> - ports[port_id].need_setup = 0;
> ports[port_id].port_status = RTE_PORT_STOPPED;
> }
>
在 2023/1/11 20:51, Ferruh Yigit 写道:
> On 12/6/2022 9:26 AM, Huisong Li wrote:
>> This patch supports attach and detach port in primary and secondary
>> process.
>>
> Hi Huisong,
>
> This patch moves port setup and remove (via alarm callback) to event
> callback,
>
> 1) I can see it is for MP but can you please give more details, what was
> the problem before, how this solves the issue
The port information in testpmd needs to be updated due to attatching
and detaching
port, which are done in the same thread as removing or probing device.
If testpmd don't support multiple process, it is ok. But it can not work
again
for multiple process. For example, if primary process does attatch or
detach a port,
primary and secondary process can receive 'new' or 'destroy' event. And
primary can
update its port information, but secondary will never update its port
information.
So we have to move updating port information to event callback.
The reason for adding an alarm callback is that the ethdev state is set
from 'ATTACHED'
to 'UNUSED' only after the event callback finished. The
remove_invalid_ports() function
removes invalid port according to ethdev state. If we don't add alarm
callback, this
detached port information can not be removed.
>
> 2) I am concerned about doing more work on the event callback and
> observe some unexpected side effects later, can't we handle this out of
> event callback
There's no way, if you don't, the port information in testpmd is all
messed up. That's very bad.
>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> ---
>> app/test-pmd/testpmd.c | 38 ++++++++++++++++-----------
>> app/test-pmd/testpmd.h | 1 -
>> drivers/net/bonding/bonding_testpmd.c | 1 -
>> 3 files changed, 22 insertions(+), 18 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index bc25703490..2e6329c853 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -3463,15 +3463,12 @@ attach_port(char *identifier)
>> return;
>> }
>>
>> - /* first attach mode: event */
>> - if (setup_on_probe_event) {
>> - /* new ports are detected on RTE_ETH_EVENT_NEW event */
>> - for (pi = 0; pi < RTE_MAX_ETHPORTS; pi++)
>> - if (ports[pi].port_status == RTE_PORT_HANDLING &&
>> - ports[pi].need_setup != 0)
>> - setup_attached_port(pi);
>> + /*
>> + * first attach mode: event, setting up attached port is done in
>> + * probing callback.
>> + */
>> + if (setup_on_probe_event)
>> return;
>> - }
>>
>> /* second attach mode: iterator */
>> RTE_ETH_FOREACH_MATCHING_DEV(pi, identifier, &iterator) {
>> @@ -3502,7 +3499,6 @@ setup_attached_port(portid_t pi)
>> ports_ids[nb_ports++] = pi;
>> fwd_ports_ids[nb_fwd_ports++] = pi;
>> nb_cfg_ports = nb_fwd_ports;
>> - ports[pi].need_setup = 0;
>> ports[pi].port_status = RTE_PORT_STOPPED;
>>
>> printf("Port %d is attached. Now total ports is %d\n", pi, nb_ports);
>> @@ -3536,10 +3532,8 @@ detach_device(struct rte_device *dev)
>> TESTPMD_LOG(ERR, "Failed to detach device %s\n", rte_dev_name(dev));
>> return;
>> }
>> - remove_invalid_ports();
>>
>> printf("Device is detached\n");
>> - printf("Now total ports is %d\n", nb_ports);
>> printf("Done\n");
>> return;
>> }
>> @@ -3606,11 +3600,9 @@ detach_devargs(char *identifier)
>> return;
>> }
>>
>> - remove_invalid_ports();
>> -
>> printf("Device %s is detached\n", identifier);
>> - printf("Now total ports is %d\n", nb_ports);
>> printf("Done\n");
>> +
>> rte_devargs_reset(&da);
>> }
>>
>> @@ -3774,11 +3766,22 @@ rmv_port_callback(void *arg)
>> struct rte_device *device = dev_info.device;
>> close_port(port_id);
>> detach_device(device); /* might be already removed or have more ports */
>> + remove_invalid_ports();
>> + printf("Now total ports is %d\n", nb_ports);
>> }
>> if (need_to_start)
>> start_packet_forwarding(0);
>> }
>>
>> +static void
>> +remove_invalid_ports_callback(void *arg)
>> +{
>> + RTE_SET_USED(arg);
>> +
>> + remove_invalid_ports();
>> + printf("Now total ports is %d\n", nb_ports);
>> +}
>> +
>> /* This function is used by the interrupt thread */
>> static int
>> eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
>> @@ -3803,8 +3806,8 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
>>
>> switch (type) {
>> case RTE_ETH_EVENT_NEW:
>> - ports[port_id].need_setup = 1;
>> - ports[port_id].port_status = RTE_PORT_HANDLING;
>> + if (setup_on_probe_event)
>> + setup_attached_port(port_id);
>> break;
>> case RTE_ETH_EVENT_INTR_RMV:
>> if (rte_eal_alarm_set(100000,
>> @@ -3815,6 +3818,9 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
>> case RTE_ETH_EVENT_DESTROY:
>> 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,
>> + (void *)(intptr_t)port_id))
>> + fprintf(stderr, "Could not set up deferred device released\n");
>> break;
>> case RTE_ETH_EVENT_RX_AVAIL_THRESH: {
>> uint16_t rxq_id;
>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>> index 7d24d25970..080d3a1139 100644
>> --- a/app/test-pmd/testpmd.h
>> +++ b/app/test-pmd/testpmd.h
>> @@ -306,7 +306,6 @@ struct rte_port {
>> uint16_t tx_vlan_id;/**< The tag ID */
>> uint16_t tx_vlan_id_outer;/**< The outer tag ID */
>> volatile uint16_t port_status; /**< port started or not */
>> - uint8_t need_setup; /**< port just attached */
>> uint8_t need_reconfig; /**< need reconfiguring port or not */
>> uint8_t need_reconfig_queues; /**< need reconfiguring queues or not */
>> uint8_t rss_flag; /**< enable rss or not */
>> diff --git a/drivers/net/bonding/bonding_testpmd.c b/drivers/net/bonding/bonding_testpmd.c
>> index 9529e16fb6..9216271314 100644
>> --- a/drivers/net/bonding/bonding_testpmd.c
>> +++ b/drivers/net/bonding/bonding_testpmd.c
>> @@ -765,7 +765,6 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,
>>
>> ports[port_id].update_conf = 1;
>> ports[port_id].bond_flag = 1;
>> - ports[port_id].need_setup = 0;
>> ports[port_id].port_status = RTE_PORT_STOPPED;
>> }
>>
> .
@@ -3463,15 +3463,12 @@ attach_port(char *identifier)
return;
}
- /* first attach mode: event */
- if (setup_on_probe_event) {
- /* new ports are detected on RTE_ETH_EVENT_NEW event */
- for (pi = 0; pi < RTE_MAX_ETHPORTS; pi++)
- if (ports[pi].port_status == RTE_PORT_HANDLING &&
- ports[pi].need_setup != 0)
- setup_attached_port(pi);
+ /*
+ * first attach mode: event, setting up attached port is done in
+ * probing callback.
+ */
+ if (setup_on_probe_event)
return;
- }
/* second attach mode: iterator */
RTE_ETH_FOREACH_MATCHING_DEV(pi, identifier, &iterator) {
@@ -3502,7 +3499,6 @@ setup_attached_port(portid_t pi)
ports_ids[nb_ports++] = pi;
fwd_ports_ids[nb_fwd_ports++] = pi;
nb_cfg_ports = nb_fwd_ports;
- ports[pi].need_setup = 0;
ports[pi].port_status = RTE_PORT_STOPPED;
printf("Port %d is attached. Now total ports is %d\n", pi, nb_ports);
@@ -3536,10 +3532,8 @@ detach_device(struct rte_device *dev)
TESTPMD_LOG(ERR, "Failed to detach device %s\n", rte_dev_name(dev));
return;
}
- remove_invalid_ports();
printf("Device is detached\n");
- printf("Now total ports is %d\n", nb_ports);
printf("Done\n");
return;
}
@@ -3606,11 +3600,9 @@ detach_devargs(char *identifier)
return;
}
- remove_invalid_ports();
-
printf("Device %s is detached\n", identifier);
- printf("Now total ports is %d\n", nb_ports);
printf("Done\n");
+
rte_devargs_reset(&da);
}
@@ -3774,11 +3766,22 @@ rmv_port_callback(void *arg)
struct rte_device *device = dev_info.device;
close_port(port_id);
detach_device(device); /* might be already removed or have more ports */
+ remove_invalid_ports();
+ printf("Now total ports is %d\n", nb_ports);
}
if (need_to_start)
start_packet_forwarding(0);
}
+static void
+remove_invalid_ports_callback(void *arg)
+{
+ RTE_SET_USED(arg);
+
+ remove_invalid_ports();
+ printf("Now total ports is %d\n", nb_ports);
+}
+
/* This function is used by the interrupt thread */
static int
eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
@@ -3803,8 +3806,8 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
switch (type) {
case RTE_ETH_EVENT_NEW:
- ports[port_id].need_setup = 1;
- ports[port_id].port_status = RTE_PORT_HANDLING;
+ if (setup_on_probe_event)
+ setup_attached_port(port_id);
break;
case RTE_ETH_EVENT_INTR_RMV:
if (rte_eal_alarm_set(100000,
@@ -3815,6 +3818,9 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
case RTE_ETH_EVENT_DESTROY:
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,
+ (void *)(intptr_t)port_id))
+ fprintf(stderr, "Could not set up deferred device released\n");
break;
case RTE_ETH_EVENT_RX_AVAIL_THRESH: {
uint16_t rxq_id;
@@ -306,7 +306,6 @@ struct rte_port {
uint16_t tx_vlan_id;/**< The tag ID */
uint16_t tx_vlan_id_outer;/**< The outer tag ID */
volatile uint16_t port_status; /**< port started or not */
- uint8_t need_setup; /**< port just attached */
uint8_t need_reconfig; /**< need reconfiguring port or not */
uint8_t need_reconfig_queues; /**< need reconfiguring queues or not */
uint8_t rss_flag; /**< enable rss or not */
@@ -765,7 +765,6 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,
ports[port_id].update_conf = 1;
ports[port_id].bond_flag = 1;
- ports[port_id].need_setup = 0;
ports[port_id].port_status = RTE_PORT_STOPPED;
}