[4/6] examples/ntb: enable an example for ntb

  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
  

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
>
  

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
> >
  

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>

[snip]

>---
>+
>+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) &&
>+		    (rte_rawdevs[i].attached == 1))
>+			break;

Can replace above code block with rawdev API rte_rawdev_get_dev_id.

Thanks,
Xiaolong

>+
>+	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
>
  

Hi

> -----Original Message-----
> From: Ye, Xiaolong
> Sent: Tuesday, June 4, 2019 16:49
> 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
> > [snip]
> 
> >---
> >+
> >+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) &&
> >+		    (rte_rawdevs[i].attached == 1))
> >+			break;
> 
> Can replace above code block with rawdev API rte_rawdev_get_dev_id.
> 
In fact, I've considered this API. But this API uses name (device name) not driver name to identify device and it uses strcmp not strncmp.
But I want the first device who driver is ''raw_ntb". The device name is set to "NTB:[bus]:[dev].[func]" which is specific to the device.

> Thanks,
> Xiaolong
> 
> >+
> >+	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
> >
  

Hi
> > 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);
> >

Double pointer does not represent 2D arrays. Please refer to
https://stackoverflow.com/questions/4470950/why-cant-we-use-double-pointer-to-represent-two-dimensional-arrays

It will cause segment fault in enqueue/dequeue.

I will free twice.

> > >2.17.1
> > >
  

On 06/05, Li, Xiaoyun wrote:
>Hi
>> > 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);
>> >
>
>Double pointer does not represent 2D arrays. Please refer to
>https://stackoverflow.com/questions/4470950/why-cant-we-use-double-pointer-to-represent-two-dimensional-arrays
>

Yes, I agree, but I don't think it is the case here.

>It will cause segment fault in enqueue/dequeue.

Hmm, have you debug it? Which line caused the segfault?

You can refer to test_rawdev_enqdeq function in skeleton_rawdev_test.c, what it
does is similar to my suggestion, and you just need one alloc/free.

Thanks,
Xiaolong

>
>I will free twice.
>
>> > >2.17.1
>> > >
  

> 
> Yes, I agree, but I don't think it is the case here.
> 
> >It will cause segment fault in enqueue/dequeue.
> 
> Hmm, have you debug it? Which line caused the segfault?
> 
> You can refer to test_rawdev_enqdeq function in skeleton_rawdev_test.c, what
> it does is similar to my suggestion, and you just need one alloc/free.
> 
Of course I debug on it. It will cause seg fault at rte_memcpy. Because buffers[0]->buf_addr is not right.
You can see the debug log.
(gdb) p test_pkts_send[0].buf_addr
$3 = (void *) 0x5555562784c0
(gdb) n
164             rte_rawdev_set_attr(dev_id, "spad14", val);
(gdb)
ntb_attr_set(): Set attribute (spad14) Value (0)
165             val = size;
(gdb)
166             rte_rawdev_set_attr(dev_id, "spad15", val);
(gdb)
ntb_attr_set(): Set attribute (spad15) Value (35)
175             rte_rawdev_enqueue_buffers(dev_id, (struct rte_rawdev_buf **)&test_pkts_send, 1, (void *)size);
(gdb)

Thread 1 "ntb_fwd" hit Breakpoint 2, ntb_enqueue_bufs (dev=0x555556023b80 <rte_rawdevices>, buffers=0x7fffffff7fe0, count=1,
    context=0x23) at /home/xiaoyun/ntb/dpdk-next-net-intel/drivers/raw/ntb_rawdev/ntb_rawdev.c:244
244             struct ntb_hw *hw = dev->dev_private;
(gdb) n
248             if (hw->ntb_ops->get_peer_mw_addr == NULL)
(gdb)
249                     return -ENOTSUP;
(gdb) p buffers[0]->buf_addr
$4 = (void *) 0x6f77206f6c6c6568
(gdb) p &buffers[0]->buf_addr
$5 = (void **) 0x5555562784c0

> Thanks,
> Xiaolong
> 
> >
> >I will free twice.
> >
> >> > >2.17.1
> >> > >
  

On 06/05, Li, Xiaoyun wrote:
>> 
>> Yes, I agree, but I don't think it is the case here.
>> 
>> >It will cause segment fault in enqueue/dequeue.
>> 
>> Hmm, have you debug it? Which line caused the segfault?
>> 
>> You can refer to test_rawdev_enqdeq function in skeleton_rawdev_test.c, what
>> it does is similar to my suggestion, and you just need one alloc/free.
>> 
>Of course I debug on it. It will cause seg fault at rte_memcpy. Because buffers[0]->buf_addr is not right.
>You can see the debug log.
>(gdb) p test_pkts_send[0].buf_addr
>$3 = (void *) 0x5555562784c0
>(gdb) n
>164             rte_rawdev_set_attr(dev_id, "spad14", val);
>(gdb)
>ntb_attr_set(): Set attribute (spad14) Value (0)
>165             val = size;
>(gdb)
>166             rte_rawdev_set_attr(dev_id, "spad15", val);
>(gdb)
>ntb_attr_set(): Set attribute (spad15) Value (35)
>175             rte_rawdev_enqueue_buffers(dev_id, (struct rte_rawdev_buf **)&test_pkts_send, 1, (void *)size);
>(gdb)
>
>Thread 1 "ntb_fwd" hit Breakpoint 2, ntb_enqueue_bufs (dev=0x555556023b80 <rte_rawdevices>, buffers=0x7fffffff7fe0, count=1,
>    context=0x23) at /home/xiaoyun/ntb/dpdk-next-net-intel/drivers/raw/ntb_rawdev/ntb_rawdev.c:244
>244             struct ntb_hw *hw = dev->dev_private;
>(gdb) n
>248             if (hw->ntb_ops->get_peer_mw_addr == NULL)
>(gdb)
>249                     return -ENOTSUP;
>(gdb) p buffers[0]->buf_addr
>$4 = (void *) 0x6f77206f6c6c6568
>(gdb) p &buffers[0]->buf_addr
>$5 = (void **) 0x5555562784c0

Hmm, you're right, after a second thought, we do need double alloc/free in this
case, my bad.

Thanks,
Xiaolong

>
>> Thanks,
>> Xiaolong
>> 
>> >
>> >I will free twice.
>> >
>> >> > >2.17.1
>> >> > >