Re: [dpdk-dev] [PATCH v6] testpmd: Add port hotplug support

[Tetsuya Mukawa <mukawa@igel.co.jp>]

On 2015/02/04 10:44, Qiu, Michael wrote:
> On 2/3/2015 6:30 PM, Tetsuya Mukawa wrote:
>> On 2015/02/03 18:14, Qiu, Michael wrote:
>>> On 2/3/2015 2:16 PM, Qiu, Michael wrote:
>>>> On 2/1/2015 12:02 PM, Tetsuya Mukawa wrote:
>>>>> The patch introduces following commands.
>>>>> - port attach [ident]
>>>>> - port detach [port_id]
>>>>>  - attach: attaching a port
>>>>>  - detach: detaching a port
>>>>>  - ident: pci address of physical device.
>>>>>           Or device name and paramerters of virtual device.
>>>>>          (ex. 0000:02:00.0, eth_pcap0,iface=eth0)
>>>>>  - port_id: port identifier
>>>>>
>>>>> v5:
>>>>> - Add testpmd documentation.
>>>>>   (Thanks to Iremonger, Bernard)
>>>>> v4:
>>>>>  - Fix strings of command help.
>>>>>
>>>>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
>>> [...]
>>>
>>>>> +static int
>>>>> +port_is_closed(portid_t port_id)
>>>>> +{
>>>>> +	if (port_id_is_invalid(port_id, ENABLED_WARN))
>>>>> +		return 0;
>>>>> +
>>>>> +	if (ports[port_id].port_status != RTE_PORT_CLOSED)
>>>>> +		return 0;
>>>>> +
>>>>> +	return 1;
>>>>> +}
>>>>> +
>>>>> +int
>>>>>  start_port(portid_t pid)
>>>>>  {
>>>>>  	int diag, need_check_link_status = 0;
>>>>> @@ -1296,8 +1347,8 @@ start_port(portid_t pid)
>>>>>  
>>>>>  	if(dcb_config)
>>>>>  		dcb_test = 1;
>>>>> -	for (pi = 0; pi < nb_ports; pi++) {
>>>>> -		if (pid < nb_ports && pid != pi)
>>>>> +	FOREACH_PORT(pi, ports) {
>>>>> +		if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi)
>>>> Here may it be:
>>>>
>>>> if (!port_id_is_invalid(pid, DISABLED_WARN) && (pid != pi || pid == RET_PORT_ALL))
>>> Sorry, should be:
>>>
>>> if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi && pid != (portid_t)RET_PORT_ALL)
>>>
>>> Otherwise, should check for "RET_PORT_ALL" in function port_id_is_invalid()
>> Thanks for comment. I've found 2 issues.
>> (I guess the original code has same issue.)
> Original code may not have this issue, cause RET_PORT_ALL is always
> greater than nb_ports, so "continue" will not exec. The logic may be 
> right, but it is a little hard to understand.
>
>> One is that "port_id_is_invalid" should receives "pi" instead of "pid".
>> The other is if statement is wrong as you said.
>>
>> I guess following statement will be good.
>>
>> if (port_id_is_invalid(pi, DISABLED_WARN) || (pid != pi && pid !=
>> (portid_t)RTE_PORT_ALL))
> Actually, "port_id_is_invalid(pi, DISABLED_WARN)" could be removed as
> "FOREACH_PORT" will find a valid port.

Good point!

> So it could be:
>
> if (pid != pi && pid != (portid_t)RTE_PORT_ALL)
>
> What do you think?

I agree with you.
I will change like above.

Thanks,
Tetsuya

> Thanks,
> Michael
>> How about it?
>>
>> Thanks,
>> Tetsuya
>>
>>
>>> Thanks,
>>> Michael
>>>
>>>> Otherwise no port will be start by default.
>>>>
>>>>
>>>> Thanks,
>>>> Michael
>>>>
>>>>>  			continue;
>>>>>  
>>>>>  		port = &ports[pi];
>>>>> @@ -1421,7 +1472,7 @@ start_port(portid_t pid)
>>>>>  	}
>>>>>  
>>>>>  	if (need_check_link_status && !no_link_check)
>>>>> -		check_all_ports_link_status(nb_ports, RTE_PORT_ALL);
>>>>> +		check_all_ports_link_status(RTE_PORT_ALL);
>>>>>  	else
>>>>>  		printf("Please stop the ports first\n");
>>>>>  
>>>>> @@ -1446,8 +1497,8 @@ stop_port(portid_t pid)
>>>>>  	}
>>>>>  	printf("Stopping ports...\n");
>>>>>  
>>>>> -	for (pi = 0; pi < nb_ports; pi++) {
>>>>> -		if (pid < nb_ports && pid != pi)
>>>>> +	FOREACH_PORT(pi, ports) {
>>>>> +		if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi)
>>>>>  			continue;
>>>>>  
>>>>>  		port = &ports[pi];
>>>>> @@ -1463,7 +1514,7 @@ stop_port(portid_t pid)
>>>>>  		need_check_link_status = 1;
>>>>>  	}
>>>>>  	if (need_check_link_status && !no_link_check)
>>>>> -		check_all_ports_link_status(nb_ports, RTE_PORT_ALL);
>>>>> +		check_all_ports_link_status(RTE_PORT_ALL);
>>>>>  
>>>>>  	printf("Done\n");
>>>>>  }
>>>>> @@ -1481,8 +1532,8 @@ close_port(portid_t pid)
>>>>>  
>>>>>  	printf("Closing ports...\n");
>>>>>  
>>>>> -	for (pi = 0; pi < nb_ports; pi++) {
>>>>> -		if (pid < nb_ports && pid != pi)
>>>>> +	FOREACH_PORT(pi, ports) {
>>>>> +		if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi)
>>>>>  			continue;
>>>>>  
>>>>>  		port = &ports[pi];
>>>>> @@ -1502,31 +1553,83 @@ close_port(portid_t pid)
>>>>>  	printf("Done\n");
>>>>>  }
>>>>>  
>>>>> -int
>>>>> -all_ports_stopped(void)
>>>>> +void
>>>>> +attach_port(char *identifier)
>>>>>  {
>>>>> -	portid_t pi;
>>>>> -	struct rte_port *port;
>>>>> +	portid_t i, j, pi = 0;
>>>>>  
>>>>> -	for (pi = 0; pi < nb_ports; pi++) {
>>>>> -		port = &ports[pi];
>>>>> -		if (port->port_status != RTE_PORT_STOPPED)
>>>>> -			return 0;
>>>>> +	printf("Attaching a new port...\n");
>>>>> +
>>>>> +	if (identifier == NULL) {
>>>>> +		printf("Invalid parameters are speficied\n");
>>>>> +		return;
>>>>>  	}
>>>>>  
>>>>> -	return 1;
>>>>> +	if (test_done == 0) {
>>>>> +		printf("Please stop forwarding first\n");
>>>>> +		return;
>>>>> +	}
>>>>> +
>>>>> +	if (rte_eal_dev_attach(identifier, &pi))
>>>>> +		return;
>>>>> +
>>>>> +	ports[pi].enabled = 1;
>>>>> +	reconfig(pi, rte_eth_dev_socket_id(pi));
>>>>> +	rte_eth_promiscuous_enable(pi);
>>>>> +
>>>>> +	nb_ports = rte_eth_dev_count();
>>>>> +
>>>>> +	/* set_default_fwd_ports_config(); */
>>>>> +	bzero(fwd_ports_ids, sizeof(fwd_ports_ids));
>>>>> +	i = 0;
>>>>> +	FOREACH_PORT(j, ports) {
>>>>> +		fwd_ports_ids[i] = j;
>>>>> +		i++;
>>>>> +	}
>>>>> +	nb_cfg_ports = nb_ports;
>>>>> +	nb_fwd_ports++;
>>>>> +
>>>>> +	ports[pi].port_status = RTE_PORT_STOPPED;
>>>>> +
>>>>> +	printf("Port %d is attached. Now total ports is %d\n", pi, nb_ports);
>>>>> +	printf("Done\n");
>>>>>  }
>>>>>  
>>>>> -int
>>>>> -port_is_started(portid_t port_id)
>>>>> +void
>>>>> +detach_port(uint8_t port_id)
>>>>>  {
>>>>> -	if (port_id_is_invalid(port_id))
>>>>> -		return -1;
>>>>> +	portid_t i, pi = 0;
>>>>> +	char name[RTE_ETH_NAME_MAX_LEN];
>>>>>  
>>>>> -	if (ports[port_id].port_status != RTE_PORT_STARTED)
>>>>> -		return 0;
>>>>> +	printf("Detaching a port...\n");
>>>>>  
>>>>> -	return 1;
>>>>> +	if (!port_is_closed(port_id)) {
>>>>> +		printf("Please close port first\n");
>>>>> +		return;
>>>>> +	}
>>>>> +
>>>>> +	rte_eth_promiscuous_disable(port_id);
>>>>> +
>>>>> +	if (rte_eal_dev_detach(port_id, name))
>>>>> +		return;
>>>>> +
>>>>> +	ports[port_id].enabled = 0;
>>>>> +	nb_ports = rte_eth_dev_count();
>>>>> +
>>>>> +	/* set_default_fwd_ports_config(); */
>>>>> +	bzero(fwd_ports_ids, sizeof(fwd_ports_ids));
>>>>> +	i = 0;
>>>>> +	FOREACH_PORT(pi, ports) {
>>>>> +		fwd_ports_ids[i] = pi;
>>>>> +		i++;
>>>>> +	}
>>>>> +	nb_cfg_ports = nb_ports;
>>>>> +	nb_fwd_ports--;
>>>>> +
>>>>> +	printf("Port '%s' is detached. Now total ports is %d\n",
>>>>> +			name, nb_ports);
>>>>> +	printf("Done\n");
>>>>> +	return;
>>>>>  }
>>>>>  
>>>>>  void
>>>>> @@ -1534,7 +1637,7 @@ pmd_test_exit(void)
>>>>>  {
>>>>>  	portid_t pt_id;
>>>>>  
>>>>> -	for (pt_id = 0; pt_id < nb_ports; pt_id++) {
>>>>> +	FOREACH_PORT(pt_id, ports) {
>>>>>  		printf("Stopping port %d...", pt_id);
>>>>>  		fflush(stdout);
>>>>>  		rte_eth_dev_close(pt_id);
>>>>> @@ -1553,7 +1656,7 @@ struct pmd_test_command {
>>>>>  
>>>>>  /* Check the link status of all ports in up to 9s, and print them finally */
>>>>>  static void
>>>>> -check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
>>>>> +check_all_ports_link_status(uint32_t port_mask)
>>>>>  {
>>>>>  #define CHECK_INTERVAL 100 /* 100ms */
>>>>>  #define MAX_CHECK_TIME 90 /* 9s (90 * 100ms) in total */
>>>>> @@ -1564,7 +1667,7 @@ check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
>>>>>  	fflush(stdout);
>>>>>  	for (count = 0; count <= MAX_CHECK_TIME; count++) {
>>>>>  		all_ports_up = 1;
>>>>> -		for (portid = 0; portid < port_num; portid++) {
>>>>> +		FOREACH_PORT(portid, ports) {
>>>>>  			if ((port_mask & (1 << portid)) == 0)
>>>>>  				continue;
>>>>>  			memset(&link, 0, sizeof(link));
>>>>> @@ -1688,7 +1791,7 @@ init_port_config(void)
>>>>>  	portid_t pid;
>>>>>  	struct rte_port *port;
>>>>>  
>>>>> -	for (pid = 0; pid < nb_ports; pid++) {
>>>>> +	FOREACH_PORT(pid, ports) {
>>>>>  		port = &ports[pid];
>>>>>  		port->dev_conf.rxmode = rx_mode;
>>>>>  		port->dev_conf.fdir_conf = fdir_conf;
>>>>> @@ -1877,7 +1980,7 @@ main(int argc, char** argv)
>>>>>  
>>>>>  	nb_ports = (portid_t) rte_eth_dev_count();
>>>>>  	if (nb_ports == 0)
>>>>> -		rte_exit(EXIT_FAILURE, "No probed ethernet device\n");
>>>>> +		RTE_LOG(WARNING, EAL, "No probed ethernet devices\n");
>>>>>  
>>>>>  	set_def_fwd_config();
>>>>>  	if (nb_lcores == 0)
>>>>> @@ -1899,7 +2002,7 @@ main(int argc, char** argv)
>>>>>  		rte_exit(EXIT_FAILURE, "Start ports failed\n");
>>>>>  
>>>>>  	/* set all ports to promiscuous mode by default */
>>>>> -	for (port_id = 0; port_id < nb_ports; port_id++)
>>>>> +	FOREACH_PORT(port_id, ports)
>>>>>  		rte_eth_promiscuous_enable(port_id);
>>>>>  
>>>>>  #ifdef RTE_LIBRTE_CMDLINE
>>>>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>>>>> index 8f5e6c7..109c670 100644
>>>>> --- a/app/test-pmd/testpmd.h
>>>>> +++ b/app/test-pmd/testpmd.h
>>>>> @@ -134,6 +134,7 @@ struct fwd_stream {
>>>>>   * The data structure associated with each port.
>>>>>   */
>>>>>  struct rte_port {
>>>>> +	uint8_t                 enabled;    /**< Port enabled or not */
>>>>>  	struct rte_eth_dev_info dev_info;   /**< PCI info + driver name */
>>>>>  	struct rte_eth_conf     dev_conf;   /**< Port configuration. */
>>>>>  	struct ether_addr       eth_addr;   /**< Port ethernet address */
>>>>> @@ -159,6 +160,14 @@ struct rte_port {
>>>>>  	struct rte_eth_txconf   tx_conf;    /**< tx configuration */
>>>>>  };
>>>>>  
>>>>> +extern portid_t __rte_unused
>>>>> +find_next_port(portid_t p, struct rte_port *ports, int size);
>>>>> +
>>>>> +#define FOREACH_PORT(p, ports) \
>>>>> +	for (p = find_next_port(0, ports, RTE_MAX_ETHPORTS); \
>>>>> +	    p < RTE_MAX_ETHPORTS; \
>>>>> +	    p = find_next_port(p + 1, ports, RTE_MAX_ETHPORTS))
>>>>> +
>>>>>  /**
>>>>>   * The data structure associated with each forwarding logical core.
>>>>>   * The logical cores are internally numbered by a core index from 0 to
>>>>> @@ -515,6 +524,8 @@ int init_port_dcb_config(portid_t pid,struct dcb_config *dcb_conf);
>>>>>  int start_port(portid_t pid);
>>>>>  void stop_port(portid_t pid);
>>>>>  void close_port(portid_t pid);
>>>>> +void attach_port(char *identifier);
>>>>> +void detach_port(uint8_t port_id);
>>>>>  int all_ports_stopped(void);
>>>>>  int port_is_started(portid_t port_id);
>>>>>  void pmd_test_exit(void);
>>>>> @@ -558,10 +569,15 @@ void get_ethertype_filter(uint8_t port_id, uint16_t index);
>>>>>  void get_2tuple_filter(uint8_t port_id, uint16_t index);
>>>>>  void get_5tuple_filter(uint8_t port_id, uint16_t index);
>>>>>  void get_flex_filter(uint8_t port_id, uint16_t index);
>>>>> -int port_id_is_invalid(portid_t port_id);
>>>>>  int rx_queue_id_is_invalid(queueid_t rxq_id);
>>>>>  int tx_queue_id_is_invalid(queueid_t txq_id);
>>>>>  
>>>>> +enum print_warning {
>>>>> +	ENABLED_WARN = 0,
>>>>> +	DISABLED_WARN
>>>>> +};
>>>>> +int port_id_is_invalid(portid_t port_id, enum print_warning warning);
>>>>> +
>>>>>  /*
>>>>>   * Work-around of a compilation error with ICC on invocations of the
>>>>>   * rte_be_to_cpu_16() function.
>>>>> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>>> index 218835a..1cacbcf 100644
>>>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>>> @@ -808,6 +808,63 @@ The following sections show functions for configuring ports.
>>>>>  
>>>>>      Port configuration changes only become active when forwarding is started/restarted.
>>>>>  
>>>>> +port attach
>>>>> +~~~~~~~~~~~
>>>>> +
>>>>> +Attach a port specified by pci address or virtual device args.
>>>>> +
>>>>> +To attach a new pci device, the device should be recognized by kernel first.
>>>>> +Then it should be moved under DPDK management.
>>>>> +Finally the port can be attached to testpmd.
>>>>> +On the other hand, to attach a port created by virtual device, above steps are not needed.
>>>>> +
>>>>> +port attach (identifier)
>>>>> +
>>>>> +For example, to attach a port that pci address is 0000:02:00.0.
>>>>> +
>>>>> +.. code-block:: console
>>>>> +
>>>>> +    testpmd> port attach 0000:02:00.0
>>>>> +    Attaching a new port...
>>>>> +    ... snip ...
>>>>> +    Port 0 is attached. Now total ports is 1
>>>>> +    Done
>>>>> +
>>>>> +For example, to attach a port created by pcap PMD.
>>>>> +
>>>>> +.. code-block:: console
>>>>> +
>>>>> +    testpmd> port attach eth_pcap0,iface=eth0
>>>>> +    Attaching a new port...
>>>>> +    ... snip ...
>>>>> +    Port 0 is attached. Now total ports is 1
>>>>> +    Done
>>>>> +
>>>>> +In this case, identifier is "eth_pcap0,iface=eth0".
>>>>> +This identifier format is the same as "--vdev" format of DPDK applications.
>>>>> +
>>>>> +port detach
>>>>> +~~~~~~~~~~~
>>>>> +
>>>>> +Detach a specific port.
>>>>> +
>>>>> +Before detaching a port, the port should be closed.
>>>>> +Also to remove a pci device completely from the system, first detach the port from testpmd.
>>>>> +Then the device should be moved under kernel management.
>>>>> +Finally the device can be remove using kernel pci hotplug functionality.
>>>>> +On the other hand, to remove a port created by virtual device, above steps are not needed.
>>>>> +
>>>>> +port detach (port_id)
>>>>> +
>>>>> +For example, to detach a port 0.
>>>>> +
>>>>> +.. code-block:: console
>>>>> +
>>>>> +    testpmd> port detach 0
>>>>> +    Detaching a port...
>>>>> +    ... snip ...
>>>>> +    Done
>>>>> +
>>>>>  port start
>>>>>  ~~~~~~~~~~
>>>>>  
>>