Re: [dpdk-dev] [PATCH v7 08/14] app/testpmd: add hairpin support

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/30/2019 11:53 PM, Ori Kam wrote:
> This commit introduce the hairpin queues to the testpmd.
> the hairpin queue is configured using --hairpinq=<n>
> the hairpin queue adds n queue objects for both the total number
> of TX queues and RX queues.
> The connection between the queues are 1 to 1, first Rx hairpin queue
> will be connected to the first Tx hairpin queue
> 
> Signed-off-by: Ori Kam <orika@mellanox.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
>  app/test-pmd/parameters.c |  28 ++++++++++++
>  app/test-pmd/testpmd.c    | 109 +++++++++++++++++++++++++++++++++++++++++++++-
>  app/test-pmd/testpmd.h    |   3 ++

New parameter should be documented in 'doc/guides/testpmd_app_ug/run_app.rst',
can you please describe befiefly how it works, what the mapping will like by
default etc..

<...>

> @@ -2028,6 +2076,11 @@ struct extmem_param {
>  	queueid_t qi;
>  	struct rte_port *port;
>  	struct rte_ether_addr mac_addr;
> +	struct rte_eth_hairpin_conf hairpin_conf = {
> +		.peer_count = 1,
> +	};
> +	int i;
> +	struct rte_eth_hairpin_cap cap;
>  
>  	if (port_id_is_invalid(pid, ENABLED_WARN))
>  		return 0;
> @@ -2060,9 +2113,16 @@ struct extmem_param {
>  			configure_rxtx_dump_callbacks(0);
>  			printf("Configuring Port %d (socket %u)\n", pi,
>  					port->socket_id);
> +			if (nb_hairpinq > 0 &&
> +			    rte_eth_dev_hairpin_capability_get(pi, &cap)) {
> +				printf("Port %d doesn't support hairpin "
> +				       "queues\n", pi);
> +				return -1;
> +			}
>  			/* configure port */
> -			diag = rte_eth_dev_configure(pi, nb_rxq, nb_txq,
> -						&(port->dev_conf));
> +			diag = rte_eth_dev_configure(pi, nb_rxq + nb_hairpinq,
> +						     nb_txq + nb_hairpinq,
> +						     &(port->dev_conf));
>  			if (diag != 0) {
>  				if (rte_atomic16_cmpset(&(port->port_status),
>  				RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
> @@ -2155,6 +2215,51 @@ struct extmem_param {
>  				port->need_reconfig_queues = 1;
>  				return -1;
>  			}
> +			/* setup hairpin queues */
> +			i = 0;
> +			for (qi = nb_txq; qi < nb_hairpinq + nb_txq; qi++) {
> +				hairpin_conf.peers[0].port = pi;
> +				hairpin_conf.peers[0].queue = i + nb_rxq;
> +				diag = rte_eth_tx_hairpin_queue_setup
> +					(pi, qi, nb_txd, &hairpin_conf);
> +				i++;
> +				if (diag == 0)
> +					continue;
> +
> +				/* Fail to setup rx queue, return */
> +				if (rte_atomic16_cmpset(&(port->port_status),
> +							RTE_PORT_HANDLING,
> +							RTE_PORT_STOPPED) == 0)
> +					printf("Port %d can not be set back "
> +							"to stopped\n", pi);
> +				printf("Fail to configure port %d hairpin "
> +				       "queues\n", pi);
> +				/* try to reconfigure queues next time */
> +				port->need_reconfig_queues = 1;
> +				return -1;
> +			}
> +			i = 0;
> +			for (qi = nb_rxq; qi < nb_hairpinq + nb_rxq; qi++) {
> +				hairpin_conf.peers[0].port = pi;
> +				hairpin_conf.peers[0].queue = i + nb_txq;
> +				diag = rte_eth_rx_hairpin_queue_setup
> +					(pi, qi, nb_rxd, &hairpin_conf);
> +				i++;
> +				if (diag == 0)
> +					continue;
> +
> +				/* Fail to setup rx queue, return */
> +				if (rte_atomic16_cmpset(&(port->port_status),
> +							RTE_PORT_HANDLING,
> +							RTE_PORT_STOPPED) == 0)
> +					printf("Port %d can not be set back "
> +							"to stopped\n", pi);
> +				printf("Fail to configure port %d hairpin "
> +				       "queues\n", pi);
> +				/* try to reconfigure queues next time */
> +				port->need_reconfig_queues = 1;
> +				return -1;
> +			}

'start_port()' function is already huge, what do you think moving hairpin
related setup into a specific function and call it when "nb_hairpinq > 0" only?
This makes the hairpin related config more clear and 'start_port()' function
simpler.
I think all hairpin related operations can be extracted, like capability check,
'rte_eth_dev_configure' & hairpin queue setup.