Re: [dpdk-dev] [dpdk-dev v2] [PATCH] examples/l2fwd-event: add option to configure port pairs

[Andrzej Ostruszka <amo@semihalf.com>]

On 3/26/20 7:42 AM, pbhagavatula@marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> 
> Current l2fwd-event application statically configures adjacent ports as
> destination ports for forwarding the traffic.
> 
> Add a config option to pass the forwarding port pair mapping which allows
> the user to configure forwarding port mapping.
> 
> If no config argument is specified, destination port map is not
> changed and traffic gets forwarded with existing mapping.
> 
> To align port/queue configuration of each lcore with destination port
> map, port/queue configuration of each lcore gets modified when config
> option is specificed.
> 
> Ex: ./l2fwd-event -c 0xff -- -p 0x3f -q 2 --config="(0,3)(1,4)(2,5)"
> 
> With above config option, traffic received from portid = 0 gets forwarded
> to port = 3 and vice versa, similarly traffic gets forwarded on other port
> pairs (1,4) and (2,5).
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> ---
>  v2 Changes:
>  - Fix minor formatting error.
>  - Change uint8_t to bool.
[...]
> @@ -99,6 +103,69 @@ l2fwd_event_parse_eventq_sched(const char *optarg,
>  		rsrc->sched_type = RTE_SCHED_TYPE_PARALLEL;
>  }
> 
> +static int
> +l2fwd_parse_port_pair_config(const char *q_arg, struct l2fwd_resources *rsrc)
> +{
> +	enum fieldnames {
> +		FLD_PORT1 = 0,
> +		FLD_PORT2,
> +		_NUM_FLD
> +	};
> +	const char *p, *p0 = q_arg;
> +	uint16_t int_fld[_NUM_FLD];
> +	char *str_fld[_NUM_FLD];
> +	uint16_t port_pair = 0;
> +	unsigned int size;
> +	char s[256];
> +	char *end;
> +	int i;
> +
> +	while ((p = strchr(p0, '(')) != NULL) {
> +		++p;
> +		p0 = strchr(p, ')');
> +		if (p0 == NULL)
> +			return -1;
> +
> +		size = p0 - p;
> +		if (size >= sizeof(s))
> +			return -1;
> +
> +		snprintf(s, sizeof(s), "%.*s", size, p);

This is a bit peculiar form of memcpy - you want no more than sizeof(s)
copied but that you checked above so here simple memcpy should be enough.

> +		if (rte_strsplit(s, sizeof(s), str_fld,
> +					_NUM_FLD, ',') != _NUM_FLD)
> +			return -1;
> +
> +		for (i = 0; i < _NUM_FLD; i++) {
> +			errno = 0;
> +			int_fld[i] = strtoul(str_fld[i], &end, 0);
> +			if (errno != 0 || end == str_fld[i] || int_fld[i] > 255)

Replace 255 with check on ">= RTE_MAX_ETHPORTS".

> +				return -1;
> +		}
> +
> +		if (port_pair >= RTE_MAX_ETHPORTS / 2) {
> +			printf("exceeded max number of port pair params: Current %d Max = %d\n",
> +			       port_pair, RTE_MAX_ETHPORTS / 2);
> +			return -1;
> +		}
> +
> +		if ((rsrc->dst_ports[int_fld[FLD_PORT1]] != UINT32_MAX) ||
> +			(rsrc->dst_ports[int_fld[FLD_PORT2]] != UINT32_MAX)) {
> +			printf("Duplicate port pair (%d,%d) config\n",
> +					int_fld[FLD_PORT1], int_fld[FLD_PORT2]);
> +			return -1;
> +		}
> +
> +		rsrc->dst_ports[int_fld[FLD_PORT1]] = int_fld[FLD_PORT2];
> +		rsrc->dst_ports[int_fld[FLD_PORT2]] = int_fld[FLD_PORT1];
> +
> +		port_pair++;
> +	}
> +
> +	rsrc->port_pairs = true;
> +
> +	return 0;
> +}
> +
[...]
> @@ -209,6 +293,51 @@ l2fwd_event_parse_args(int argc, char **argv,
>  	return ret;
>  }
> 
> +/*
> + * Check port pair config with enabled port mask,
> + * and for valid port pair combinations.
> + */
> +static int
> +check_port_pair_config(struct l2fwd_resources *rsrc)
> +{
> +	uint32_t port_pair_mask = 0;
> +	uint32_t portid;
> +	uint16_t index;
> +
> +	for (index = 0; index < rte_eth_dev_count_avail(); index++) {
> +		if ((rsrc->enabled_port_mask & (1 << index)) == 0)
> +			continue;
> +
> +		portid = rsrc->dst_ports[index];
> +		if (portid == UINT32_MAX) {
> +			printf("port %u is enabled in but no valid port pair\n",
> +			       index);
> +			return -1;
> +		}
> +
> +		if (!rte_eth_dev_is_valid_port(index)) {
> +			printf("port %u is not valid\n", index);
> +			return -1;
> +		}
> +
> +		if (!rte_eth_dev_is_valid_port(portid)) {
> +			printf("port %u is not valid\n", portid);
> +			return -1;
> +		}
> +
> +		if (port_pair_mask & (1 << portid) &&
> +				rsrc->dst_ports[portid] != index) {
> +			printf("port %u is used in other port pairs\n", portid);
> +			return -1;
> +		}
> +
> +		port_pair_mask |= (1 << portid);
> +		port_pair_mask |= (1 << index);
> +	}

In the above loop you are doing checks twice.  Suppose you have pair
(2,3) and you go by index from 0 (like you do) and reach point i=2.
Then you check i=2 and p=3, then on next iteration you do the same
checks (this time i=3,p=2).  I guess simple fix would be to skip loop
iteration both on not enabled (like you do) and on check if the port was
already checked (test bit in port_pair_mask).

With regards
Andrzej Ostruszka