Re: [PATCH v3 10/10] ntb_perf: clear link_is_up flag when the link goes down.

Re: [PATCH v3 10/10] ntb_perf: clear link_is_up flag when the link goes down.

[Jiang, Dave]

On Wed, 2016-06-15 at 15:26 -0600, Logan Gunthorpe wrote:
> When the link goes down, the link_is_up flag did not return to
> false. This could have caused some subtle corner case bugs
> when the link goes up and down quickly.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

Acked-by: Dave Jiang <dave.jiang@intel.com>

And all the other ntb_perf patches since there were no additional
changes. 

> ---
>  drivers/ntb/test/ntb_perf.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/ntb/test/ntb_perf.c
> b/drivers/ntb/test/ntb_perf.c
> index f0784e5..ae9d1b2 100644
> --- a/drivers/ntb/test/ntb_perf.c
> +++ b/drivers/ntb/test/ntb_perf.c
> @@ -557,6 +557,8 @@ static void perf_link_cleanup(struct work_struct
> *work)
>  
>  	if (!perf->link_is_up)
>  		cancel_delayed_work_sync(&perf->link_work);
> +
> +	perf->link_is_up = false;
>  }
>  
>  static int perf_setup_mw(struct ntb_dev *ntb, struct perf_ctx *perf)
> -- 
> 2.1.4
> 

RE: [PATCH v3 08/10] ntb_pingpong: Add a debugfs file to get the ping count

[Allen Hubbe]

From: Logan Gunthorpe
> This commit adds a debugfs 'count' file to ntb_pingpong. This is so
> testing with ntb_pingpong can be automated beyond just checking the
> logs for pong messages.
> 
> The count file returns a number which increments every pong. The
> counter can be cleared by writing a zero.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

Acked-by: Allen Hubbe <Allen.Hubbe@emc.com>

> ---
>  drivers/ntb/test/ntb_pingpong.c | 62 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 61 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ntb/test/ntb_pingpong.c b/drivers/ntb/test/ntb_pingpong.c
> index fe16005..7d31179 100644
> --- a/drivers/ntb/test/ntb_pingpong.c
> +++ b/drivers/ntb/test/ntb_pingpong.c
> @@ -61,6 +61,7 @@
>  #include <linux/pci.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> +#include <linux/debugfs.h>
> 
>  #include <linux/ntb.h>
> 
> @@ -96,8 +97,13 @@ struct pp_ctx {
>  	spinlock_t			db_lock;
>  	struct timer_list		db_timer;
>  	unsigned long			db_delay;
> +	struct dentry			*debugfs_node_dir;
> +	struct dentry			*debugfs_count;
> +	atomic_t			count;
>  };
> 
> +static struct dentry *pp_debugfs_dir;
> +
>  static void pp_ping(unsigned long ctx)
>  {
>  	struct pp_ctx *pp = (void *)ctx;
> @@ -171,10 +177,32 @@ static void pp_db_event(void *ctx, int vec)
>  		dev_dbg(&pp->ntb->dev,
>  			"Pong vec %d bits %#llx\n",
>  			vec, db_bits);
> +		atomic_inc(&pp->count);
>  	}
>  	spin_unlock_irqrestore(&pp->db_lock, irqflags);
>  }
> 
> +static int pp_debugfs_setup(struct pp_ctx *pp)
> +{
> +	struct pci_dev *pdev = pp->ntb->pdev;
> +
> +	if (!pp_debugfs_dir)
> +		return -ENODEV;
> +
> +	pp->debugfs_node_dir = debugfs_create_dir(pci_name(pdev),
> +						  pp_debugfs_dir);
> +	if (!pp->debugfs_node_dir)
> +		return -ENODEV;
> +
> +	pp->debugfs_count = debugfs_create_atomic_t("count", S_IRUSR | S_IWUSR,
> +						    pp->debugfs_node_dir,
> +						    &pp->count);
> +	if (!pp->debugfs_count)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
>  static const struct ntb_ctx_ops pp_ops = {
>  	.link_event = pp_link_event,
>  	.db_event = pp_db_event,
> @@ -210,6 +238,7 @@ static int pp_probe(struct ntb_client *client,
> 
>  	pp->ntb = ntb;
>  	pp->db_bits = 0;
> +	atomic_set(&pp->count, 0);
>  	spin_lock_init(&pp->db_lock);
>  	setup_timer(&pp->db_timer, pp_ping, (unsigned long)pp);
>  	pp->db_delay = msecs_to_jiffies(delay_ms);
> @@ -218,6 +247,10 @@ static int pp_probe(struct ntb_client *client,
>  	if (rc)
>  		goto err_ctx;
> 
> +	rc = pp_debugfs_setup(pp);
> +	if (rc)
> +		goto err_ctx;
> +
>  	ntb_link_enable(ntb, NTB_SPEED_AUTO, NTB_WIDTH_AUTO);
>  	ntb_link_event(ntb);
> 
> @@ -234,6 +267,8 @@ static void pp_remove(struct ntb_client *client,
>  {
>  	struct pp_ctx *pp = ntb->ctx;
> 
> +	debugfs_remove_recursive(pp->debugfs_node_dir);
> +
>  	ntb_clear_ctx(ntb);
>  	del_timer_sync(&pp->db_timer);
>  	ntb_link_disable(ntb);
> @@ -247,4 +282,29 @@ static struct ntb_client pp_client = {
>  		.remove = pp_remove,
>  	},
>  };
> -module_ntb_client(pp_client);
> +
> +static int __init pp_init(void)
> +{
> +	int rc;
> +
> +	if (debugfs_initialized())
> +		pp_debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
> +
> +	rc = ntb_register_client(&pp_client);
> +	if (rc)
> +		goto err_client;
> +
> +	return 0;
> +
> +err_client:
> +	debugfs_remove_recursive(pp_debugfs_dir);
> +	return rc;
> +}
> +module_init(pp_init);
> +
> +static void __exit pp_exit(void)
> +{
> +	ntb_unregister_client(&pp_client);
> +	debugfs_remove_recursive(pp_debugfs_dir);
> +}
> +module_exit(pp_exit);
> --
> 2.1.4

RE: [PATCH v3 07/10] ntb_tool: Add link status and files to debugfs

[Allen Hubbe]

From: Logan Gunthorpe
> In order to more successfully script with ntb_tool it's useful to
> have a link file to check the link status so that the script
> doesn't use the other files until the link is up.
> 
> This commit adds a 'link' file to the debugfs directory which reads
> boolean (Y or N) depending on the link status. Writing to the file
> change the link state using ntb_link_enable or ntb_link_disable.
> 
> A 'link_event' file is also provided so an application can block until
> the link changes to the desired state. If the user writes a 1, it will
> block until the link is up. If the user writes a 0, it will block until
> the link is down.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

Acked-by: Allen Hubbe <Allen.Hubbe@emc.com>

> ---
>  drivers/ntb/test/ntb_tool.c | 89 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
> 
> diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
> index 031723d..caef74c 100644
> --- a/drivers/ntb/test/ntb_tool.c
> +++ b/drivers/ntb/test/ntb_tool.c
> @@ -59,6 +59,12 @@
>   *
>   * Eg: check if clearing the doorbell mask generates an interrupt.
>   *
> + * # Check the link status
> + * root@self# cat $DBG_DIR/link
> + *
> + * # Block until the link is up
> + * root@self# echo Y > $DBG_DIR/link_event
> + *
>   * # Set the doorbell mask
>   * root@self# echo 's 1' > $DBG_DIR/mask
>   *
> @@ -131,6 +137,7 @@ struct tool_mw {
>  struct tool_ctx {
>  	struct ntb_dev *ntb;
>  	struct dentry *dbgfs;
> +	wait_queue_head_t link_wq;
>  	int mw_count;
>  	struct tool_mw mws[MAX_MWS];
>  };
> @@ -159,6 +166,7 @@ static void tool_link_event(void *ctx)
>  	dev_dbg(&tc->ntb->dev, "link is %s speed %d width %d\n",
>  		up ? "up" : "down", speed, width);
> 
> +	wake_up(&tc->link_wq);
>  }
> 
>  static void tool_db_event(void *ctx, int vec)
> @@ -473,6 +481,80 @@ static TOOL_FOPS_RDWR(tool_peer_spad_fops,
>  		      tool_peer_spad_read,
>  		      tool_peer_spad_write);
> 
> +static ssize_t tool_link_read(struct file *filep, char __user *ubuf,
> +			      size_t size, loff_t *offp)
> +{
> +	struct tool_ctx *tc = filep->private_data;
> +	char buf[3];
> +
> +	buf[0] = ntb_link_is_up(tc->ntb, NULL, NULL) ? 'Y' : 'N';
> +	buf[1] = '\n';
> +	buf[2] = '\0';
> +
> +	return simple_read_from_buffer(ubuf, size, offp, buf, 2);
> +}
> +
> +static ssize_t tool_link_write(struct file *filep, const char __user *ubuf,
> +			       size_t size, loff_t *offp)
> +{
> +	struct tool_ctx *tc = filep->private_data;
> +	char buf[32];
> +	size_t buf_size;
> +	bool val;
> +	int rc;
> +
> +	buf_size = min(size, (sizeof(buf) - 1));
> +	if (copy_from_user(buf, ubuf, buf_size))
> +		return -EFAULT;
> +
> +	buf[buf_size] = '\0';
> +
> +	rc = strtobool(buf, &val);
> +	if (rc)
> +		return rc;
> +
> +	if (val)
> +		ntb_link_enable(tc->ntb, NTB_SPEED_AUTO, NTB_WIDTH_AUTO);
> +	else
> +		ntb_link_disable(tc->ntb);
> +
> +	return size;
> +}
> +
> +static TOOL_FOPS_RDWR(tool_link_fops,
> +		      tool_link_read,
> +		      tool_link_write);
> +
> +static ssize_t tool_link_event_write(struct file *filep,
> +				     const char __user *ubuf,
> +				     size_t size, loff_t *offp)
> +{
> +	struct tool_ctx *tc = filep->private_data;
> +	char buf[32];
> +	size_t buf_size;
> +	bool val;
> +	int rc;
> +
> +	buf_size = min(size, (sizeof(buf) - 1));
> +	if (copy_from_user(buf, ubuf, buf_size))
> +		return -EFAULT;
> +
> +	buf[buf_size] = '\0';
> +
> +	rc = strtobool(buf, &val);
> +	if (rc)
> +		return rc;
> +
> +	if (wait_event_interruptible(tc->link_wq,
> +		ntb_link_is_up(tc->ntb, NULL, NULL) == val))
> +		return -ERESTART;
> +
> +	return size;
> +}
> +
> +static TOOL_FOPS_RDWR(tool_link_event_fops,
> +		      NULL,
> +		      tool_link_event_write);
> 
>  static ssize_t tool_mw_read(struct file *filep, char __user *ubuf,
>  			    size_t size, loff_t *offp)
> @@ -793,6 +875,12 @@ static void tool_setup_dbgfs(struct tool_ctx *tc)
>  	debugfs_create_file("peer_spad", S_IRUSR | S_IWUSR, tc->dbgfs,
>  			    tc, &tool_peer_spad_fops);
> 
> +	debugfs_create_file("link", S_IRUSR | S_IWUSR, tc->dbgfs,
> +			    tc, &tool_link_fops);
> +
> +	debugfs_create_file("link_event", S_IWUSR, tc->dbgfs,
> +			    tc, &tool_link_event_fops);
> +
>  	for (i = 0; i < tc->mw_count; i++) {
>  		char buf[30];
> 
> @@ -825,6 +913,7 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
>  	}
> 
>  	tc->ntb = ntb;
> +	init_waitqueue_head(&tc->link_wq);
> 
>  	tc->mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
>  	for (i = 0; i < tc->mw_count; i++) {
> --
> 2.1.4

RE: [PATCH v3 09/10] ntb_test: Add a selftest script for the NTB subsystem

[Allen Hubbe]

From: Logan Gunthorpe
> This script automates testing doorbells, scratchpads and memory windows
> for an NTB device. It can be run locally, with the NTB looped
> back to the same host or use SSH to remotely control the second host.
> 
> In the single host case, the script just needs to be passed two
> arguments: a PCI ID for each side of the link. In the two host case
> the -r option must be used to specify the remote hostname (which must
> be SSH accessible and should probably have ssh-keys exchanged).
> 
> A sample run looks like this:
> 
> $ sudo ./ntb_test.sh 0000:03:00.1 0000:83:00.1 -p 29
> Starting ntb_tool tests...
> Running link tests on: 0000:03:00.1 / 0000:83:00.1
>   Passed
> Running link tests on: 0000:83:00.1 / 0000:03:00.1
>   Passed
> Running db tests on: 0000:03:00.1 / 0000:83:00.1
>   Passed
> Running db tests on: 0000:83:00.1 / 0000:03:00.1
>   Passed
> Running spad tests on: 0000:03:00.1 / 0000:83:00.1
>   Passed
> Running spad tests on: 0000:83:00.1 / 0000:03:00.1
>   Passed
> Running mw0 tests on: 0000:03:00.1 / 0000:83:00.1
>   Passed
> Running mw0 tests on: 0000:83:00.1 / 0000:03:00.1
>   Passed
> Running mw1 tests on: 0000:03:00.1 / 0000:83:00.1
>   Passed
> Running mw1 tests on: 0000:83:00.1 / 0000:03:00.1
>   Passed
> 
> Starting ntb_pingpong tests...
> Running ping pong tests on: 0000:03:00.1 / 0000:83:00.1
>   Passed
> 
> Starting ntb_perf tests...
> Running local perf test without DMA
>   0: copied 536870912 bytes in 164453 usecs, 3264 MBytes/s
>   Passed
> Running remote perf test without DMA
>   0: copied 536870912 bytes in 164453 usecs, 3264 MBytes/s
>   Passed
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Acked-by: Shuah Khan <shuahkh@osg.samsung.com>

Acked-by: Allen Hubbe <Allen.Hubbe@emc.com>

note one comment below, link_test

> ---
>  MAINTAINERS                             |   1 +
>  tools/testing/selftests/ntb/ntb_test.sh | 417 ++++++++++++++++++++++++++++++++
>  2 files changed, 418 insertions(+)
>  create mode 100755 tools/testing/selftests/ntb/ntb_test.sh
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9c567a4..f178e7e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7846,6 +7846,7 @@ F:	drivers/ntb/
>  F:	drivers/net/ntb_netdev.c
>  F:	include/linux/ntb.h
>  F:	include/linux/ntb_transport.h
> +F:	tools/testing/selftests/ntb/
> 
>  NTB INTEL DRIVER
>  M:	Jon Mason <jdmason@kudzu.us>
> diff --git a/tools/testing/selftests/ntb/ntb_test.sh
> b/tools/testing/selftests/ntb/ntb_test.sh
> new file mode 100755
> index 0000000..2b7bf81
> --- /dev/null
> +++ b/tools/testing/selftests/ntb/ntb_test.sh
> @@ -0,0 +1,417 @@
> +#!/bin/bash
> +# Copyright (c) 2016 Microsemi. All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation; either version 2 of
> +# the License, or (at your option) any later version.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# Author: Logan Gunthorpe <logang@deltatee.com>
> +
> +REMOTE_HOST=
> +LIST_DEVS=FALSE
> +
> +DEBUGFS=${DEBUGFS-/sys/kernel/debug}
> +
> +PERF_RUN_ORDER=32
> +MAX_MW_SIZE=0
> +RUN_DMA_TESTS=
> +DONT_CLEANUP=
> +MW_SIZE=65536
> +
> +function show_help()
> +{
> +	echo "Usage: $0 [OPTIONS] LOCAL_DEV REMOTE_DEV"
> +	echo "Run tests on a pair of NTB endpoints."
> +	echo
> +	echo "If the NTB device loops back to the same host then,"
> +	echo "just specifying the two PCI ids on the command line is"
> +	echo "sufficient. Otherwise, if the NTB link spans two hosts"
> +	echo "use the -r option to specify the hostname for the remote"
> +	echo "device. SSH will then be used to test the remote side."
> +	echo "An SSH key between the root users of the host would then"
> +	echo "be highly recommended."
> +	echo
> +	echo "Options:"
> +	echo "  -C              don't cleanup ntb modules on exit"
> +	echo "  -d              run dma tests"
> +	echo "  -h              show this help message"
> +	echo "  -l              list available local and remote PCI ids"
> +	echo "  -r REMOTE_HOST  specify the remote's hostname to connect"
> +        echo "                  to for the test (using ssh)"
> +	echo "  -p NUM          ntb_perf run order (default: $PERF_RUN_ORDER)"
> +	echo "  -w max_mw_size  maxmium memory window size"
> +	echo
> +}
> +
> +function parse_args()
> +{
> +	OPTIND=0
> +	while getopts "Cdhlm:r:p:w:" opt; do
> +		case "$opt" in
> +		C)  DONT_CLEANUP=1 ;;
> +		d)  RUN_DMA_TESTS=1 ;;
> +		h)  show_help; exit 0 ;;
> +		l)  LIST_DEVS=TRUE ;;
> +		m)  MW_SIZE=${OPTARG} ;;
> +		r)  REMOTE_HOST=${OPTARG} ;;
> +		p)  PERF_RUN_ORDER=${OPTARG} ;;
> +		w)  MAX_MW_SIZE=${OPTARG} ;;
> +		\?)
> +		    echo "Invalid option: -$OPTARG" >&2
> +		    exit 1
> +		    ;;
> +		esac
> +	done
> +}
> +
> +parse_args "$@"
> +shift $((OPTIND-1))
> +LOCAL_DEV=$1
> +shift
> +parse_args "$@"
> +shift $((OPTIND-1))
> +REMOTE_DEV=$1
> +shift
> +parse_args "$@"
> +
> +set -e
> +
> +function _modprobe()
> +{
> +        modprobe "$@"
> +}
> +
> +function split_remote()
> +{
> +	VPATH=$1
> +	REMOTE=
> +
> +	if [[ "$VPATH" == *":/"* ]]; then
> +		REMOTE=${VPATH%%:*}
> +		VPATH=${VPATH#*:}
> +	fi
> +}
> +
> +function read_file()
> +{
> +	split_remote $1
> +	if [[ "$REMOTE" != "" ]]; then
> +		ssh "$REMOTE" cat "$VPATH"
> +	else
> +		cat "$VPATH"
> +	fi
> +}
> +
> +function write_file()
> +{
> +	split_remote $2
> +	VALUE=$1
> +
> +	if [[ "$REMOTE" != "" ]]; then
> +		ssh "$REMOTE" "echo \"$VALUE\" > \"$VPATH\""
> +	else
> +		echo "$VALUE" > "$VPATH"
> +	fi
> +}
> +
> +function link_test()
> +{
> +	LOC=$1
> +	REM=$2
> +	EXP=0
> +
> +	echo "Running link tests on: $(basename $LOC) / $(basename $REM)"
> +
> +	write_file "N" "$LOC/link"
> +	write_file "N" "$LOC/link_event"

If it fails to bring down the link, won't it just block waiting on link_event and never make it to the next step of the test?

> +	if [[ $(read_file "$REM/link") != "N" ]]; then
> +		echo "Expected remote link to be down in $REM/link" >&2
> +		exit -1
> +	fi
> +
> +	write_file "Y" "$LOC/link"
> +	write_file "Y" "$LOC/link_event"
> +
> +	echo "  Passed"
> +}
> +
> +function doorbell_test()
> +{
> +	LOC=$1
> +	REM=$2
> +	EXP=0
> +
> +	echo "Running db tests on: $(basename $LOC) / $(basename $REM)"
> +
> +	write_file "c 0xFFFFFFFF" "$REM/db"
> +
> +	for ((i=1; i <= 8; i++)); do
> +		let DB=$(read_file "$REM/db") || true
> +		if [[ "$DB" != "$EXP" ]]; then
> +			echo "Doorbell doesn't match expected value $EXP " \
> +			     "in $REM/db" >&2
> +			exit -1
> +		fi
> +
> +		let "MASK=1 << ($i-1)" || true
> +		let "EXP=$EXP | $MASK" || true
> +		write_file "s $MASK" "$LOC/peer_db"
> +	done
> +
> +	echo "  Passed"
> +}
> +
> +function read_spad()
> +{
> +       VPATH=$1
> +       IDX=$2
> +
> +       ROW=($(read_file "$VPATH" | grep -e "^$IDX"))
> +       let VAL=${ROW[1]} || true
> +       echo $VAL
> +}
> +
> +function scratchpad_test()
> +{
> +	LOC=$1
> +	REM=$2
> +	CNT=$(read_file "$LOC/spad" | wc -l)
> +
> +	echo "Running spad tests on: $(basename $LOC) / $(basename $REM)"
> +
> +	for ((i = 0; i < $CNT; i++)); do
> +		VAL=$RANDOM
> +		write_file "$i $VAL" "$LOC/peer_spad"
> +		RVAL=$(read_spad "$REM/spad" $i)
> +
> +		if [[ "$VAL" != "$RVAL" ]]; then
> +			echo "Scratchpad doesn't match expected value $VAL " \
> +			     "in $REM/spad, got $RVAL" >&2
> +			exit -1
> +		fi
> +
> +	done
> +
> +	echo "  Passed"
> +}
> +
> +function write_mw()
> +{
> +	split_remote $2
> +
> +	if [[ "$REMOTE" != "" ]]; then
> +		ssh "$REMOTE" \
> +			dd if=/dev/urandom "of=$VPATH" 2> /dev/null || true
> +	else
> +		dd if=/dev/urandom "of=$VPATH" 2> /dev/null || true
> +	fi
> +}
> +
> +function mw_test()
> +{
> +	IDX=$1
> +	LOC=$2
> +	REM=$3
> +
> +	echo "Running $IDX tests on: $(basename $LOC) / $(basename $REM)"
> +
> +	write_mw "$LOC/$IDX"
> +
> +	split_remote "$LOC/$IDX"
> +	if [[ "$REMOTE" == "" ]]; then
> +		A=$VPATH
> +	else
> +		A=/tmp/ntb_test.$$.A
> +		ssh "$REMOTE" cat "$VPATH" > "$A"
> +	fi
> +
> +	split_remote "$REM/peer_$IDX"
> +	if [[ "$REMOTE" == "" ]]; then
> +		B=$VPATH
> +	else
> +		B=/tmp/ntb_test.$$.B
> +		ssh "$REMOTE" cat "$VPATH" > "$B"
> +	fi
> +
> +	cmp -n $MW_SIZE "$A" "$B"
> +	if [[ $? != 0 ]]; then
> +		echo "Memory window $MW did not match!" >&2
> +	fi
> +
> +	if [[ "$A" == "/tmp/*" ]]; then
> +		rm "$A"
> +	fi
> +
> +	if [[ "$B" == "/tmp/*" ]]; then
> +		rm "$B"
> +	fi
> +
> +	echo "  Passed"
> +}
> +
> +function pingpong_test()
> +{
> +	LOC=$1
> +	REM=$2
> +
> +	echo "Running ping pong tests on: $(basename $LOC) / $(basename $REM)"
> +
> +	LOC_START=$(read_file $LOC/count)
> +	REM_START=$(read_file $REM/count)
> +
> +	sleep 7
> +
> +	LOC_END=$(read_file $LOC/count)
> +	REM_END=$(read_file $REM/count)
> +
> +	if [[ $LOC_START == $LOC_END ]] || [[ $REM_START == $REM_END ]]; then
> +		echo "Ping pong counter not incrementing!" >&2
> +		exit 1
> +	fi
> +
> +	echo "  Passed"
> +}
> +
> +function perf_test()
> +{
> +	USE_DMA=$1
> +
> +	if [[ $USE_DMA == "1" ]]; then
> +		WITH="with"
> +	else
> +		WITH="without"
> +	fi
> +
> +	_modprobe ntb_perf run_order=$PERF_RUN_ORDER \
> +		max_mw_size=$MAX_MW_SIZE use_dma=$USE_DMA
> +
> +	echo "Running local perf test $WITH DMA"
> +	write_file "" $LOCAL_PERF/run
> +	echo -n "  "
> +	read_file $LOCAL_PERF/run
> +	echo "  Passed"
> +
> +	echo "Running remote perf test $WITH DMA"
> +	write_file "" $REMOTE_PERF/run
> +	echo -n "  "
> +	read_file $LOCAL_PERF/run
> +	echo "  Passed"
> +
> +	_modprobe -r ntb_perf
> +}
> +
> +function ntb_tool_tests()
> +{
> +	LOCAL_TOOL=$DEBUGFS/ntb_tool/$LOCAL_DEV
> +	REMOTE_TOOL=$REMOTE_HOST:$DEBUGFS/ntb_tool/$REMOTE_DEV
> +
> +	echo "Starting ntb_tool tests..."
> +
> +	_modprobe ntb_tool
> +
> +	write_file Y $LOCAL_TOOL/link_event
> +	write_file Y $REMOTE_TOOL/link_event
> +
> +	link_test $LOCAL_TOOL $REMOTE_TOOL
> +	link_test $REMOTE_TOOL $LOCAL_TOOL
> +
> +	for PEER_TRANS in $(ls $LOCAL_TOOL/peer_trans*); do
> +		PT=$(basename $PEER_TRANS)
> +		write_file $MW_SIZE $LOCAL_TOOL/$PT
> +		write_file $MW_SIZE $REMOTE_TOOL/$PT
> +	done
> +
> +	doorbell_test $LOCAL_TOOL $REMOTE_TOOL
> +	doorbell_test $REMOTE_TOOL $LOCAL_TOOL
> +	scratchpad_test $LOCAL_TOOL $REMOTE_TOOL
> +	scratchpad_test $REMOTE_TOOL $LOCAL_TOOL
> +
> +	for MW in $(ls $LOCAL_TOOL/mw*); do
> +		MW=$(basename $MW)
> +
> +		mw_test $MW $LOCAL_TOOL $REMOTE_TOOL
> +		mw_test $MW $REMOTE_TOOL $LOCAL_TOOL
> +	done
> +
> +	_modprobe -r ntb_tool
> +}
> +
> +function ntb_pingpong_tests()
> +{
> +	LOCAL_PP=$DEBUGFS/ntb_pingpong/$LOCAL_DEV
> +	REMOTE_PP=$REMOTE_HOST:$DEBUGFS/ntb_pingpong/$REMOTE_DEV
> +
> +	echo "Starting ntb_pingpong tests..."
> +
> +	_modprobe ntb_pingpong
> +
> +	pingpong_test $LOCAL_PP $REMOTE_PP
> +
> +	_modprobe -r ntb_pingpong
> +}
> +
> +function ntb_perf_tests()
> +{
> +	LOCAL_PERF=$DEBUGFS/ntb_perf/$LOCAL_DEV
> +	REMOTE_PERF=$REMOTE_HOST:$DEBUGFS/ntb_perf/$REMOTE_DEV
> +
> +	echo "Starting ntb_perf tests..."
> +
> +	perf_test 0
> +
> +	if [[ $RUN_DMA_TESTS ]]; then
> +		perf_test 1
> +	fi
> +}
> +
> +function cleanup()
> +{
> +	set +e
> +	_modprobe -r ntb_tool 2> /dev/null
> +	_modprobe -r ntb_perf 2> /dev/null
> +	_modprobe -r ntb_pingpong 2> /dev/null
> +	_modprobe -r ntb_transport 2> /dev/null
> +	set -e
> +}
> +
> +cleanup
> +
> +if ! [[ $$DONT_CLEANUP ]]; then
> +	trap cleanup EXIT
> +fi
> +
> +if [ "$(id -u)" != "0" ]; then
> +	echo "This script must be run as root" 1>&2
> +	exit 1
> +fi
> +
> +if [[ "$LIST_DEVS" == TRUE ]]; then
> +	echo "Local Devices:"
> +	ls -1 /sys/bus/ntb/devices
> +	echo
> +
> +	if [[ "$REMOTE_HOST" != "" ]]; then
> +		echo "Remote Devices:"
> +		ssh $REMOTE_HOST ls -1 /sys/bus/ntb/devices
> +	fi
> +
> +	exit 0
> +fi
> +
> +if [[ "$LOCAL_DEV" == $"" ]] || [[ "$REMOTE_DEV" == $"" ]]; then
> +	show_help
> +	exit 1
> +fi
> +
> +ntb_tool_tests
> +echo
> +ntb_pingpong_tests
> +echo
> +ntb_perf_tests
> +echo
> --
> 2.1.4

RE: [PATCH v3 06/10] ntb_tool: Postpone memory window initialization for the user

[Allen Hubbe]

From: Logan Gunthorpe
> In order to make the interface closer to the raw NTB API, this commit
> changes memory windows so they are not initialized on link up.
> Instead, the 'peer_trans*' debugfs files are introduced. When read,
> they return information provided by ntb_mw_get_range. When written,
> they create a buffer and initialize the memory window. The
> value written is taken as the requested size of the buffer (which
> is then rounded for alignment). Writing a value of zero frees the buffer
> and tears down the memory window translation. The 'peer_mw*' file is
> only created once the memory window translation is setup by the user.
> 
> Additionally, it was noticed that the read and write functions for the
> 'peer_mw*' files should have checked for a NULL pointer.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

Acked-by: Allen Hubbe <Allen.Hubbe@emc.com>

I am happy to see all the link_work complexity go away with this patch.

> ---
>  drivers/ntb/test/ntb_tool.c | 356 +++++++++++++++++++++++++++-----------------
>  1 file changed, 218 insertions(+), 138 deletions(-)
> 
> diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
> index cba31fd..031723d 100644
> --- a/drivers/ntb/test/ntb_tool.c
> +++ b/drivers/ntb/test/ntb_tool.c
> @@ -79,6 +79,13 @@
>   * root@self# cat $DBG_DIR/spad
>   *
>   * Observe that spad 0 and 1 have the values set by the peer.
> + *
> + * # Check the memory window translation info
> + * cat $DBG_DIR/peer_trans0
> + *
> + * # Setup a 16k memory window buffer
> + * echo 16384 > $DBG_DIR/peer_trans0
> + *
>   */
> 
>  #include <linux/init.h>
> @@ -108,25 +115,22 @@ MODULE_DESCRIPTION(DRIVER_DESCRIPTION);
> 
>  #define MAX_MWS 16
> 
> -static unsigned long mw_size = 16;
> -module_param(mw_size, ulong, 0644);
> -MODULE_PARM_DESC(mw_size, "size order [n^2] of the memory window for testing");
> -
>  static struct dentry *tool_dbgfs;
> 
>  struct tool_mw {
> +	int idx;
> +	struct tool_ctx *tc;
> +	resource_size_t win_size;
>  	resource_size_t size;
>  	u8 __iomem *local;
>  	u8 *peer;
>  	dma_addr_t peer_dma;
> +	struct dentry *peer_dbg_file;
>  };
> 
>  struct tool_ctx {
>  	struct ntb_dev *ntb;
>  	struct dentry *dbgfs;
> -	struct work_struct link_cleanup;
> -	bool link_is_up;
> -	struct delayed_work link_work;
>  	int mw_count;
>  	struct tool_mw mws[MAX_MWS];
>  };
> @@ -143,111 +147,6 @@ struct tool_ctx {
>  		.write = __write,		\
>  	}
> 
> -static int tool_setup_mw(struct tool_ctx *tc, int idx)
> -{
> -	int rc;
> -	struct tool_mw *mw = &tc->mws[idx];
> -	phys_addr_t base;
> -	resource_size_t size, align, align_size;
> -
> -	if (mw->local)
> -		return 0;
> -
> -	rc = ntb_mw_get_range(tc->ntb, idx, &base, &size, &align,
> -			      &align_size);
> -	if (rc)
> -		return rc;
> -
> -	mw->size = min_t(resource_size_t, 1 << mw_size, size);
> -	mw->size = round_up(mw->size, align);
> -	mw->size = round_up(mw->size, align_size);
> -
> -	mw->local = ioremap_wc(base, size);
> -	if (mw->local == NULL)
> -		return -EFAULT;
> -
> -	mw->peer = dma_alloc_coherent(&tc->ntb->pdev->dev, mw->size,
> -				      &mw->peer_dma, GFP_KERNEL);
> -
> -	if (mw->peer == NULL)
> -		return -ENOMEM;
> -
> -	rc = ntb_mw_set_trans(tc->ntb, idx, mw->peer_dma, mw->size);
> -	if (rc)
> -		return rc;
> -
> -	return 0;
> -}
> -
> -static void tool_free_mws(struct tool_ctx *tc)
> -{
> -	int i;
> -
> -	for (i = 0; i < tc->mw_count; i++) {
> -		if (tc->mws[i].peer) {
> -			ntb_mw_clear_trans(tc->ntb, i);
> -			dma_free_coherent(&tc->ntb->pdev->dev, tc->mws[i].size,
> -					  tc->mws[i].peer,
> -					  tc->mws[i].peer_dma);
> -
> -		}
> -
> -		tc->mws[i].peer = NULL;
> -		tc->mws[i].peer_dma = 0;
> -
> -		if (tc->mws[i].local)
> -			iounmap(tc->mws[i].local);
> -
> -		tc->mws[i].local = NULL;
> -	}
> -
> -	tc->mw_count = 0;
> -}
> -
> -static int tool_setup_mws(struct tool_ctx *tc)
> -{
> -	int i;
> -	int rc;
> -
> -	tc->mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
> -
> -	for (i = 0; i < tc->mw_count; i++) {
> -		rc = tool_setup_mw(tc, i);
> -		if (rc)
> -			goto err_out;
> -	}
> -
> -	return 0;
> -
> -err_out:
> -	tool_free_mws(tc);
> -	return rc;
> -}
> -
> -static void tool_link_work(struct work_struct *work)
> -{
> -	int rc;
> -	struct tool_ctx *tc = container_of(work, struct tool_ctx,
> -					   link_work.work);
> -
> -	tool_free_mws(tc);
> -	rc = tool_setup_mws(tc);
> -	if (rc)
> -		dev_err(&tc->ntb->dev,
> -			"Error setting up memory windows: %d\n", rc);
> -
> -	tc->link_is_up = true;
> -}
> -
> -static void tool_link_cleanup(struct work_struct *work)
> -{
> -	struct tool_ctx *tc = container_of(work, struct tool_ctx,
> -					   link_cleanup);
> -
> -	if (!tc->link_is_up)
> -		cancel_delayed_work_sync(&tc->link_work);
> -}
> -
>  static void tool_link_event(void *ctx)
>  {
>  	struct tool_ctx *tc = ctx;
> @@ -260,10 +159,6 @@ static void tool_link_event(void *ctx)
>  	dev_dbg(&tc->ntb->dev, "link is %s speed %d width %d\n",
>  		up ? "up" : "down", speed, width);
> 
> -	if (up)
> -		schedule_delayed_work(&tc->link_work, 2*HZ);
> -	else
> -		schedule_work(&tc->link_cleanup);
>  }
> 
>  static void tool_db_event(void *ctx, int vec)
> @@ -591,10 +486,10 @@ static ssize_t tool_mw_read(struct file *filep, char __user *ubuf,
>  		return -EIO;
>  	if (pos < 0)
>  		return -EINVAL;
> -	if (pos >= mw->size || !size)
> +	if (pos >= mw->win_size || !size)
>  		return 0;
> -	if (size > mw->size - pos)
> -		size = mw->size - pos;
> +	if (size > mw->win_size - pos)
> +		size = mw->win_size - pos;
> 
>  	buf = kmalloc(size, GFP_KERNEL);
>  	if (!buf)
> @@ -627,10 +522,10 @@ static ssize_t tool_mw_write(struct file *filep, const char __user
> *ubuf,
> 
>  	if (pos < 0)
>  		return -EINVAL;
> -	if (pos >= mw->size || !size)
> +	if (pos >= mw->win_size || !size)
>  		return 0;
> -	if (size > mw->size - pos)
> -		size = mw->size - pos;
> +	if (size > mw->win_size - pos)
> +		size = mw->win_size - pos;
> 
>  	buf = kmalloc(size, GFP_KERNEL);
>  	if (!buf)
> @@ -658,20 +553,25 @@ static TOOL_FOPS_RDWR(tool_mw_fops,
>  		      tool_mw_read,
>  		      tool_mw_write);
> 
> -
>  static ssize_t tool_peer_mw_read(struct file *filep, char __user *ubuf,
> -				   size_t size, loff_t *offp)
> +				 size_t size, loff_t *offp)
>  {
>  	struct tool_mw *mw = filep->private_data;
> 
> +	if (!mw->peer)
> +		return -ENXIO;
> +
>  	return simple_read_from_buffer(ubuf, size, offp, mw->peer, mw->size);
>  }
> 
>  static ssize_t tool_peer_mw_write(struct file *filep, const char __user *ubuf,
> -				    size_t size, loff_t *offp)
> +				  size_t size, loff_t *offp)
>  {
>  	struct tool_mw *mw = filep->private_data;
> 
> +	if (!mw->peer)
> +		return -ENXIO;
> +
>  	return simple_write_to_buffer(mw->peer, mw->size, offp, ubuf, size);
>  }
> 
> @@ -679,9 +579,189 @@ static TOOL_FOPS_RDWR(tool_peer_mw_fops,
>  		      tool_peer_mw_read,
>  		      tool_peer_mw_write);
> 
> +static int tool_setup_mw(struct tool_ctx *tc, int idx, size_t req_size)
> +{
> +	int rc;
> +	struct tool_mw *mw = &tc->mws[idx];
> +	phys_addr_t base;
> +	resource_size_t size, align, align_size;
> +	char buf[16];
> +
> +	if (mw->peer)
> +		return 0;
> +
> +	rc = ntb_mw_get_range(tc->ntb, idx, &base, &size, &align,
> +			      &align_size);
> +	if (rc)
> +		return rc;
> +
> +	mw->size = min_t(resource_size_t, req_size, size);
> +	mw->size = round_up(mw->size, align);
> +	mw->size = round_up(mw->size, align_size);
> +	mw->peer = dma_alloc_coherent(&tc->ntb->pdev->dev, mw->size,
> +				      &mw->peer_dma, GFP_KERNEL);
> +
> +	if (!mw->peer)
> +		return -ENOMEM;
> +
> +	rc = ntb_mw_set_trans(tc->ntb, idx, mw->peer_dma, mw->size);
> +	if (rc)
> +		return rc;
> +
> +	snprintf(buf, sizeof(buf), "peer_mw%d", idx);
> +	mw->peer_dbg_file = debugfs_create_file(buf, S_IRUSR | S_IWUSR,
> +						mw->tc->dbgfs, mw,
> +						&tool_peer_mw_fops);
> +
> +	return 0;
> +}
> +
> +static void tool_free_mw(struct tool_ctx *tc, int idx)
> +{
> +	struct tool_mw *mw = &tc->mws[idx];
> +
> +	if (mw->peer) {
> +		ntb_mw_clear_trans(tc->ntb, idx);
> +		dma_free_coherent(&tc->ntb->pdev->dev, mw->size,
> +				  mw->peer,
> +				  mw->peer_dma);
> +	}
> +
> +	mw->peer = NULL;
> +	mw->peer_dma = 0;
> +
> +	debugfs_remove(mw->peer_dbg_file);
> +
> +	mw->peer_dbg_file = NULL;
> +}
> +
> +static ssize_t tool_peer_mw_trans_read(struct file *filep,
> +				       char __user *ubuf,
> +				       size_t size, loff_t *offp)
> +{
> +	struct tool_mw *mw = filep->private_data;
> +
> +	char *buf;
> +	size_t buf_size;
> +	ssize_t ret, off = 0;
> +
> +	phys_addr_t base;
> +	resource_size_t mw_size;
> +	resource_size_t align;
> +	resource_size_t align_size;
> +
> +	buf_size = min_t(size_t, size, 512);
> +
> +	buf = kmalloc(buf_size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ntb_mw_get_range(mw->tc->ntb, mw->idx,
> +			 &base, &mw_size, &align, &align_size);
> +
> +	off += scnprintf(buf + off, buf_size - off,
> +			 "Peer MW %d Information:\n", mw->idx);
> +
> +	off += scnprintf(buf + off, buf_size - off,
> +			 "Physical Address      \t%pa[p]\n",
> +			 &base);
> +
> +	off += scnprintf(buf + off, buf_size - off,
> +			 "Window Size           \t%lld\n",
> +			 (unsigned long long)mw_size);
> +
> +	off += scnprintf(buf + off, buf_size - off,
> +			 "Alignment             \t%lld\n",
> +			 (unsigned long long)align);
> +
> +	off += scnprintf(buf + off, buf_size - off,
> +			 "Size Alignment        \t%lld\n",
> +			 (unsigned long long)align_size);
> +
> +	off += scnprintf(buf + off, buf_size - off,
> +			 "Ready                 \t%c\n",
> +			 (mw->peer) ? 'Y' : 'N');
> +
> +	off += scnprintf(buf + off, buf_size - off,
> +			 "Allocated Size       \t%zd\n",
> +			 (mw->peer) ? (size_t)mw->size : 0);
> +
> +	ret = simple_read_from_buffer(ubuf, size, offp, buf, off);
> +	kfree(buf);
> +	return ret;
> +}
> +
> +static ssize_t tool_peer_mw_trans_write(struct file *filep,
> +					const char __user *ubuf,
> +					size_t size, loff_t *offp)
> +{
> +	struct tool_mw *mw = filep->private_data;
> +
> +	char buf[32];
> +	size_t buf_size;
> +	unsigned long long val;
> +	int rc;
> +
> +	buf_size = min(size, (sizeof(buf) - 1));
> +	if (copy_from_user(buf, ubuf, buf_size))
> +		return -EFAULT;
> +
> +	buf[buf_size] = '\0';
> +
> +	rc = kstrtoull(buf, 0, &val);
> +	if (rc)
> +		return rc;
> +
> +	tool_free_mw(mw->tc, mw->idx);
> +	if (val)
> +		rc = tool_setup_mw(mw->tc, mw->idx, val);
> +
> +	if (rc)
> +		return rc;
> +
> +	return size;
> +}
> +
> +static TOOL_FOPS_RDWR(tool_peer_mw_trans_fops,
> +		      tool_peer_mw_trans_read,
> +		      tool_peer_mw_trans_write);
> +
> +static int tool_init_mw(struct tool_ctx *tc, int idx)
> +{
> +	struct tool_mw *mw = &tc->mws[idx];
> +	phys_addr_t base;
> +	int rc;
> +
> +	rc = ntb_mw_get_range(tc->ntb, idx, &base, &mw->win_size,
> +			      NULL, NULL);
> +	if (rc)
> +		return rc;
> +
> +	mw->tc = tc;
> +	mw->idx = idx;
> +	mw->local = ioremap_wc(base, mw->win_size);
> +	if (!mw->local)
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static void tool_free_mws(struct tool_ctx *tc)
> +{
> +	int i;
> +
> +	for (i = 0; i < tc->mw_count; i++) {
> +		tool_free_mw(tc, i);
> +
> +		if (tc->mws[i].local)
> +			iounmap(tc->mws[i].local);
> +
> +		tc->mws[i].local = NULL;
> +	}
> +}
> +
>  static void tool_setup_dbgfs(struct tool_ctx *tc)
>  {
> -	int mw_count;
>  	int i;
> 
>  	/* This modules is useless without dbgfs... */
> @@ -713,18 +793,16 @@ static void tool_setup_dbgfs(struct tool_ctx *tc)
>  	debugfs_create_file("peer_spad", S_IRUSR | S_IWUSR, tc->dbgfs,
>  			    tc, &tool_peer_spad_fops);
> 
> -	mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
> -	for (i = 0; i < mw_count; i++) {
> +	for (i = 0; i < tc->mw_count; i++) {
>  		char buf[30];
> 
>  		snprintf(buf, sizeof(buf), "mw%d", i);
>  		debugfs_create_file(buf, S_IRUSR | S_IWUSR, tc->dbgfs,
>  				    &tc->mws[i], &tool_mw_fops);
> 
> -		snprintf(buf, sizeof(buf), "peer_mw%d", i);
> +		snprintf(buf, sizeof(buf), "peer_trans%d", i);
>  		debugfs_create_file(buf, S_IRUSR | S_IWUSR, tc->dbgfs,
> -				    &tc->mws[i], &tool_peer_mw_fops);
> -
> +				    &tc->mws[i], &tool_peer_mw_trans_fops);
>  	}
>  }
> 
> @@ -732,6 +810,7 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
>  {
>  	struct tool_ctx *tc;
>  	int rc;
> +	int i;
> 
>  	if (ntb_db_is_unsafe(ntb))
>  		dev_dbg(&ntb->dev, "doorbell is unsafe\n");
> @@ -746,8 +825,13 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
>  	}
> 
>  	tc->ntb = ntb;
> -	INIT_DELAYED_WORK(&tc->link_work, tool_link_work);
> -	INIT_WORK(&tc->link_cleanup, tool_link_cleanup);
> +
> +	tc->mw_count = min(ntb_mw_count(tc->ntb), MAX_MWS);
> +	for (i = 0; i < tc->mw_count; i++) {
> +		rc = tool_init_mw(tc, i);
> +		if (rc)
> +			goto err_ctx;
> +	}
> 
>  	tool_setup_dbgfs(tc);
> 
> @@ -761,9 +845,8 @@ static int tool_probe(struct ntb_client *self, struct ntb_dev *ntb)
>  	return 0;
> 
>  err_ctx:
> +	tool_free_mws(tc);
>  	debugfs_remove_recursive(tc->dbgfs);
> -	cancel_delayed_work_sync(&tc->link_work);
> -	cancel_work_sync(&tc->link_cleanup);
>  	kfree(tc);
>  err_tc:
>  	return rc;
> @@ -773,9 +856,6 @@ static void tool_remove(struct ntb_client *self, struct ntb_dev *ntb)
>  {
>  	struct tool_ctx *tc = ntb->ctx;
> 
> -	cancel_delayed_work_sync(&tc->link_work);
> -	cancel_work_sync(&tc->link_cleanup);
> -
>  	tool_free_mws(tc);
> 
>  	ntb_clear_ctx(ntb);
> --
> 2.1.4

Re: [PATCH v3 09/10] ntb_test: Add a selftest script for the NTB subsystem

[Logan Gunthorpe]

On 15/06/16 03:49 PM, Allen Hubbe wrote:
>> +function link_test()
>> +{
>> +	LOC=$1
>> +	REM=$2
>> +	EXP=0
>> +
>> +	echo "Running link tests on: $(basename $LOC) / $(basename $REM)"
>> +
>> +	write_file "N" "$LOC/link"
>> +	write_file "N" "$LOC/link_event"
> 
> If it fails to bring down the link, won't it just block waiting on link_event and never make it to the next step of the test?
> 
>> +	if [[ $(read_file "$REM/link") != "N" ]]; then
>> +		echo "Expected remote link to be down in $REM/link" >&2
>> +		exit -1
>> +	fi
>> +
>> +	write_file "Y" "$LOC/link"
>> +	write_file "Y" "$LOC/link_event"
>> +
>> +	echo "  Passed"
>> +}

Well, the test is really intended to ensure both sides of the link see
changes to the link status. If the driver is somehow buggy and the link
never goes down/up when requested there's little I can do here except
block forever. Unless we want to add a timeout to the link_event file
(which I'd rather not).

You'd have the same issue if, when bringing the link up for the first
time, the link does not come back.

Logan

RE: [PATCH v3 09/10] ntb_test: Add a selftest script for the NTB subsystem

[Allen Hubbe]

From: Logan Gunthorpe
> On 15/06/16 03:49 PM, Allen Hubbe wrote:
> >> +function link_test()
> >> +{
> >> +	LOC=$1
> >> +	REM=$2
> >> +	EXP=0
> >> +
> >> +	echo "Running link tests on: $(basename $LOC) / $(basename $REM)"
> >> +
> >> +	write_file "N" "$LOC/link"
> >> +	write_file "N" "$LOC/link_event"
> >
> > If it fails to bring down the link, won't it just block waiting on link_event and never
> make it to the next step of the test?
> >
> >> +	if [[ $(read_file "$REM/link") != "N" ]]; then
> >> +		echo "Expected remote link to be down in $REM/link" >&2
> >> +		exit -1
> >> +	fi
> >> +
> >> +	write_file "Y" "$LOC/link"
> >> +	write_file "Y" "$LOC/link_event"
> >> +
> >> +	echo "  Passed"
> >> +}
> 
> Well, the test is really intended to ensure both sides of the link see
> changes to the link status. If the driver is somehow buggy and the link
> never goes down/up when requested there's little I can do here except
> block forever. Unless we want to add a timeout to the link_event file
> (which I'd rather not).
> 
> You'd have the same issue if, when bringing the link up for the first
> time, the link does not come back.

The link might come up, but this test checks if the link can be forced down.

This test should fail on Intel RP/TB topology (two cpu sharing one ntb).  The link state is the link state of the secondary side pcie bus connected to the secondary side cpu.  The link must be up in order for the secondary side cpu to discover the ntb device, so the driver does not allow the link to be disabled in such topology.

A simple thing to do here might be:

write_file "N" "$LOC/link"
sleep 1
read_file "$REM/link"

You already have my Ack.  This minor issue can be fixed later if anyone cares.  I don't think it is a big deal, just worth pointing out that the script will hang here instead of report a failure.  If it is worth fixing later, at that point we might also want to change this script to continue with other tests instead of exit on the first failure.

Re: [PATCH v3 10/10] ntb_perf: clear link_is_up flag when the link goes down.

[Logan Gunthorpe]

Hey,

Actually, I have to retract this patch. After some more thorough testing
I'm finding an issue:

When you remove and re-install the ntb_perf module very quickly,
ntb_perf will occasionally miss the link up event. This is because the
link_cleanup work gets delayed long enough that it gets scheduled after
the link up event gets sent. It then cancels the link work that should
have occurred. Without this patch, it never happens because link_is_up
never returns to false.

I think the correct solution is to just remove the link_cleanup work and
do those actions immediately on receipt of the event. If there's
agreement on this I can re-spin it again.

Thanks,

Logan


On 15/06/16 03:33 PM, Jiang, Dave wrote:
> On Wed, 2016-06-15 at 15:26 -0600, Logan Gunthorpe wrote:
>> When the link goes down, the link_is_up flag did not return to
>> false. This could have caused some subtle corner case bugs
>> when the link goes up and down quickly.
>>
>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> 
> Acked-by: Dave Jiang <dave.jiang@intel.com>
> 
> And all the other ntb_perf patches since there were no additional
> changes. 
> 
>> ---
>>  drivers/ntb/test/ntb_perf.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/ntb/test/ntb_perf.c
>> b/drivers/ntb/test/ntb_perf.c
>> index f0784e5..ae9d1b2 100644
>> --- a/drivers/ntb/test/ntb_perf.c
>> +++ b/drivers/ntb/test/ntb_perf.c
>> @@ -557,6 +557,8 @@ static void perf_link_cleanup(struct work_struct
>> *work)
>>  
>>  	if (!perf->link_is_up)
>>  		cancel_delayed_work_sync(&perf->link_work);
>> +
>> +	perf->link_is_up = false;
>>  }
>>  
>>  static int perf_setup_mw(struct ntb_dev *ntb, struct perf_ctx *perf)
>> -- 
>> 2.1.4

Re: [PATCH v3 10/10] ntb_perf: clear link_is_up flag when the link goes down.

[Jiang, Dave]

On Wed, 2016-06-15 at 16:20 -0600, Logan Gunthorpe wrote:
> Hey,
> 
> Actually, I have to retract this patch. After some more thorough
> testing
> I'm finding an issue:
> 
> When you remove and re-install the ntb_perf module very quickly,
> ntb_perf will occasionally miss the link up event. This is because
> the
> link_cleanup work gets delayed long enough that it gets scheduled
> after
> the link up event gets sent. It then cancels the link work that
> should
> have occurred. Without this patch, it never happens because
> link_is_up
> never returns to false.
> 
> I think the correct solution is to just remove the link_cleanup work
> and
> do those actions immediately on receipt of the event. If there's
> agreement on this I can re-spin it again.

I'm ok with that. This is not an issue with ntb_transport?

> 
> Thanks,
> 
> Logan
> 
> 
> On 15/06/16 03:33 PM, Jiang, Dave wrote:
> > 
> > On Wed, 2016-06-15 at 15:26 -0600, Logan Gunthorpe wrote:
> > > 
> > > When the link goes down, the link_is_up flag did not return to
> > > false. This could have caused some subtle corner case bugs
> > > when the link goes up and down quickly.
> > > 
> > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> > Acked-by: Dave Jiang <dave.jiang@intel.com>
> > 
> > And all the other ntb_perf patches since there were no additional
> > changes. 
> > 
> > > 
> > > ---
> > >  drivers/ntb/test/ntb_perf.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/ntb/test/ntb_perf.c
> > > b/drivers/ntb/test/ntb_perf.c
> > > index f0784e5..ae9d1b2 100644
> > > --- a/drivers/ntb/test/ntb_perf.c
> > > +++ b/drivers/ntb/test/ntb_perf.c
> > > @@ -557,6 +557,8 @@ static void perf_link_cleanup(struct
> > > work_struct
> > > *work)
> > >  
> > >  	if (!perf->link_is_up)
> > >  		cancel_delayed_work_sync(&perf->link_work);
> > > +
> > > +	perf->link_is_up = false;
> > >  }
> > >  
> > >  static int perf_setup_mw(struct ntb_dev *ntb, struct perf_ctx
> > > *perf)
> > > -- 
> > > 2.1.4

Re: [PATCH v3 09/10] ntb_test: Add a selftest script for the NTB subsystem

[Logan Gunthorpe]

On 15/06/16 04:17 PM, Allen Hubbe wrote:
> This test should fail on Intel RP/TB topology (two cpu sharing one ntb).  The link state is the link state of the secondary side pcie bus connected to the secondary side cpu.  The link must be up in order for the secondary side cpu to discover the ntb device, so the driver does not allow the link to be disabled in such topology.

Ok, I wasn't aware of this.  But looking closer I think I have a better
solution:

ntb_link_disable should return -EINVAL if the hardware can't support
bringing the link down. If I add the error check to my tool_link_write
(which I neglected to do) then writing an "N" to $LOC/link will fail and
the test could be skipped. I'll make a v4 spin.

Logan

> A simple thing to do here might be:
> 
> write_file "N" "$LOC/link"
> sleep 1
> read_file "$REM/link"
> 
> You already have my Ack.  This minor issue can be fixed later if anyone cares.  I don't think it is a big deal, just worth pointing out that the script will hang here instead of report a failure.  If it is worth fixing later, at that point we might also want to change this script to continue with other tests instead of exit on the first failure.
> 

Re: [PATCH v3 10/10] ntb_perf: clear link_is_up flag when the link goes down.

[Logan Gunthorpe]

Hey,

On 15/06/16 04:24 PM, Jiang, Dave wrote:
> On Wed, 2016-06-15 at 16:20 -0600, Logan Gunthorpe wrote:
>> Hey,
>>
>> Actually, I have to retract this patch. After some more thorough
>> testing
>> I'm finding an issue:
>>
>> When you remove and re-install the ntb_perf module very quickly,
>> ntb_perf will occasionally miss the link up event. This is because
>> the
>> link_cleanup work gets delayed long enough that it gets scheduled
>> after
>> the link up event gets sent. It then cancels the link work that
>> should
>> have occurred. Without this patch, it never happens because
>> link_is_up
>> never returns to false.
>>
>> I think the correct solution is to just remove the link_cleanup work
>> and
>> do those actions immediately on receipt of the event. If there's
>> agreement on this I can re-spin it again.
> 
> I'm ok with that. This is not an issue with ntb_transport?

Looks like I can get something similar to happen in ntb_transport.
However, it's much rarer and takes significantly more tries to get it to
occur. It does appear to correctly set its link_is_up to false when the
link goes down.

I'm not sure I'm quite clear on the flow in ntb_transport and don't have
time right now to study it so I'll have to let that be someone else's
(fairly minor) issue.

Logan