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).
> + 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?
> - /*
> - * 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?
>
> 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
