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?
>
>> <...>
>>
>>> @@ -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
>
