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

[Ori Kam <orika@mellanox.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, October 31, 2019 7:55 PM
> To: Ori Kam <orika@mellanox.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>;
> Jingjing Wu <jingjing.wu@intel.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>
> Cc: dev@dpdk.org; stephen@networkplumber.org
> Subject: Re: [dpdk-dev] [PATCH v7 08/14] app/testpmd: add hairpin support
> 
> On 10/31/2019 5:36 PM, Ori Kam wrote:
> > Hi Ferruh,
> >
> > Thanks, for the comments PSB,
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Thursday, October 31, 2019 7:12 PM
> >> To: Ori Kam <orika@mellanox.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>;
> >> Jingjing Wu <jingjing.wu@intel.com>; Bernard Iremonger
> >> <bernard.iremonger@intel.com>
> >> Cc: dev@dpdk.org; stephen@networkplumber.org
> >> Subject: Re: [dpdk-dev] [PATCH v7 08/14] app/testpmd: add hairpin support
> >>
> >> 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..
> >>
> >
> > The default is no hairpin queues,
> > If hairpinq = x is specified then we are adding x queues to the Rx queue list
> and x queues to the Tx, and bind them one
> > Rx queue to on Tx queue.
> >
> > For example: test pmd parameters are:
> > --rxq=3 --txq=2 --hairpinq=4 the result will be:
> >
> > 3 normal Txq (queues 0,1,2)
> > 2 normal Txq (queues 0,1)
> > 4 hairpin queues (Rxq 3,4,5,6 Txq 2,3,4,5) while Rxq(3) will be connected to
> Txq(2), Rxq(4) will be connected to Txq(3) and so on.
> 
> Thanks, can you please put them into documentation in next version?
> 

Sure no problem,

> >
> >> <...>
> >>
> >>> @@ -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.
> >
> > I have no strong feeling, I just wanted to keep the function with the same
> format that all actions are done inside
> > the function, also it was my intention to easily show the connection between
> the two types of queues.
> >
> > Best,
> > Ori
> >

Thanks,
Ori