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...@intel.com>
> Cc: Wu, Jingjing <jingjing...@intel.com>; Wiles, Keith
> <keith.wi...@intel.com>;
> Liang, Cunming <cunming.li...@intel.com>; Maslekar, Omkar
> <omkar.masle...@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...@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
> >
