>Hi Vamsi & Pavan,
>
>I like this idea, couple of queries
>
>snipped
>> +static int
>> +check_port_pair_config(void)
>> +{
>> + uint32_t port_pair_config_mask = 0;
>> + uint32_t port_pair_mask = 0;
>> + uint16_t index, i, portid;
>> +
>> + for (index = 0; index < nb_port_pair_params; index++) {
>> + port_pair_mask = 0;
>> +
>> + for (i = 0; i < NUM_PORTS; i++) {
>> + portid = port_pair_params[index].port[i];
>> + if ((l2fwd_enabled_port_mask & (1 << portid))
>== 0) {
>> + printf("port %u is not enabled in port
>> mask\n",
>> + portid);
>> + return -1;
>> + }
>> + if (!rte_eth_dev_is_valid_port(portid)) {
>> + printf("port %u is not present on the
>> board\n",
>> + portid);
>> + return -1;
>> + }
>> +
>
>Should we check & warn the user if
>1. port speed mismatch
>2. on different NUMA
>3. port pairs are physical and vdev like tap, and KNI (performance).
>
Sure, it can be a separate patch as it will be applicable for multiple examples.
>> + port_pair_mask |= 1 << portid;
>> + }
>> +
>
>snipped
>
>
>>
>> + if (port_pair_params != NULL) {
>> + if (check_port_pair_config() < 0)
>> + rte_exit(EXIT_FAILURE, "Invalid port pair
>config\n");
>> + }
>> +
>> /* check port mask to possible port mask */
>> if (l2fwd_enabled_port_mask & ~((1 << nb_ports) - 1))
>> rte_exit(EXIT_FAILURE, "Invalid portmask; possible
>(0x%x)\n",
>> @@ -565,26 +689,35 @@ main(int argc, char **argv)
>> l2fwd_dst_ports[portid] = 0;
>> last_port = 0;
>>
>
>Should not the check_port_pair be after this? If the port is not enabled
>in port_mask will you skip that pair? or skip RX-TX from that port?
We check every port pair against l2fwd_enabled_port_mask in
check_port_pair_config()
>
>> - /*
>> - * Each logical core is assigned a dedicated TX queue on each
>port.
>> - */
>> - RTE_ETH_FOREACH_DEV(portid) {
>> - /* skip ports that are not enabled */
>> - if ((l2fwd_enabled_port_mask & (1 << portid)) == 0)
>> - continue;
>> + /* populate destination port details */
>> + if (port_pair_params != NULL) {
>> + uint16_t idx, p;
>>
>> - if (nb_ports_in_mask % 2) {
>> - l2fwd_dst_ports[portid] = last_port;
>> - l2fwd_dst_ports[last_port] = portid;
>> + for (idx = 0; idx < (nb_port_pair_params << 1); idx++) {
>> + p = idx & 1;
>> + portid = port_pair_params[idx >> 1].port[p];
>> + l2fwd_dst_ports[portid] =
>> + port_pair_params[idx >> 1].port[p ^ 1];
>> }
>> - else
>> - last_port = portid;
>> + } else {
>> + RTE_ETH_FOREACH_DEV(portid) {
>> + /* skip ports that are not enabled */
>> + if ((l2fwd_enabled_port_mask & (1 << portid))
>== 0)
>> + continue;
>>
>> - nb_ports_in_mask++;
>> - }
>> - if (nb_ports_in_mask % 2) {
>> - printf("Notice: odd number of ports in portmask.\n");
>> - l2fwd_dst_ports[last_port] = last_port;
>> + if (nb_ports_in_mask % 2) {
>> + l2fwd_dst_ports[portid] = last_port;
>> + l2fwd_dst_ports[last_port] = portid;
>> + } else {
>> + last_port = portid;
>> + }
>> +
>> + nb_ports_in_mask++;
>> + }
>> + if (nb_ports_in_mask % 2) {
>> + printf("Notice: odd number of ports in
>portmask.\n");
>> + l2fwd_dst_ports[last_port] = last_port;
>> + }
>> }
>
>As mentioned above there can ports in mask which might be disabled
>for port pair. Should not that be skipped rather than setting last port rx-
>tx loopback?
There could be scenarios where user might want to test 2x10G and 1x40G
Why force the user to explicitly mention 1x40G as port pair of itself in the
portpair config?
>
>>
>> rx_lcore_id = 0;
>> @@ -613,7 +746,8 @@ main(int argc, char **argv)
>>
>> qconf->rx_port_list[qconf->n_rx_port] = portid;
>> qconf->n_rx_port++;
>> - printf("Lcore %u: RX port %u\n", rx_lcore_id, portid);
>> + printf("Lcore %u: RX port %u TX port %u\n",
>rx_lcore_id,
>> + portid, l2fwd_dst_ports[portid]);
>> }
>>
>> nb_mbufs = RTE_MAX(nb_ports * (nb_rxd + nb_txd +
>> MAX_PKT_BURST +
>> --
>> 2.17.1
