[PATCH] app/testpmd: fix flow transfer proxy port handling

[Ivan Malov <ivan.malov@oktetlabs.ru>]

The current approach detects the proxy port on each port (re-)plug and
may spam the log with error messages if the PMD does not support flows.
As testpmd is a debug tool, it must not do such implicit port handling.
Instead, the new API should be called only when the user requests that.

Revoke the existing code. Implement an explicit command-line primitive
to let the user find the proxy port themselves. Provide relevant hints.

Fixes: 1179f05cc9a0 ("ethdev: query proxy port to manage transfer flows")

Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 app/test-pmd/cmdline.c                      |  75 ++++++++++++
 app/test-pmd/config.c                       | 121 +++-----------------
 app/test-pmd/testpmd.c                      |  20 ----
 app/test-pmd/testpmd.h                      |   4 -
 app/test-pmd/util.c                         |   5 +-
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  13 +++
 6 files changed, 107 insertions(+), 131 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 4f51b259fe..a8c199f0ee 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -252,6 +252,9 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"show port (port_id) macs|mcast_macs"
 			"       Display list of mac addresses added to port.\n\n"
 
+			"show port (port_id) flow transfer proxy\n"
+			"	Display proxy port to manage transfer flows\n\n"
+
 			"show port (port_id) fec capabilities"
 			"	Show fec capabilities of a port.\n\n"
 
@@ -17588,6 +17591,77 @@ cmdline_parse_inst_t cmd_showport_macs = {
 	},
 };
 
+/* *** show flow transfer proxy port ID for the given port *** */
+struct cmd_show_port_flow_transfer_proxy_result {
+	cmdline_fixed_string_t show;
+	cmdline_fixed_string_t port;
+	portid_t port_id;
+	cmdline_fixed_string_t flow;
+	cmdline_fixed_string_t transfer;
+	cmdline_fixed_string_t proxy;
+};
+
+cmdline_parse_token_string_t cmd_show_port_flow_transfer_proxy_show =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_show_port_flow_transfer_proxy_result,
+		 show, "show");
+cmdline_parse_token_string_t cmd_show_port_flow_transfer_proxy_port =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_show_port_flow_transfer_proxy_result,
+		 port, "port");
+cmdline_parse_token_num_t cmd_show_port_flow_transfer_proxy_port_id =
+	TOKEN_NUM_INITIALIZER
+		(struct cmd_show_port_flow_transfer_proxy_result,
+		 port_id, RTE_UINT16);
+cmdline_parse_token_string_t cmd_show_port_flow_transfer_proxy_flow =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_show_port_flow_transfer_proxy_result,
+		 flow, "flow");
+cmdline_parse_token_string_t cmd_show_port_flow_transfer_proxy_transfer =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_show_port_flow_transfer_proxy_result,
+		 transfer, "transfer");
+cmdline_parse_token_string_t cmd_show_port_flow_transfer_proxy_proxy =
+	TOKEN_STRING_INITIALIZER
+		(struct cmd_show_port_flow_transfer_proxy_result,
+		 proxy, "proxy");
+
+static void
+cmd_show_port_flow_transfer_proxy_parsed(void *parsed_result,
+					 __rte_unused struct cmdline *cl,
+					 __rte_unused void *data)
+{
+	struct cmd_show_port_flow_transfer_proxy_result *res = parsed_result;
+	portid_t proxy_port_id;
+	int ret;
+
+	printf("\n");
+
+	ret = rte_flow_pick_transfer_proxy(res->port_id, &proxy_port_id, NULL);
+	if (ret != 0) {
+		fprintf(stderr, "Failed to pick transfer proxy: %s\n",
+			rte_strerror(-ret));
+		return;
+	}
+
+	printf("Transfer proxy port ID: %u\n\n", proxy_port_id);
+}
+
+cmdline_parse_inst_t cmd_show_port_flow_transfer_proxy = {
+	.f = cmd_show_port_flow_transfer_proxy_parsed,
+	.data = NULL,
+	.help_str = "show port <port_id> flow transfer proxy",
+	.tokens = {
+		(void *)&cmd_show_port_flow_transfer_proxy_show,
+		(void *)&cmd_show_port_flow_transfer_proxy_port,
+		(void *)&cmd_show_port_flow_transfer_proxy_port_id,
+		(void *)&cmd_show_port_flow_transfer_proxy_flow,
+		(void *)&cmd_show_port_flow_transfer_proxy_transfer,
+		(void *)&cmd_show_port_flow_transfer_proxy_proxy,
+		NULL,
+	}
+};
+
 /* ******************************************************************************** */
 
 /* list of instructions */
@@ -17716,6 +17790,7 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_config_rss_reta,
 	(cmdline_parse_inst_t *)&cmd_showport_reta,
 	(cmdline_parse_inst_t *)&cmd_showport_macs,
+	(cmdline_parse_inst_t *)&cmd_show_port_flow_transfer_proxy,
 	(cmdline_parse_inst_t *)&cmd_config_burst,
 	(cmdline_parse_inst_t *)&cmd_config_thresh,
 	(cmdline_parse_inst_t *)&cmd_config_threshold,
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 26cadf39f7..0477e02abe 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1449,6 +1449,21 @@ port_flow_complain(struct rte_flow_error *error)
 					 error->cause), buf) : "",
 		error->message ? error->message : "(no stated reason)",
 		rte_strerror(err));
+
+	switch (error->type) {
+	case RTE_FLOW_ERROR_TYPE_ATTR_TRANSFER:
+		fprintf(stderr, "The status suggests the use of \"transfer\" "
+				"as the possible cause of the failure. Make "
+				"sure that the flow in question and its "
+				"indirect components (if any) are managed "
+				"via \"transfer\" proxy port. Use command "
+				"\"show port (port_id) flow transfer proxy\" "
+				"to figure out the proxy port ID\n");
+		break;
+	default:
+		break;
+	}
+
 	return -err;
 }
 
@@ -1588,25 +1603,10 @@ port_action_handle_create(portid_t port_id, uint32_t id,
 	struct port_indirect_action *pia;
 	int ret;
 	struct rte_flow_error error;
-	struct rte_port *port;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
 
 	ret = action_alloc(port_id, id, &pia);
 	if (ret)
 		return ret;
-
-	port = &ports[port_id];
-
-	if (conf->transfer)
-		port_id = port->flow_transfer_proxy;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
 	if (action->type == RTE_FLOW_ACTION_TYPE_AGE) {
 		struct rte_flow_action_age *age =
 			(struct rte_flow_action_age *)(uintptr_t)(action->conf);
@@ -1629,7 +1629,6 @@ port_action_handle_create(portid_t port_id, uint32_t id,
 		return port_flow_complain(&error);
 	}
 	pia->type = action->type;
-	pia->transfer = conf->transfer;
 	printf("Indirect action #%u created\n", pia->id);
 	return 0;
 }
@@ -1656,18 +1655,9 @@ port_action_handle_destroy(portid_t port_id,
 		for (i = 0; i != n; ++i) {
 			struct rte_flow_error error;
 			struct port_indirect_action *pia = *tmp;
-			portid_t port_id_eff = port_id;
 
 			if (actions[i] != pia->id)
 				continue;
-
-			if (pia->transfer)
-				port_id_eff = port->flow_transfer_proxy;
-
-			if (port_id_is_invalid(port_id_eff, ENABLED_WARN) ||
-			    port_id_eff == (portid_t)RTE_PORT_ALL)
-				return -EINVAL;
-
 			/*
 			 * Poisoning to make sure PMDs update it in case
 			 * of error.
@@ -1675,7 +1665,7 @@ port_action_handle_destroy(portid_t port_id,
 			memset(&error, 0x33, sizeof(error));
 
 			if (pia->handle && rte_flow_action_handle_destroy(
-					port_id_eff, pia->handle, &error)) {
+					port_id, pia->handle, &error)) {
 				ret = port_flow_complain(&error);
 				continue;
 			}
@@ -1710,15 +1700,8 @@ port_action_handle_update(portid_t port_id, uint32_t id,
 	struct rte_flow_error error;
 	struct rte_flow_action_handle *action_handle;
 	struct port_indirect_action *pia;
-	struct rte_port *port;
 	const void *update;
 
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
-	port = &ports[port_id];
-
 	action_handle = port_action_handle_get_by_id(port_id, id);
 	if (!action_handle)
 		return -EINVAL;
@@ -1733,14 +1716,6 @@ port_action_handle_update(portid_t port_id, uint32_t id,
 		update = action;
 		break;
 	}
-
-	if (pia->transfer)
-		port_id = port->flow_transfer_proxy;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
 	if (rte_flow_action_handle_update(port_id, action_handle, update,
 					  &error)) {
 		return port_flow_complain(&error);
@@ -1759,14 +1734,6 @@ port_action_handle_query(portid_t port_id, uint32_t id)
 		struct rte_flow_query_age age;
 		struct rte_flow_action_conntrack ct;
 	} query;
-	portid_t port_id_eff = port_id;
-	struct rte_port *port;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
-	port = &ports[port_id];
 
 	pia = action_get_by_id(port_id, id);
 	if (!pia)
@@ -1781,19 +1748,10 @@ port_action_handle_query(portid_t port_id, uint32_t id)
 			id, pia->type, port_id);
 		return -ENOTSUP;
 	}
-
-	if (pia->transfer)
-		port_id_eff = port->flow_transfer_proxy;
-
-	if (port_id_is_invalid(port_id_eff, ENABLED_WARN) ||
-	    port_id_eff == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
 	/* Poisoning to make sure PMDs update it in case of error. */
 	memset(&error, 0x55, sizeof(error));
 	memset(&query, 0, sizeof(query));
-	if (rte_flow_action_handle_query(port_id_eff, pia->handle, &query,
-					 &error))
+	if (rte_flow_action_handle_query(port_id, pia->handle, &query, &error))
 		return port_flow_complain(&error);
 	switch (pia->type) {
 	case RTE_FLOW_ACTION_TYPE_AGE:
@@ -2012,20 +1970,6 @@ port_flow_validate(portid_t port_id,
 {
 	struct rte_flow_error error;
 	struct port_flow_tunnel *pft = NULL;
-	struct rte_port *port;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
-	port = &ports[port_id];
-
-	if (attr->transfer)
-		port_id = port->flow_transfer_proxy;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
 
 	/* Poisoning to make sure PMDs update it in case of error. */
 	memset(&error, 0x11, sizeof(error));
@@ -2079,19 +2023,7 @@ port_flow_create(portid_t port_id,
 	struct port_flow_tunnel *pft = NULL;
 	struct rte_flow_action_age *age = age_action_get(actions);
 
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
 	port = &ports[port_id];
-
-	if (attr->transfer)
-		port_id = port->flow_transfer_proxy;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
 	if (port->flow_list) {
 		if (port->flow_list->id == UINT32_MAX) {
 			fprintf(stderr,
@@ -2155,7 +2087,6 @@ port_flow_destroy(portid_t port_id, uint32_t n, const uint32_t *rule)
 		uint32_t i;
 
 		for (i = 0; i != n; ++i) {
-			portid_t port_id_eff = port_id;
 			struct rte_flow_error error;
 			struct port_flow *pf = *tmp;
 
@@ -2166,15 +2097,7 @@ port_flow_destroy(portid_t port_id, uint32_t n, const uint32_t *rule)
 			 * of error.
 			 */
 			memset(&error, 0x33, sizeof(error));
-
-			if (pf->rule.attr->transfer)
-				port_id_eff = port->flow_transfer_proxy;
-
-			if (port_id_is_invalid(port_id_eff, ENABLED_WARN) ||
-			    port_id_eff == (portid_t)RTE_PORT_ALL)
-				return -EINVAL;
-
-			if (rte_flow_destroy(port_id_eff, pf->flow, &error)) {
+			if (rte_flow_destroy(port_id, pf->flow, &error)) {
 				ret = port_flow_complain(&error);
 				continue;
 			}
@@ -2308,14 +2231,6 @@ port_flow_query(portid_t port_id, uint32_t rule,
 		fprintf(stderr, "Flow rule #%u not found\n", rule);
 		return -ENOENT;
 	}
-
-	if (pf->rule.attr->transfer)
-		port_id = port->flow_transfer_proxy;
-
-	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
-	    port_id == (portid_t)RTE_PORT_ALL)
-		return -EINVAL;
-
 	ret = rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
 			    &name, sizeof(name),
 			    (void *)(uintptr_t)action->type, &error);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index c18942279a..37c67180de 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -578,25 +578,6 @@ eth_rx_metadata_negotiate_mp(uint16_t port_id)
 	}
 }
 
-static void
-flow_pick_transfer_proxy_mp(uint16_t port_id)
-{
-	struct rte_port *port = &ports[port_id];
-	int ret;
-
-	port->flow_transfer_proxy = port_id;
-
-	if (!is_proc_primary())
-		return;
-
-	ret = rte_flow_pick_transfer_proxy(port_id, &port->flow_transfer_proxy,
-					   NULL);
-	if (ret != 0) {
-		fprintf(stderr, "Error picking flow transfer proxy for port %u: %s - ignore\n",
-			port_id, rte_strerror(-ret));
-	}
-}
-
 static int
 eth_dev_configure_mp(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		      const struct rte_eth_conf *dev_conf)
@@ -1569,7 +1550,6 @@ init_config_port_offloads(portid_t pid, uint32_t socket_id)
 	int i;
 
 	eth_rx_metadata_negotiate_mp(pid);
-	flow_pick_transfer_proxy_mp(pid);
 
 	port->dev_conf.txmode = tx_mode;
 	port->dev_conf.rxmode = rx_mode;
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 669ce1e87d..3492ce8217 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -177,8 +177,6 @@ struct port_indirect_action {
 	enum rte_flow_action_type type; /**< Action type. */
 	struct rte_flow_action_handle *handle;	/**< Indirect action handle. */
 	enum age_action_context_type age_type; /**< Age action context type. */
-	/** If true, the action applies to "transfer" flows, and vice versa */
-	bool transfer;
 };
 
 struct port_flow_tunnel {
@@ -251,8 +249,6 @@ struct rte_port {
 	/**< dynamic flags. */
 	uint64_t		mbuf_dynf;
 	const struct rte_eth_rxtx_callback *tx_set_dynf_cb[RTE_MAX_QUEUES_PER_PORT+1];
-	/** Associated port which is supposed to handle "transfer" flows */
-	portid_t		flow_transfer_proxy;
 	struct xstat_display_info xstats_info;
 };
 
diff --git a/app/test-pmd/util.c b/app/test-pmd/util.c
index 8020f999d6..fd98e8b51d 100644
--- a/app/test-pmd/util.c
+++ b/app/test-pmd/util.c
@@ -98,7 +98,6 @@ dump_pkt_burst(uint16_t port_id, uint16_t queue, struct rte_mbuf *pkts[],
 		int ret;
 		struct rte_flow_error error;
 		struct rte_flow_restore_info info = { 0, };
-		struct rte_port *port = &ports[port_id];
 
 		mb = pkts[i];
 		if (rxq_share > 0)
@@ -108,9 +107,7 @@ dump_pkt_burst(uint16_t port_id, uint16_t queue, struct rte_mbuf *pkts[],
 		eth_type = RTE_BE_TO_CPU_16(eth_hdr->ether_type);
 		packet_type = mb->packet_type;
 		is_encapsulation = RTE_ETH_IS_TUNNEL_PKT(packet_type);
-
-		ret = rte_flow_get_restore_info(port->flow_transfer_proxy,
-						mb, &info, &error);
+		ret = rte_flow_get_restore_info(port_id, mb, &info, &error);
 		if (!ret) {
 			MKDUMPSTR(print_buf, buf_size, cur_len,
 				  "restore info:");
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index d8d50539e2..44228cd7d2 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -527,6 +527,15 @@ Show multicast mac addresses added for a specific port::
 
    testpmd> show port (port_id) mcast_macs
 
+show flow transfer proxy port ID for the given port
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Show proxy port ID to use as the 1st argument in commands to
+manage ``transfer`` flows and their indirect components.
+::
+
+   testpmd> show port (port_id) flow transfer proxy
+
 show device info
 ~~~~~~~~~~~~~~~~
 
@@ -3477,6 +3486,10 @@ specified before the ``pattern`` token.
 - ``egress``: rule applies to egress traffic.
 - ``transfer``: apply rule directly to endpoints found in pattern.
 
+Please note that use of ``transfer`` attribute requires that the flow and
+its indirect components be managed via so-called ``transfer`` proxy port.
+See `show flow transfer proxy port ID for the given port`_ for details.
+
 Each instance of an attribute specified several times overrides the previous
 value as shown below (group 4 is used)::
 
-- 
2.30.2