> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@intel.com>
> Sent: Thursday, October 31, 2019 7:55 PM
> To: Ori Kam <or...@mellanox.com>; Wenzhuo Lu <wenzhuo...@intel.com>;
> Jingjing Wu <jingjing...@intel.com>; Bernard Iremonger
> <bernard.iremon...@intel.com>
> Cc: dev@dpdk.org; step...@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.yi...@intel.com>
> >> Sent: Thursday, October 31, 2019 7:12 PM
> >> To: Ori Kam <or...@mellanox.com>; Wenzhuo Lu <wenzhuo...@intel.com>;
> >> Jingjing Wu <jingjing...@intel.com>; Bernard Iremonger
> >> <bernard.iremon...@intel.com>
> >> Cc: dev@dpdk.org; step...@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 <or...@mellanox.com>
> >>> Acked-by: Viacheslav Ovsiienko <viachesl...@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
