Re: [PATCH] testpmd: add hairpin-map parameter

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 28 Sep 2023 16:39:06 +0300
Gregory Etelson <getelson@nvidia.com> wrote:

> +static __rte_always_inline
> +char *parse_hairpin_map_entry(char *input, char **next)
> +{
> +	char *tail = strchr(input, ':');
> +
> +	if (!tail)
> +		return NULL;
> +	tail[0] = '\0';
> +	*next = tail + 1;
> +	return input;
> +}
> +

There is no reason to mark this as inline. It is not in fast path.
Let compiler decide.


> +	head = parse_hairpin_map_entry(next, &next);
> +	if (!head)
> +		goto err;
> +	map->rx_port = atoi(head);

Use strtoul() to allow checking for invalid number.
Seems like the parsing here is following the parsing code pattern
that could use strtok_r.

> +		/* Fail to setup rx queue, return */
> +		if (port->port_status == RTE_PORT_HANDLING)
> +			port->port_status = RTE_PORT_STOPPED;
> +		else
> +			fprintf(stderr,
> +				"Port %d can not be set back to stopped\n", pi);
> +		fprintf(stderr,
> +			"Port %d failed to configure hairpin on rxq %u.\n"
> +			"Peer port: %u peer txq: %u\n",
> +			pi, qi, peer_tx_port, i);

Minor nit, port should be printed with %u since it is unsigned.
Lots of testpmd code gets this wrong, and compiler doesn't care.


> +/* Configure the Rx and Tx hairpin queues for the selected port. */
> +static int
> +setup_hairpin_queues(portid_t pi, portid_t p_pi, uint16_t cnt_pi)
> +{
> +	return !hairpin_multiport_mode ?
> +	       setup_legacy_hairpin_queus(pi, p_pi, cnt_pi) :
> +	       setup_mapped_harpin_queues(pi);
> +}

It is clearer to use an if statement in this case.
Also, better to write positive rather than negative logic.

	if (hairpin_multiport_mode)
		return setup_mapped_hairpin_queues(pi);
	else
		return setup_legacy_hairpin_queus(pi, p_pi, cnt_pi);