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

[Pavan Nikhilesh Bhagavatula]

>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