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

2016-06-15 Thread 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 09/10] ntb_test: Add a selftest script for the NTB subsystem

2016-06-15 Thread 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 09/10] ntb_test: Add a selftest script for the NTB subsystem

2016-06-15 Thread 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 09/10] ntb_test: Add a selftest script for the NTB subsystem

2016-06-15 Thread 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 09/10] ntb_test: Add a selftest script for the NTB subsystem

2016-06-15 Thread 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

2016-06-15 Thread 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

2016-06-15 Thread 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 :03:00.1 :83:00.1 -p 29
> Starting ntb_tool tests...
> Running link tests on: :03:00.1 / :83:00.1
>   Passed
> Running link tests on: :83:00.1 / :03:00.1
>   Passed
> Running db tests on: :03:00.1 / :83:00.1
>   Passed
> Running db tests on: :83:00.1 / :03:00.1
>   Passed
> Running spad tests on: :03:00.1 / :83:00.1
>   Passed
> Running spad tests on: :83:00.1 / :03:00.1
>   Passed
> Running mw0 tests on: :03:00.1 / :83:00.1
>   Passed
> Running mw0 tests on: :83:00.1 / :03:00.1
>   Passed
> Running mw1 tests on: :03:00.1 / :83:00.1
>   Passed
> Running mw1 tests on: :83:00.1 / :03:00.1
>   Passed
> 
> Starting ntb_pingpong tests...
> Running ping pong tests on: :03:00.1 / :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 
> Acked-by: Shuah Khan 

Acked-by: Allen Hubbe 

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 
> diff --git a/tools/testing/selftests/ntb/ntb_test.sh
> b/tools/testing/selftests/ntb/ntb_test.sh
> new file mode 100755
> index 000..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 
> +
> +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} ;;
> +

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

2016-06-15 Thread 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 :03:00.1 :83:00.1 -p 29
> Starting ntb_tool tests...
> Running link tests on: :03:00.1 / :83:00.1
>   Passed
> Running link tests on: :83:00.1 / :03:00.1
>   Passed
> Running db tests on: :03:00.1 / :83:00.1
>   Passed
> Running db tests on: :83:00.1 / :03:00.1
>   Passed
> Running spad tests on: :03:00.1 / :83:00.1
>   Passed
> Running spad tests on: :83:00.1 / :03:00.1
>   Passed
> Running mw0 tests on: :03:00.1 / :83:00.1
>   Passed
> Running mw0 tests on: :83:00.1 / :03:00.1
>   Passed
> Running mw1 tests on: :03:00.1 / :83:00.1
>   Passed
> Running mw1 tests on: :83:00.1 / :03:00.1
>   Passed
> 
> Starting ntb_pingpong tests...
> Running ping pong tests on: :03:00.1 / :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 
> Acked-by: Shuah Khan 

Acked-by: Allen Hubbe 

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 
> diff --git a/tools/testing/selftests/ntb/ntb_test.sh
> b/tools/testing/selftests/ntb/ntb_test.sh
> new file mode 100755
> index 000..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 
> +
> +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)