Re: [dpdk-dev] [PATCH v4] examples/l2fwd: add cmdline option for forwarding port info

["Varghese, Vipin" <vipin.varghese@intel.com>]

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