RE: [PATCH 2/2] app/graph: implement L2FWD usecase

[Sunil Kumar Kori <skori@marvell.com>]

[-- Attachment #1: Type: text/plain, Size: 15496 bytes --]

> -----Original Message-----
> From: Rakesh Kudurumalla <rkudurumalla@marvell.com>
> Sent: Thursday, November 23, 2023 11:46 AM
> To: Sunil Kumar Kori <skori@marvell.com>; Rakesh Kudurumalla
> <rkudurumalla@marvell.com>
> Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Nithin
> Kumar Dabilpuram <ndabilpuram@marvell.com>
> Subject: [PATCH 2/2] app/graph: implement L2FWD usecase
> 
> Added l2fwd usecase for graph
> 
Need details about patch. Please refer commit to add l3fwd usecase.

> Signed-off-by: Rakesh Kudurumalla <rkudurumalla@marvell.com>
> ---
>  app/graph/cli.c              |   1 +
>  app/graph/examples/l2fwd.cli |  41 ++++++++++
>  app/graph/graph.c            |   8 +-
>  app/graph/l2fwd.c            | 148 +++++++++++++++++++++++++++++++++++
>  app/graph/l2fwd.h            |  11 +++
>  app/graph/meson.build        |   2 +
>  app/graph/module_api.h       |   2 +
>  app/graph/portfwd.c          | 110 ++++++++++++++++++++++++++
>  app/graph/portfwd.h          |  21 +++++
>  app/graph/portfwd_priv.h     |  14 ++++
>  10 files changed, 357 insertions(+), 1 deletion(-)  create mode 100644
> app/graph/examples/l2fwd.cli  create mode 100644 app/graph/l2fwd.c
> create mode 100644 app/graph/l2fwd.h  create mode 100644
> app/graph/portfwd.c  create mode 100644 app/graph/portfwd.h  create
> mode 100644 app/graph/portfwd_priv.h
> 
1. Split patch into 2 separate patches. One to add new ethdev command and second to add l2fwd usecase.
2. Code related to ethdev command can be moved to ethdev.c, ethdev.h and ethdev_priv.h. No need to add new files.
3. Update " doc/guides/tools/graph.rst " to support l2fwd usecase.

> diff --git a/app/graph/cli.c b/app/graph/cli.c index
> 30b12312d6..76f5b8e670 100644
> --- a/app/graph/cli.c
> +++ b/app/graph/cli.c
> @@ -32,6 +32,7 @@ cmdline_parse_ctx_t modules_ctx[] = {
>  	(cmdline_parse_inst_t *)&ethdev_prom_mode_cmd_ctx,
>  	(cmdline_parse_inst_t *)&ethdev_ip4_cmd_ctx,
>  	(cmdline_parse_inst_t *)&ethdev_ip6_cmd_ctx,
> +	(cmdline_parse_inst_t *)&ethdev_forward_cmd_ctx,
>  	(cmdline_parse_inst_t *)&ethdev_cmd_ctx,
>  	(cmdline_parse_inst_t *)&ethdev_help_cmd_ctx,
>  	(cmdline_parse_inst_t *)&ethdev_rx_cmd_ctx, diff --git
> a/app/graph/examples/l2fwd.cli b/app/graph/examples/l2fwd.cli new file
> mode 100644 index 0000000000..af24a5836a
> --- /dev/null
> +++ b/app/graph/examples/l2fwd.cli
> @@ -0,0 +1,41 @@
> +; SPDX-License-Identifier: BSD-3-Clause ; Copyright(c) 2023 Marvell.
> +
> +;
> +; Graph configuration for given usecase ; graph l2fwd coremask 0xff bsz
> +32 tmo 10 model default pcap_enable 1 num_pcap_pkts 100000 pcap_file
> +/tmp/output.pcap
> +
> +;
> +; Mempools to be attached with ethdev
> +;
> +mempool mempool0 size 8192 buffers 4000 cache 256 numa 0
> +
> +;
> +; DPDK devices and configuration.
> +;
> +; Note: Customize the parameters below to match your setup.
> +;
> +ethdev 0002:01:00.1 rxq 1 txq 8 mempool0 ethdev 0002:01:00.4 rxq 1 txq
> +8 mempool0 ethdev 0002:01:00.6 rxq 1 txq 8 mempool0 ethdev
> 0002:02:00.0
> +rxq 1 txq 8 mempool0
> +
> +;
> +; L2 mac forwarding rules
> +;
> +ethdev forward 0002:01:00.4 0002:02:00.0 ethdev forward 0002:01:00.1
> +0002:01:00.6
> +
> +;
> +; Port-Queue-Core mapping for ethdev_rx node ; ethdev_rx map port
> +0002:02:00.0 queue 0 core 1 ethdev_rx map port 0002:01:00.6 queue 0
> +core 2
> +
> +;
> +; Graph start command to create graph.
> +;
> +; Note: No more command should come after this.
> +;
> +graph start
> diff --git a/app/graph/graph.c b/app/graph/graph.c index
> a65723a196..4e0441f1a7 100644
> --- a/app/graph/graph.c
> +++ b/app/graph/graph.c
> @@ -24,7 +24,7 @@ cmd_graph_help[] = "graph <usecases> bsz <size>
> tmo <ns> coremask <bitmask> "
>  		   "model <rtc | mcd | default> pcap_enable <0 | 1>
> num_pcap_pkts <num>"
>  		   "pcap_file <output_capture_file>";
> 
> -static const char * const supported_usecases[] = {"l3fwd"};
> +static const char * const supported_usecases[] = {"l3fwd", "l2fwd"};
>  struct graph_config graph_config;
>  bool graph_started;
> 
> @@ -273,6 +273,12 @@ cli_graph_start(__rte_unused void *parsed_result,
> __rte_unused struct cmdline *c
>  				break;
>  			}
>  		}
> +		if (!strcmp(graph_config.usecases[i].name, "l2fwd")) {
> +			if (graph_config.usecases[i].enabled) {
> +				rc  = usecase_l2fwd_configure(conf,
> nb_conf, nb_graphs);
> +				break;
> +			}
> +		}
>  	}
> 
>  	if (!rc)
> diff --git a/app/graph/l2fwd.c b/app/graph/l2fwd.c new file mode 100644
> index 0000000000..1f18d69ea8
> --- /dev/null
> +++ b/app/graph/l2fwd.c
> @@ -0,0 +1,148 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 Marvell.
> + */
> +
> +#include <errno.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include <rte_common.h>
> +#include <rte_ethdev.h>
> +#include <rte_graph.h>
> +#include <rte_graph_worker.h>
> +#include <rte_lcore.h>
> +#include <rte_node_eth_api.h>
> +
> +#include "module_api.h"
> +
> +static int
> +l2fwd_pattern_configure(void)
> +{
> +	struct rte_graph_param graph_conf;
> +	const char **node_patterns;
> +	uint64_t pcap_pkts_count;
> +	struct lcore_conf *qconf;
> +	uint16_t nb_patterns;
> +	uint8_t pcap_ena;
> +	char *pcap_file;
> +	int lcore_id;
> +
> +	nb_patterns = 0;
> +	node_patterns = malloc((ETHDEV_RX_QUEUE_PER_LCORE_MAX +
> nb_patterns) *
> +			sizeof(*node_patterns));
> +	if (!node_patterns)
> +		return -ENOMEM;
> +
> +	memset(&graph_conf, 0, sizeof(graph_conf));
> +	graph_conf.node_patterns = node_patterns;
> +
> +	/* Pcap config */
> +	graph_pcap_config_get(&pcap_ena, &pcap_pkts_count,
> &pcap_file);
> +	graph_conf.pcap_enable = pcap_ena;
> +	graph_conf.num_pkt_to_capture = pcap_pkts_count;
> +	graph_conf.pcap_filename = strdup(pcap_file);
> +
> +	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
> +		rte_graph_t graph_id;
> +		rte_edge_t i;
> +
> +		if (rte_lcore_is_enabled(lcore_id) == 0)
> +			continue;
> +
> +		qconf = &lcore_conf[lcore_id];
> +
> +		/* Skip graph creation if no source exists */
> +		if (!qconf->n_rx_queue)
> +			continue;
> +
> +		/* Add rx node patterns of this lcore */
> +		for (i = 0; i < qconf->n_rx_queue; i++) {
> +			graph_conf.node_patterns[nb_patterns + i] =
> +				qconf->rx_queue_list[i].node_name;
> +		}
> +
> +		graph_conf.nb_node_patterns = nb_patterns + i;
> +		graph_conf.socket_id = rte_lcore_to_socket_id(lcore_id);
> +
> +		snprintf(qconf->name, sizeof(qconf->name), "worker_%u",
> +				lcore_id);
> +
> +		graph_id = rte_graph_create(qconf->name, &graph_conf);
> +		if (graph_id == RTE_GRAPH_ID_INVALID)
> +			rte_exit(EXIT_FAILURE,
> +					"rte_graph_create(): graph_id invalid"
> +					" for lcore %u\n", lcore_id);
> +
> +		qconf->graph_id = graph_id;
> +		qconf->graph = rte_graph_lookup(qconf->name);
> +		/* >8 End of graph initialization. */
> +		if (!qconf->graph)
> +			rte_exit(EXIT_FAILURE,
> +					"rte_graph_lookup(): graph %s not
> found\n",
> +					qconf->name);
> +	}
> +
> +	/* Launch per-lcore init on every worker lcore */
> +	rte_eal_mp_remote_launch(graph_walk_start, NULL, SKIP_MAIN);
> +
> +	/* Accumulate and print stats on main until exit */
> +	if (rte_graph_has_stats_feature() && app_graph_stats_enabled())
> +		graph_stats_print();
> +
> +	return 0;
> +}
> +
> +static int
> +ethdev_rx_to_tx_node_link(uint32_t lcore_id) {
> +	struct port_forwarding *port_fwd = NULL;
> +	char name[RTE_NODE_NAMESIZE];
> +	const char *next_node = name;
> +	struct lcore_conf *qconf;
> +	uint16_t queue, port_id;
> +	rte_node_t rx_id;
> +	int rc;
> +
> +	qconf = &lcore_conf[lcore_id];
> +
> +	for (queue = 0; queue < qconf->n_rx_queue; ++queue) {
> +		port_id = qconf->rx_queue_list[queue].port_id;
> +		port_fwd = find_pf_entry_rx_port(port_id);
Here IMO, Instead of returning port_forwarding information from find_pf_entry_rx_port, return tx_port only. That will solve purpose.
This way, we move port_forwarding structure to private header.


> +		if (port_fwd) {
> +			rx_id = rte_node_from_name(qconf-
> >rx_queue_list[queue].node_name);
> +			snprintf(name, sizeof(name), "ethdev_tx-%u",
> port_fwd->tx_port);
> +			rte_node_edge_update(rx_id,
> RTE_EDGE_ID_INVALID, &next_node, 1);
> +			rc = rte_node_ethdev_rx_next_update(rx_id, name);
> +			if (rc)
> +				return rc;
> +		}
> +	}
> +	return 0;
> +}
> +
> +
> +int
> +usecase_l2fwd_configure(struct rte_node_ethdev_config *conf, uint16_t
> +nb_confs, uint16_t nb_graphs) {
> +	uint32_t lcore_id;
> +	int rc;
> +
> +	rc = rte_node_eth_config(conf, nb_confs, nb_graphs);
> +	if (rc)
> +		rte_exit(EXIT_FAILURE, "rte_node_eth_config: err=%d\n", rc);
> +
> +	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
> +		rc = ethdev_rx_to_tx_node_link(lcore_id);
> +		if (rc)
> +			rte_exit(EXIT_FAILURE, "rte_node_eth_config:
> err=%d\n", rc);
> +	}
> +
> +	rc = l2fwd_pattern_configure();
> +	if (rc)
> +		rte_exit(EXIT_FAILURE, "l2fwd_pattern_failure: err=%d\n",
> rc);
> +
> +	return rc;
> +}
> diff --git a/app/graph/l2fwd.h b/app/graph/l2fwd.h new file mode 100644
> index 0000000000..3486ce52b2
> --- /dev/null
> +++ b/app/graph/l2fwd.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 Marvell.
> + */
> +
> +#ifndef APP_GRAPH_L2FWD_H
> +#define APP_GRAPH_L2FWD_H
> +
> +int usecase_l2fwd_configure(struct rte_node_ethdev_config *conf,
> uint16_t nb_conf,
> +			    uint16_t nb_graphs);
> +
> +#endif
> diff --git a/app/graph/meson.build b/app/graph/meson.build index
> 5b0f966d99..d8b1f63988 100644
> --- a/app/graph/meson.build
> +++ b/app/graph/meson.build
> @@ -17,9 +17,11 @@ sources = files(
>          'graph.c',
>          'ip4_route.c',
>          'ip6_route.c',
> +        'l2fwd.c',
>          'l3fwd.c',
>          'main.c',
>          'mempool.c',
>          'neigh.c',
> +        'portfwd.c',
>          'utils.c',
>  )
> diff --git a/app/graph/module_api.h b/app/graph/module_api.h index
> 7193e0b616..751d6cfb3b 100644
> --- a/app/graph/module_api.h
> +++ b/app/graph/module_api.h
> @@ -13,9 +13,11 @@
>  #include "ethdev.h"
>  #include "ethdev_rx.h"
>  #include "graph.h"
> +#include "l2fwd.h"
>  #include "l3fwd.h"
>  #include "mempool.h"
>  #include "neigh.h"
> +#include "portfwd.h"
>  #include "route.h"
>  #include "utils.h"
> 
> diff --git a/app/graph/portfwd.c b/app/graph/portfwd.c new file mode
> 100644 index 0000000000..6c4b54e1db
> --- /dev/null
> +++ b/app/graph/portfwd.c
> @@ -0,0 +1,110 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 Marvell.
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include <cmdline_parse.h>
> +#include <cmdline_parse_num.h>
> +#include <cmdline_parse_string.h>
> +#include <cmdline_socket.h>
> +#include <rte_ethdev.h>
> +#include <rte_node_ip4_api.h>
> +
> +#include "module_api.h"
> +#include "portfwd_priv.h"
> +
> +static const char
> +cmd_ethdev_l2fwd_help[] = "ethdev forward <tx_dev_name>
Replace with cmd_ethdev_forward_help.

> <rx_dev_name>";
> +
> +static struct prt_fw pfw = TAILQ_HEAD_INITIALIZER(pfw);
> +
> +struct port_forwarding *
> +find_pf_entry_rx_port(uint16_t portid_rx) {
> +	struct port_forwarding *port_fwd;
> +
> +	TAILQ_FOREACH(port_fwd, &pfw, next) {
> +		if (port_fwd->rx_port == portid_rx)
> +			return port_fwd;
> +	}
> +	return NULL;
> +}
> +
> +static struct port_forwarding *
> +find_l2_entry(uint16_t portid_tx, uint16_t portid_rx) {
> +	struct port_forwarding *port_fwd;
Use correct naming convention i.e. action should be at last. Please refer other APIs/function. 

> +
> +	TAILQ_FOREACH(port_fwd, &pfw, next) {
> +		if ((port_fwd->tx_port == portid_tx) &&
> +				(port_fwd->rx_port == portid_rx))
> +			return port_fwd;
> +	}
> +	return NULL;
> +}
> +
> +static int
> +ethdev_pfw_config(char *tx_name, char *rx_name) {
Replace with ethdev_forward_config()

> +	struct port_forwarding *pfwd;
> +	uint16_t portid_rx = 0;
> +	uint16_t portid_tx = 0;
> +	int rc;
> +
> +	rc = rte_eth_dev_get_port_by_name(tx_name, &portid_tx);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = rte_eth_dev_get_port_by_name(rx_name, &portid_rx);
> +	if (rc < 0)
> +		return rc;
> +
> +	pfwd = find_l2_entry(portid_tx, portid_rx);
> +	if (!pfwd) {
> +		pfwd = malloc(sizeof(struct port_forwarding));
> +		pfwd->tx_port = portid_tx;
> +		pfwd->rx_port = portid_rx;
> +		TAILQ_INSERT_TAIL(&pfw, pfwd, next);
> +		return 0;
> +	}
> +
> +	return rc;
> +}
> +
> +static void
> +cli_ethdev_forward(void *parsed_result, __rte_unused struct cmdline
> +*cl, void *data __rte_unused) {
> +	struct ethdev_fwd_cmd_tokens *res = parsed_result;
> +	int rc = -EINVAL;
> +
> +	rc = ethdev_pfw_config(res->tx_dev, res->rx_dev);
> +	if (rc < 0)
> +		printf(MSG_CMD_FAIL, res->cmd);
> +}
> +
> +
> +cmdline_parse_token_string_t ethdev_l2_cmd =
> +	TOKEN_STRING_INITIALIZER(struct ethdev_fwd_cmd_tokens, cmd,
> "ethdev");
> +cmdline_parse_token_string_t ethdev_fwd_cmd =
> +	TOKEN_STRING_INITIALIZER(struct ethdev_fwd_cmd_tokens, fwd,
> +"forward"); cmdline_parse_token_string_t ethdev_tx_device =
> +	TOKEN_STRING_INITIALIZER(struct ethdev_fwd_cmd_tokens,
> tx_dev, NULL);
> +cmdline_parse_token_string_t ethdev_rx_device =
> +	TOKEN_STRING_INITIALIZER(struct ethdev_fwd_cmd_tokens,
> rx_dev, NULL);
> +
> +cmdline_parse_inst_t ethdev_forward_cmd_ctx = {
> +	.f = cli_ethdev_forward,
> +	.data = NULL,
> +	.help_str = cmd_ethdev_l2fwd_help,
> +	.tokens = {
> +		(void *)&ethdev_l2_cmd,
> +		(void *)&ethdev_fwd_cmd,
> +		(void *)&ethdev_tx_device,
> +		(void *)&ethdev_rx_device,
> +		NULL,
> +	},
> +};
> +
> diff --git a/app/graph/portfwd.h b/app/graph/portfwd.h new file mode
> 100644 index 0000000000..420fa73746
> --- /dev/null
> +++ b/app/graph/portfwd.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 Marvell.
> + */
> +
> +#ifndef APP_GRAPH_PORTFWD_H
> +#define APP_GRAPH_PORTFWD_H
> +
> +extern cmdline_parse_inst_t ethdev_forward_cmd_ctx;
> +
> +struct port_forwarding {
> +	TAILQ_ENTRY(port_forwarding) next;
> +	uint16_t tx_port;
> +	uint16_t rx_port;
> +	bool is_used;
Looks like "is_used" is unused. Please check.
Can't be this mapping in " struct ethdev_config". IMO, " struct ethdev_config" is used to contain device specific configuration.
Is there any other purpose of "struct port_forwarding" ?

> +} __rte_cache_aligned;
> +
> +TAILQ_HEAD(prt_fw, port_forwarding);
> +
> +struct port_forwarding *find_pf_entry_rx_port(uint16_t portid_rx);
> +
> +#endif
> diff --git a/app/graph/portfwd_priv.h b/app/graph/portfwd_priv.h new file
> mode 100644 index 0000000000..9da1ce6e68
> --- /dev/null
> +++ b/app/graph/portfwd_priv.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 Marvell.
> + */
> +
> +#ifndef APP_GRAPH_PORTFWD_PRIV_H
> +#define APP_GRAPH_PORTFWD_PRIV_H
> +
> +struct ethdev_fwd_cmd_tokens {
> +	cmdline_fixed_string_t cmd;
> +	cmdline_fixed_string_t fwd;
> +	cmdline_fixed_string_t tx_dev;
> +	cmdline_fixed_string_t rx_dev;
> +};
> +#endif
> --
> 2.25.1


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 41303 bytes --]