Re: [dpdk-dev] [PATCH 4/6] examples/ntb: enable an example for ntb

["Li, Xiaoyun" <xiaoyun.li@intel.com>]

OK. Will fix them in next version including other patches' comments. Thanks.

> -----Original Message-----
> From: Ye, Xiaolong
> Sent: Tuesday, June 4, 2019 14:48
> To: Li, Xiaoyun <xiaoyun.li@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Wiles, Keith <keith.wiles@intel.com>;
> Liang, Cunming <cunming.liang@intel.com>; Maslekar, Omkar
> <omkar.maslekar@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 4/6] examples/ntb: enable an example for ntb
> 
> On 06/03, Xiaoyun Li wrote:
> >Enable an example for rawdev ntb. Support interactive mode to send file
> >on one host and receive file from another host. The command line would
> >be 'send [filepath]' and 'receive [filepath]'.
> >
> >But since the FIFO is not enabled right now, use rte_memcpy as the
> >enqueue and dequeue functions and only support transmitting file no more
> than 4M.
> >
> >Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> >---
> > drivers/raw/ntb_rawdev/ntb_rawdev.c |  25 +-
> > examples/Makefile                   |   1 +
> > examples/meson.build                |  21 +-
> > examples/ntb/Makefile               |  68 ++++++
> > examples/ntb/meson.build            |  16 ++
> > examples/ntb/ntb_fwd.c              | 364 ++++++++++++++++++++++++++++
> > 6 files changed, 477 insertions(+), 18 deletions(-)  create mode
> >100644 examples/ntb/Makefile  create mode 100644
> >examples/ntb/meson.build  create mode 100644 examples/ntb/ntb_fwd.c
> >
> >diff --git a/drivers/raw/ntb_rawdev/ntb_rawdev.c
> >b/drivers/raw/ntb_rawdev/ntb_rawdev.c
> >index 35bc34c54..1824842f2 100644
> >--- a/drivers/raw/ntb_rawdev/ntb_rawdev.c
> >+++ b/drivers/raw/ntb_rawdev/ntb_rawdev.c
> >@@ -212,11 +212,16 @@ ntb_enqueue_bufs(struct rte_rawdev *dev,
> > 		 unsigned int count,
> > 		 rte_rawdev_obj_t context)
> > {
> >-	RTE_SET_USED(dev);
> >-	RTE_SET_USED(buffers);
> >-	RTE_SET_USED(count);
> >-	RTE_SET_USED(context);
> >+	/* Not FIFO right now. Just for test memory write. */
> >+	struct ntb_hw *hw = dev->dev_private;
> >+	uint64_t bar_addr, size;
> >+	unsigned int i;
> >+
> >+	bar_addr = (*hw->ntb_ops->get_peer_mw_addr)(dev, 0);
> 
> check the func pointer before calling it.
> 
> >+	size = (uint64_t)context;
> >
> >+	for (i = 0; i < count; i++)
> >+		rte_memcpy((void *)bar_addr, buffers[i]->buf_addr, size);
> > 	return 0;
> > }
> >
> >@@ -226,11 +231,15 @@ ntb_dequeue_bufs(struct rte_rawdev *dev,
> > 		 unsigned int count,
> > 		 rte_rawdev_obj_t context)
> > {
> >-	RTE_SET_USED(dev);
> >-	RTE_SET_USED(buffers);
> >-	RTE_SET_USED(count);
> >-	RTE_SET_USED(context);
> >+	/* Not FIFO. Just for test memory read. */
> >+	struct ntb_hw *hw = dev->dev_private;
> >+	uint64_t size;
> >+	unsigned int i;
> >+
> >+	size = (uint64_t)context;
> >
> >+	for (i = 0; i < count; i++)
> >+		rte_memcpy(buffers[i]->buf_addr, hw->mz[i]->addr, size);
> > 	return 0;
> > }
> >
> >diff --git a/examples/Makefile b/examples/Makefile index
> >7562424d9..de11dd487 100644
> >--- a/examples/Makefile
> >+++ b/examples/Makefile
> >@@ -53,6 +53,7 @@ DIRS-y += link_status_interrupt
> > DIRS-$(CONFIG_RTE_LIBRTE_LPM) += load_balancer  DIRS-y +=
> >multi_process  DIRS-y += netmap_compat/bridge
> >+DIRS-y += ntb
> > DIRS-$(CONFIG_RTE_LIBRTE_REORDER) += packet_ordering  ifeq
> >($(CONFIG_RTE_ARCH_X86_64),y)  DIRS-y += performance-thread diff --git
> >a/examples/meson.build b/examples/meson.build index
> >de35656d4..dda4a07a8 100644
> >--- a/examples/meson.build
> >+++ b/examples/meson.build
> >@@ -25,16 +25,17 @@ all_examples = [
> > 	'l3fwd-acl', 'l3fwd-power',
> > 	'l3fwd-vf', 'link_status_interrupt',
> > 	'load_balancer', 'multi_process',
> >-	'netmap_compat', 'packet_ordering',
> >-	'performance-thread', 'ptpclient',
> >-	'qos_meter', 'qos_sched',
> >-	'quota_watermark', 'rxtx_callbacks',
> >-	'server_node_efd', 'service_cores',
> >-	'skeleton', 'tep_termination',
> >-	'timer', 'vdpa',
> >-	'vhost', 'vhost_crypto',
> >-	'vhost_scsi', 'vm_power_manager',
> >-	'vmdq', 'vmdq_dcb',
> >+	'netmap_compat', 'ntb',
> >+	'packet_ordering', 'performance-thread',
> >+	'ptpclient', 'qos_meter',
> >+	'qos_sched', 'quota_watermark',
> >+	'rxtx_callbacks', 'server_node_efd',
> >+	'service_cores', 'skeleton',
> >+	'tep_termination', 'timer',
> >+	'vdpa', 'vhost',
> >+	'vhost_crypto', 'vhost_scsi',
> >+	'vm_power_manager', 'vmdq',
> >+	'vmdq_dcb',
> > ]
> > # install all example code on install - irrespective of whether the
> >example in  # question is to be built as part of this build or not.
> >diff --git a/examples/ntb/Makefile b/examples/ntb/Makefile new file
> >mode 100644 index 000000000..5ddd9b95f
> >--- /dev/null
> >+++ b/examples/ntb/Makefile
> >@@ -0,0 +1,68 @@
> >+# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2019 Intel
> >+Corporation
> >+
> >+# binary name
> >+APP = ntb_fwd
> >+
> >+# all source are stored in SRCS-y
> >+SRCS-y := ntb_fwd.c
> >+
> >+# Build using pkg-config variables if possible $(shell pkg-config
> >+--exists libdpdk) ifeq ($(.SHELLSTATUS),0)
> >+
> >+all: shared
> >+.PHONY: shared static
> >+shared: build/$(APP)-shared
> >+	ln -sf $(APP)-shared build/$(APP)
> >+static: build/$(APP)-static
> >+	ln -sf $(APP)-static build/$(APP)
> >+
> >+CFLAGS += -D_FILE_OFFSET_BITS=64
> >+LDFLAGS += -pthread
> >+
> >+PC_FILE := $(shell pkg-config --path libdpdk) CFLAGS += -O3 $(shell
> >+pkg-config --cflags libdpdk) LDFLAGS_SHARED = $(shell pkg-config
> >+--libs libdpdk) LDFLAGS_STATIC = -Wl,-Bstatic $(shell pkg-config
> >+--static --libs libdpdk)
> >+
> >+build/$(APP)-shared: $(SRCS-y) Makefile $(PC_FILE) | build
> >+	$(CC) $(CFLAGS) $(SRCS-y) -o $@ $(LDFLAGS) $(LDFLAGS_SHARED)
> >+
> >+build/$(APP)-static: $(SRCS-y) Makefile $(PC_FILE) | build
> >+	$(CC) $(CFLAGS) $(SRCS-y) -o $@ $(LDFLAGS) $(LDFLAGS_STATIC)
> >+
> >+build:
> >+	@mkdir -p $@
> >+
> >+.PHONY: clean
> >+clean:
> >+	rm -f build/$(APP) build/$(APP)-static build/$(APP)-shared
> >+	rmdir --ignore-fail-on-non-empty build
> >+
> >+else # Build using legacy build system
> >+
> >+ifeq ($(RTE_SDK),)
> >+$(error "Please define RTE_SDK environment variable") endif
> >+
> >+# Default target, can be overridden by command line or environment
> >+RTE_TARGET ?= x86_64-native-linuxapp-gcc
> >+
> >+include $(RTE_SDK)/mk/rte.vars.mk
> >+
> >+ifneq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
> >+$(info This application can only operate in a linuxapp environment, \
> >+please change the definition of the RTE_TARGET environment variable)
> >+all:
> >+else
> >+
> >+CFLAGS += -D_FILE_OFFSET_BITS=64
> >+CFLAGS += -O2
> >+CFLAGS += $(WERROR_FLAGS)
> >+CFLAGS += -DALLOW_EXPERIMENTAL_API
> >+
> >+include $(RTE_SDK)/mk/rte.extapp.mk
> >+
> >+endif
> >+endif
> >diff --git a/examples/ntb/meson.build b/examples/ntb/meson.build new
> >file mode 100644 index 000000000..9a6288f4f
> >--- /dev/null
> >+++ b/examples/ntb/meson.build
> >@@ -0,0 +1,16 @@
> >+# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2019 Intel
> >+Corporation
> >+
> >+# meson file, for building this example as part of a main DPDK build.
> >+#
> >+# To build this example as a standalone application with an
> >+already-installed # DPDK instance, use 'make'
> >+
> >+if host_machine.system() != 'linux'
> >+	build = false
> >+endif
> >+deps += 'rawdev'
> >+cflags += ['-D_FILE_OFFSET_BITS=64']
> >+sources = files(
> >+	'ntb_fwd.c'
> >+)
> >diff --git a/examples/ntb/ntb_fwd.c b/examples/ntb/ntb_fwd.c new file
> >mode 100644 index 000000000..7835d817a
> >--- /dev/null
> >+++ b/examples/ntb/ntb_fwd.c
> >@@ -0,0 +1,364 @@
> >+/* SPDX-License-Identifier: BSD-3-Clause
> >+ * Copyright(c) 2019 Intel Corporation  */ #include <stdint.h>
> >+#include <stdio.h> #include <unistd.h> #include <signal.h> #include
> >+<string.h> #include <getopt.h>
> >+
> >+#include <cmdline_parse_string.h>
> >+#include <cmdline_socket.h>
> >+#include <cmdline.h>
> >+#include <rte_common.h>
> >+#include <rte_rawdev.h>
> >+#include <rte_lcore.h>
> >+
> >+static uint64_t max_file_size = 0x400000; static uint8_t interactive =
> >+1; static uint16_t dev_id;
> >+
> >+/* *** Help command with introduction. *** */ struct cmd_help_result {
> >+	cmdline_fixed_string_t help;
> >+};
> >+
> >+static void cmd_help_parsed(__attribute__((unused)) void *parsed_result,
> >+			    struct cmdline *cl,
> >+			    __attribute__((unused)) void *data) {
> >+	cmdline_printf(
> >+		cl,
> >+		"\n"
> >+		"The following commands are currently available:\n\n"
> >+		"Control:\n"
> >+		"    quit                                      :"
> >+		" Quit the application.\n"
> >+		"\nFile transmit:\n"
> >+		"    send [path]                               :"
> >+		" Send [path] file. (No more than %lu)\n"
> >+		"    recv [path]                            :"
> >+		" Receive file to [path]. Make sure sending is done"
> >+		" on the other side.\n",
> >+		max_file_size
> >+	);
> >+
> >+}
> >+
> >+cmdline_parse_token_string_t cmd_help_help =
> >+	TOKEN_STRING_INITIALIZER(struct cmd_help_result, help, "help");
> >+
> >+cmdline_parse_inst_t cmd_help = {
> >+	.f = cmd_help_parsed,
> >+	.data = NULL,
> >+	.help_str = "show help",
> >+	.tokens = {
> >+		(void *)&cmd_help_help,
> >+		NULL,
> >+	},
> >+};
> >+
> >+/* *** QUIT *** */
> >+struct cmd_quit_result {
> >+	cmdline_fixed_string_t quit;
> >+};
> >+
> >+static void cmd_quit_parsed(__attribute__((unused)) void *parsed_result,
> >+			    struct cmdline *cl,
> >+			    __attribute__((unused)) void *data) {
> >+	/* Stop traffic and Close port. */
> >+	rte_rawdev_stop(dev_id);
> >+	rte_rawdev_close(dev_id);
> >+
> >+	cmdline_quit(cl);
> >+}
> >+
> >+cmdline_parse_token_string_t cmd_quit_quit =
> >+		TOKEN_STRING_INITIALIZER(struct cmd_quit_result, quit,
> "quit");
> >+
> >+cmdline_parse_inst_t cmd_quit = {
> >+	.f = cmd_quit_parsed,
> >+	.data = NULL,
> >+	.help_str = "exit application",
> >+	.tokens = {
> >+		(void *)&cmd_quit_quit,
> >+		NULL,
> >+	},
> >+};
> >+
> >+/* *** SEND FILE PARAMETERS *** */
> >+struct cmd_sendfile_result {
> >+	cmdline_fixed_string_t send_string;
> >+	char filepath[];
> >+};
> >+
> >+static void
> >+cmd_sendfile_parsed(void *parsed_result,
> >+		    __attribute__((unused)) struct cmdline *cl,
> >+		    __attribute__((unused)) void *data) {
> >+	struct cmd_sendfile_result *res = parsed_result;
> >+	struct rte_rawdev_buf *pkts_send[1];
> >+	uint64_t rsize, size, link;
> >+	char *filepath;
> >+	uint8_t *buff;
> >+	uint32_t val;
> >+	FILE *file;
> >+
> >+	if (!rte_rawdevs[dev_id].started)
> >+		printf("Device needs to be up first. Try later.\n");
> 
> I think we can directly return here when the device is not started, instead of
> continuing the execution.
> 
> >+
> >+	rte_rawdev_get_attr(dev_id, "link_status", &link);
> >+	if (!link)
> >+		printf("Link is not up, cannot send file.\n");
> 
> return if link is down.
> 
> >+
> >+	filepath = strdup(res->filepath);
> >+	if (filepath == NULL) {
> >+		printf("Fail to get filepath.\n");
> >+		return;
> >+	}
> >+
> >+	file = fopen(filepath, "r");
> 
> Actually I was thinking why we need the filepath, why not just
> 
> 	file = fopen(res->filepath, "r")
> 
> Then we don't need to handle filepath free at all.
> 
> >+	if (!file) {
> 
> style 1.8.1
> 
> >+		printf("Fail to open the file.\n");
> 
> Need to free filepath in this error handling.
> 
> >+		return;
> >+	}
> >+
> >+	fseek(file, 0, SEEK_END);
> >+	size = ftell(file);
> >+	fseek(file, 0, SEEK_SET);
> >+
> >+	/**
> >+	 * No FIFO now. Only test memory. Limit sending file
> >+	 * size <= max_file_size.
> >+	 */
> >+	if (size > max_file_size)
> >+		size = max_file_size;
> 
> It's better to give the user some reminder info about the max size limition,
> rather than truncate the file silently.
> 
> >+
> >+	buff = (uint8_t *)malloc(size);
> >+	rsize = fread(buff, size, 1, file);
> >+	if (rsize != 1) {
> >+		printf("Fail to read file.\n");
> >+		fclose(file);
> 
> Need to free filepath and buff.
> 
> >+		return;
> >+	}
> >+
> >+	/* Tell remote about the file size. */
> >+	val = size >> 32;
> >+	rte_rawdev_set_attr(dev_id, "spad14", val);
> >+	val = size;
> >+	rte_rawdev_set_attr(dev_id, "spad15", val);
> >+
> >+	pkts_send[0] = (struct rte_rawdev_buf *)malloc
> >+			(sizeof(struct rte_rawdev_buf));
> >+	pkts_send[0]->buf_addr = buff;
> >+	rte_rawdev_enqueue_buffers(dev_id, pkts_send, 1, (void *)size);
> 
> May need to check the return value.
> 
> >+	printf("Done sending file.\n");
> >+
> >+	fclose(file);
> >+	free((void *)filepath);
> 
> Need to free allocated pkts_send[0] and buff.
> 
> >+}
> >+
> >+cmdline_parse_token_string_t cmd_send_file_send =
> >+	TOKEN_STRING_INITIALIZER(struct cmd_sendfile_result, send_string,
> >+				 "send");
> >+cmdline_parse_token_string_t cmd_send_file_filepath =
> >+	TOKEN_STRING_INITIALIZER(struct cmd_sendfile_result, filepath, NULL);
> >+
> >+
> >+cmdline_parse_inst_t cmd_send_file = {
> >+	.f = cmd_sendfile_parsed,
> >+	.data = NULL,
> >+	.help_str = "send <file_path>",
> >+	.tokens = {
> >+		(void *)&cmd_send_file_send,
> >+		(void *)&cmd_send_file_filepath,
> >+		NULL,
> >+	},
> >+};
> >+
> >+/* *** RECEIVE FILE PARAMETERS *** */
> >+struct cmd_recvfile_result {
> >+	cmdline_fixed_string_t recv_string;
> >+	char filepath[];
> >+};
> >+
> >+static void
> >+cmd_recvfile_parsed(void *parsed_result,
> >+		    __attribute__((unused)) struct cmdline *cl,
> >+		    __attribute__((unused)) void *data) {
> >+	struct cmd_sendfile_result *res = parsed_result;
> >+	struct rte_rawdev_buf *pkts_recv[1];
> >+	uint64_t size, val;
> >+	char *filepath;
> >+	uint8_t *buff;
> >+	FILE *file;
> >+
> >+	if (!rte_rawdevs[dev_id].started)
> >+		printf("Device needs to be up first. Try later.\n");
> 
> return directly.
> 
> >+
> >+	rte_rawdev_get_attr(dev_id, "link_status", &val);
> >+	if (!val)
> >+		printf("Link is not up, cannot receive file.\n");
> 
> return directly.
> 
> >+
> >+	filepath = strdup(res->filepath);
> >+	if (filepath == NULL) {
> >+		printf("Fail to get filepath.\n");
> >+		return;
> >+	}
> >+
> >+	file = fopen(filepath, "w");
> >+	if (!file) {
> 
> style 1.8.1
> 
> >+		printf("Fail to open the file.\n");
> >+		return;
> >+	}
> >+
> >+	rte_rawdev_get_attr(dev_id, "spad14", &val);
> >+	size = val << 32;
> >+	rte_rawdev_get_attr(dev_id, "spad15", &val);
> >+	size |= val;
> >+
> >+	buff = (uint8_t *)malloc(size);
> >+	pkts_recv[0] = (struct rte_rawdev_buf *)malloc
> >+			(sizeof(struct rte_rawdev_buf));
> >+	pkts_recv[0]->buf_addr = buff;
> >+
> >+	rte_rawdev_dequeue_buffers(dev_id, pkts_recv, 1, (void *)size);
> >+
> >+	fwrite(buff, size, 1, file);
> >+	printf("Done receiving to file.\n");
> >+
> >+	fclose(file);
> >+	free((void *)filepath);
> 
> free buff and pkts_recv[0] as well.
> 
> I noticed that there are two allocations, one for pkts_recv[0] and another for
> pkts_recv[0]->buf_addr, How about we declare
> 
> 	struct rte_rawdev_buf pkts_recv[1];
> 
> and allocate memory for its buf_addr
> 
> 	pkts_recv[0].buf_addr = malloc(size);
> 
> then we call
> 
> 	rte_rawdev_dequeue_buffers(dev_id, &pkts_recv, 1, (void *)size);
> 
> After that, we just need to free once.
> 
> 	free(pkts_recv[0].buf_addr);
> 
> 
> >+}
> >+
> >+cmdline_parse_token_string_t cmd_recv_file_recv =
> >+	TOKEN_STRING_INITIALIZER(struct cmd_recvfile_result, recv_string,
> >+				 "recv");
> >+cmdline_parse_token_string_t cmd_recv_file_filepath =
> >+	TOKEN_STRING_INITIALIZER(struct cmd_recvfile_result, filepath, NULL);
> >+
> >+
> >+cmdline_parse_inst_t cmd_recv_file = {
> >+	.f = cmd_recvfile_parsed,
> >+	.data = NULL,
> >+	.help_str = "recv <file_path>",
> >+	.tokens = {
> >+		(void *)&cmd_recv_file_recv,
> >+		(void *)&cmd_recv_file_filepath,
> >+		NULL,
> >+	},
> >+};
> >+
> >+/* list of instructions */
> >+cmdline_parse_ctx_t main_ctx[] = {
> >+	(cmdline_parse_inst_t *)&cmd_help,
> >+	(cmdline_parse_inst_t *)&cmd_send_file,
> >+	(cmdline_parse_inst_t *)&cmd_recv_file,
> >+	(cmdline_parse_inst_t *)&cmd_quit,
> >+	NULL,
> >+};
> >+
> >+/* prompt function, called from main on MASTER lcore */ static void
> >+prompt(void)
> >+{
> >+	struct cmdline *cl;
> >+
> >+	cl = cmdline_stdin_new(main_ctx, "ntb> ");
> >+	if (cl == NULL)
> >+		return;
> >+
> >+	cmdline_interact(cl);
> >+	cmdline_stdin_exit(cl);
> >+}
> >+
> >+static void
> >+signal_handler(int signum)
> >+{
> >+	if (signum == SIGINT || signum == SIGTERM) {
> >+		printf("\nSignal %d received, preparing to exit...\n", signum);
> >+		signal(signum, SIG_DFL);
> >+		kill(getpid(), signum);
> >+	}
> >+}
> >+
> >+static void
> >+ntb_usage(const char *prgname)
> >+{
> >+	printf("%s [EAL options] -- [options]\n"
> >+	       "-i : run in interactive mode (default value is 1)\n",
> >+	       prgname);
> >+}
> >+
> >+static int
> >+parse_args(int argc, char **argv)
> >+{
> >+	char *prgname = argv[0], **argvopt = argv;
> >+	int opt, ret;
> >+
> >+	/* Only support interactive mode to send/recv file first. */
> >+	while ((opt = getopt(argc, argvopt, "i")) != EOF) {
> >+		switch (opt) {
> >+		case 'i':
> >+			printf("Interactive-mode selected\n");
> >+			interactive = 1;
> >+			break;
> >+
> >+		default:
> >+			ntb_usage(prgname);
> >+			return -1;
> >+		}
> >+	}
> >+
> >+	if (optind >= 0)
> >+		argv[optind-1] = prgname;
> >+
> >+	ret = optind-1;
> >+	optind = 1; /* reset getopt lib */
> >+	return ret;
> >+}
> >+
> >+int
> >+main(int argc, char **argv)
> >+{
> >+	int ret, i;
> >+
> >+	signal(SIGINT, signal_handler);
> >+	signal(SIGTERM, signal_handler);
> >+
> >+	ret = rte_eal_init(argc, argv);
> >+	if (ret < 0)
> >+		rte_exit(EXIT_FAILURE, "Error with EAL initialization.\n");
> >+
> >+	/* Find 1st ntb rawdev. */
> >+	for (i = 0; i < RTE_RAWDEV_MAX_DEVS; i++)
> >+		if (rte_rawdevs[i].driver_name &&
> >+		    (strncmp(rte_rawdevs[i].driver_name, "raw_ntb", 7) == 0) &&
> 
> Use Macro for the 7.
> 
> >+		    (rte_rawdevs[i].attached == 1))
> >+			break;
> >+
> >+	if (i == RTE_RAWDEV_MAX_DEVS)
> >+		rte_exit(EXIT_FAILURE, "Cannot find any ntb device.\n");
> >+
> >+	dev_id = i;
> >+
> >+	argc -= ret;
> >+	argv += ret;
> >+
> >+	ret = parse_args(argc, argv);
> >+	if (ret < 0)
> >+		rte_exit(EXIT_FAILURE, "Invalid arguments\n");
> >+
> >+	rte_rawdev_start(dev_id);
> >+
> >+	if (interactive) {
> >+		sleep(1);
> >+		prompt();
> >+	}
> >+
> >+	return 0;
> >+}
> >--
> >2.17.1
> >