| Message ID | 20230726-cxl-event-v1-1-1cf8cb02b211@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ndctl] ndctl/cxl/test: Add CXL event test | expand |
On 7/27/23 14:21, Ira Weiny wrote: > Previously CXL event testing was run by hand. This reduces testing > coverage including a lack of regression testing. > > Add a CXL test as part of the meson test infrastructure. Passing is > predicated on receiving the appropriate number of errors in each log. > Individual event values are not checked. > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> LGTM. Have you tried running shellcheck? Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > test/cxl-events.sh | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > test/meson.build | 2 ++ > 2 files changed, 70 insertions(+) > > diff --git a/test/cxl-events.sh b/test/cxl-events.sh > new file mode 100644 > index 000000000000..f51046ec39ad > --- /dev/null > +++ b/test/cxl-events.sh > @@ -0,0 +1,68 @@ > +#!/bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (C) 2023 Intel Corporation. All rights reserved. > + > +. $(dirname $0)/common > + > +set -ex > + > +trap 'err $LINENO' ERR > + > +check_prereq "jq" > + > +modprobe -r cxl_test > +modprobe cxl_test > + > +dev_path="/sys/bus/platform/devices" > + > +test_cxl_events() > +{ > + memdev="$1" > + > + echo "TEST: triggering $memdev" > + echo 1 > $dev_path/$memdev/event_trigger > +} > + > +readarray -t memdevs < <("$CXL" list -b cxl_test -Mi | jq -r '.[].host') > + > +echo "TEST: Prep event trace" > +echo "" > /sys/kernel/tracing/trace > +echo 1 > /sys/kernel/tracing/events/cxl/enable > +echo 1 > /sys/kernel/tracing/tracing_on > + > +# Only need to test 1 device > +#for memdev in ${memdevs[@]}; do > +#done > + > +test_cxl_events "$memdevs" > + > +echo 0 > /sys/kernel/tracing/tracing_on > + > +echo "TEST: Events seen" > +cat /sys/kernel/tracing/trace > +num_overflow=$(grep "cxl_overflow" /sys/kernel/tracing/trace | wc -l) > +num_fatal=$(grep "log=Fatal" /sys/kernel/tracing/trace | wc -l) > +num_failure=$(grep "log=Failure" /sys/kernel/tracing/trace | wc -l) > +num_info=$(grep "log=Informational" /sys/kernel/tracing/trace | wc -l) > +echo " LOG (Expected) : (Found)" > +echo " overflow ( 1) : $num_overflow" > +echo " Fatal ( 2) : $num_fatal" > +echo " Failure (16) : $num_failure" > +echo " Informational ( 3) : $num_info" > + > +if [ "$num_overflow" -ne 1 ]; then > + err "$LINENO" > +fi > +if [ "$num_fatal" -ne 2 ]; then > + err "$LINENO" > +fi > +if [ "$num_failure" -ne 16 ]; then > + err "$LINENO" > +fi > +if [ "$num_info" -ne 3 ]; then > + err "$LINENO" > +fi > + > +check_dmesg "$LINENO" > + > +modprobe -r cxl_test > diff --git a/test/meson.build b/test/meson.build > index a956885f6df6..a33255bde1a8 100644 > --- a/test/meson.build > +++ b/test/meson.build > @@ -155,6 +155,7 @@ cxl_sysfs = find_program('cxl-region-sysfs.sh') > cxl_labels = find_program('cxl-labels.sh') > cxl_create_region = find_program('cxl-create-region.sh') > cxl_xor_region = find_program('cxl-xor-region.sh') > +cxl_events = find_program('cxl-events.sh') > > tests = [ > [ 'libndctl', libndctl, 'ndctl' ], > @@ -183,6 +184,7 @@ tests = [ > [ 'cxl-labels.sh', cxl_labels, 'cxl' ], > [ 'cxl-create-region.sh', cxl_create_region, 'cxl' ], > [ 'cxl-xor-region.sh', cxl_xor_region, 'cxl' ], > + [ 'cxl-events.sh', cxl_events, 'cxl' ], > ] > > if get_option('destructive').enabled() > > --- > base-commit: 2fd570a0ed788b1bd0971dfdb1466a5dbcb79775 > change-id: 20230726-cxl-event-dc00a2f94b60 > > Best regards,
Dave Jiang wrote: > > > On 7/27/23 14:21, Ira Weiny wrote: > > Previously CXL event testing was run by hand. This reduces testing > > coverage including a lack of regression testing. > > > > Add a CXL test as part of the meson test infrastructure. Passing is > > predicated on receiving the appropriate number of errors in each log. > > Individual event values are not checked. > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > LGTM. Have you tried running shellcheck? Never heard of it... searching... This? ShellCheck.x86_64 : Shell script analysis tool > > Reviewed-by: Dave Jiang <dave.jiang@intel.com> Thanks, Ira
On 7/27/23 17:15, Ira Weiny wrote: > Dave Jiang wrote: >> >> >> On 7/27/23 14:21, Ira Weiny wrote: >>> Previously CXL event testing was run by hand. This reduces testing >>> coverage including a lack of regression testing. >>> >>> Add a CXL test as part of the meson test infrastructure. Passing is >>> predicated on receiving the appropriate number of errors in each log. >>> Individual event values are not checked. >>> >>> Signed-off-by: Ira Weiny <ira.weiny@intel.com> >> >> LGTM. Have you tried running shellcheck? > > Never heard of it... searching... > > This? > > ShellCheck.x86_64 : Shell script analysis tool I think so. > >> >> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > > Thanks, > Ira
On Thu, 2023-07-27 at 14:21 -0700, Ira Weiny wrote: > Previously CXL event testing was run by hand. This reduces testing > coverage including a lack of regression testing. > > Add a CXL test as part of the meson test infrastructure. Passing is > predicated on receiving the appropriate number of errors in each log. > Individual event values are not checked. > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > --- > test/cxl-events.sh | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > test/meson.build | 2 ++ > 2 files changed, 70 insertions(+) Hi Ira, Thanks for adding this test. Just a few minor comments below, otherwise looks good. > > diff --git a/test/cxl-events.sh b/test/cxl-events.sh > new file mode 100644 > index 000000000000..f51046ec39ad > --- /dev/null > +++ b/test/cxl-events.sh > @@ -0,0 +1,68 @@ > +#!/bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (C) 2023 Intel Corporation. All rights reserved. > + > +. $(dirname $0)/common > + > +set -ex > + > +trap 'err $LINENO' ERR > + > +check_prereq "jq" > + > +modprobe -r cxl_test > +modprobe cxl_test > + > +dev_path="/sys/bus/platform/devices" > + > +test_cxl_events() > +{ > + memdev="$1" > + > + echo "TEST: triggering $memdev" > + echo 1 > $dev_path/$memdev/event_trigger Quote the "$variables" here. We don't expect spaces in the path in this case, so it will still work, but it is good practice to always quote everything. We might also need a test to see if this file exists first. For kernels that don't have this support, we probably want to print a message and skip the test (return '77'). > +} > + > +readarray -t memdevs < <("$CXL" list -b cxl_test -Mi | jq -r '.[].host') > + > +echo "TEST: Prep event trace" > +echo "" > /sys/kernel/tracing/trace > +echo 1 > /sys/kernel/tracing/events/cxl/enable > +echo 1 > /sys/kernel/tracing/tracing_on > + > +# Only need to test 1 device > +#for memdev in ${memdevs[@]}; do > +#done Probably just remove the commented out loop, if we need to test more than one memdev in the future, it is easy enough to add back then. > + > +test_cxl_events "$memdevs" Shouldn't use "$memdevs" here since it is an array. If you want to pass in just the first memdev, use "${memdevs[0]}" > + > +echo 0 > /sys/kernel/tracing/tracing_on > + > +echo "TEST: Events seen" > +cat /sys/kernel/tracing/trace > +num_overflow=$(grep "cxl_overflow" /sys/kernel/tracing/trace | wc -l) > +num_fatal=$(grep "log=Fatal" /sys/kernel/tracing/trace | wc -l) > +num_failure=$(grep "log=Failure" /sys/kernel/tracing/trace | wc -l) > +num_info=$(grep "log=Informational" /sys/kernel/tracing/trace | wc -l) Minor nit, but you can 'grep -c' instead of 'grep | wc -l' Also could put /sys/kernel/tracing/trace into a variable just for readability since it is used many times. > +echo " LOG (Expected) : (Found)" > +echo " overflow ( 1) : $num_overflow" > +echo " Fatal ( 2) : $num_fatal" > +echo " Failure (16) : $num_failure" > +echo " Informational ( 3) : $num_info" > + > +if [ "$num_overflow" -ne 1 ]; then > + err "$LINENO" > +fi > +if [ "$num_fatal" -ne 2 ]; then > + err "$LINENO" > +fi > +if [ "$num_failure" -ne 16 ]; then > + err "$LINENO" > +fi > +if [ "$num_info" -ne 3 ]; then > + err "$LINENO" > +fi Perhaps define variables for each of the expected nums, that way there is only one spot to change in case the numbers change in the future. > + > +check_dmesg "$LINENO" > + > +modprobe -r cxl_test > diff --git a/test/meson.build b/test/meson.build > index a956885f6df6..a33255bde1a8 100644 > --- a/test/meson.build > +++ b/test/meson.build > @@ -155,6 +155,7 @@ cxl_sysfs = find_program('cxl-region-sysfs.sh') > cxl_labels = find_program('cxl-labels.sh') > cxl_create_region = find_program('cxl-create-region.sh') > cxl_xor_region = find_program('cxl-xor-region.sh') > +cxl_events = find_program('cxl-events.sh') > > tests = [ > [ 'libndctl', libndctl, 'ndctl' ], > @@ -183,6 +184,7 @@ tests = [ > [ 'cxl-labels.sh', cxl_labels, 'cxl' ], > [ 'cxl-create-region.sh', cxl_create_region, 'cxl' ], > [ 'cxl-xor-region.sh', cxl_xor_region, 'cxl' ], > + [ 'cxl-events.sh', cxl_events, 'cxl' ], > ] > > if get_option('destructive').enabled() > > --- > base-commit: 2fd570a0ed788b1bd0971dfdb1466a5dbcb79775 > change-id: 20230726-cxl-event-dc00a2f94b60 > > Best regards,
Verma, Vishal L wrote: > On Thu, 2023-07-27 at 14:21 -0700, Ira Weiny wrote: > > Previously CXL event testing was run by hand. This reduces testing > > coverage including a lack of regression testing. > > > > Add a CXL test as part of the meson test infrastructure. Passing is > > predicated on receiving the appropriate number of errors in each log. > > Individual event values are not checked. > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > > test/cxl-events.sh | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > test/meson.build | 2 ++ > > 2 files changed, 70 insertions(+) > > Hi Ira, > > Thanks for adding this test. Just a few minor comments below, otherwise > looks good. Thanks! > > > > > diff --git a/test/cxl-events.sh b/test/cxl-events.sh > > new file mode 100644 > > index 000000000000..f51046ec39ad > > --- /dev/null > > +++ b/test/cxl-events.sh > > @@ -0,0 +1,68 @@ > > +#!/bin/bash > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright (C) 2023 Intel Corporation. All rights reserved. > > + > > +. $(dirname $0)/common > > + > > +set -ex > > + > > +trap 'err $LINENO' ERR > > + > > +check_prereq "jq" > > + > > +modprobe -r cxl_test > > +modprobe cxl_test > > + > > +dev_path="/sys/bus/platform/devices" > > + > > +test_cxl_events() > > +{ > > + memdev="$1" > > + > > + echo "TEST: triggering $memdev" > > + echo 1 > $dev_path/$memdev/event_trigger > > Quote the "$variables" here. We don't expect spaces in the path in this > case, so it will still work, but it is good practice to always quote > everything. Done. > > We might also need a test to see if this file exists first. For kernels > that don't have this support, we probably want to print a message and > skip the test (return '77'). Good idea. > > > +} > > + > > +readarray -t memdevs < <("$CXL" list -b cxl_test -Mi | jq -r '.[].host') > > + > > +echo "TEST: Prep event trace" > > +echo "" > /sys/kernel/tracing/trace > > +echo 1 > /sys/kernel/tracing/events/cxl/enable > > +echo 1 > /sys/kernel/tracing/tracing_on > > + > > +# Only need to test 1 device > > +#for memdev in ${memdevs[@]}; do > > +#done > > Probably just remove the commented out loop, if we need to test more > than one memdev in the future, it is easy enough to add back then. Done. > > > + > > +test_cxl_events "$memdevs" > > Shouldn't use "$memdevs" here since it is an array. If you want to pass > in just the first memdev, use "${memdevs[0]}" Ah yea caught my hack ;-) done. > > > + > > +echo 0 > /sys/kernel/tracing/tracing_on > > + > > +echo "TEST: Events seen" > > +cat /sys/kernel/tracing/trace > > +num_overflow=$(grep "cxl_overflow" /sys/kernel/tracing/trace | wc -l) > > +num_fatal=$(grep "log=Fatal" /sys/kernel/tracing/trace | wc -l) > > +num_failure=$(grep "log=Failure" /sys/kernel/tracing/trace | wc -l) > > +num_info=$(grep "log=Informational" /sys/kernel/tracing/trace | wc -l) > > Minor nit, but you can 'grep -c' instead of 'grep | wc -l' Ok Done. > > Also could put /sys/kernel/tracing/trace into a variable just for > readability since it is used many times. Done. > > > +echo " LOG (Expected) : (Found)" > > +echo " overflow ( 1) : $num_overflow" > > +echo " Fatal ( 2) : $num_fatal" > > +echo " Failure (16) : $num_failure" > > +echo " Informational ( 3) : $num_info" > > + > > +if [ "$num_overflow" -ne 1 ]; then > > + err "$LINENO" > > +fi > > +if [ "$num_fatal" -ne 2 ]; then > > + err "$LINENO" > > +fi > > +if [ "$num_failure" -ne 16 ]; then > > + err "$LINENO" > > +fi > > +if [ "$num_info" -ne 3 ]; then > > + err "$LINENO" > > +fi > > Perhaps define variables for each of the expected nums, that way there > is only one spot to change in case the numbers change in the future. Good idea, done. V2 on it's way soon, thanks for looking, Ira > > > + > > +check_dmesg "$LINENO" > > + > > +modprobe -r cxl_test > > diff --git a/test/meson.build b/test/meson.build > > index a956885f6df6..a33255bde1a8 100644 > > --- a/test/meson.build > > +++ b/test/meson.build > > @@ -155,6 +155,7 @@ cxl_sysfs = find_program('cxl-region-sysfs.sh') > > cxl_labels = find_program('cxl-labels.sh') > > cxl_create_region = find_program('cxl-create-region.sh') > > cxl_xor_region = find_program('cxl-xor-region.sh') > > +cxl_events = find_program('cxl-events.sh') > > > > tests = [ > > [ 'libndctl', libndctl, 'ndctl' ], > > @@ -183,6 +184,7 @@ tests = [ > > [ 'cxl-labels.sh', cxl_labels, 'cxl' ], > > [ 'cxl-create-region.sh', cxl_create_region, 'cxl' ], > > [ 'cxl-xor-region.sh', cxl_xor_region, 'cxl' ], > > + [ 'cxl-events.sh', cxl_events, 'cxl' ], > > ] > > > > if get_option('destructive').enabled() > > > > --- > > base-commit: 2fd570a0ed788b1bd0971dfdb1466a5dbcb79775 > > change-id: 20230726-cxl-event-dc00a2f94b60 > > > > Best regards, >
diff --git a/test/cxl-events.sh b/test/cxl-events.sh new file mode 100644 index 000000000000..f51046ec39ad --- /dev/null +++ b/test/cxl-events.sh @@ -0,0 +1,68 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (C) 2023 Intel Corporation. All rights reserved. + +. $(dirname $0)/common + +set -ex + +trap 'err $LINENO' ERR + +check_prereq "jq" + +modprobe -r cxl_test +modprobe cxl_test + +dev_path="/sys/bus/platform/devices" + +test_cxl_events() +{ + memdev="$1" + + echo "TEST: triggering $memdev" + echo 1 > $dev_path/$memdev/event_trigger +} + +readarray -t memdevs < <("$CXL" list -b cxl_test -Mi | jq -r '.[].host') + +echo "TEST: Prep event trace" +echo "" > /sys/kernel/tracing/trace +echo 1 > /sys/kernel/tracing/events/cxl/enable +echo 1 > /sys/kernel/tracing/tracing_on + +# Only need to test 1 device +#for memdev in ${memdevs[@]}; do +#done + +test_cxl_events "$memdevs" + +echo 0 > /sys/kernel/tracing/tracing_on + +echo "TEST: Events seen" +cat /sys/kernel/tracing/trace +num_overflow=$(grep "cxl_overflow" /sys/kernel/tracing/trace | wc -l) +num_fatal=$(grep "log=Fatal" /sys/kernel/tracing/trace | wc -l) +num_failure=$(grep "log=Failure" /sys/kernel/tracing/trace | wc -l) +num_info=$(grep "log=Informational" /sys/kernel/tracing/trace | wc -l) +echo " LOG (Expected) : (Found)" +echo " overflow ( 1) : $num_overflow" +echo " Fatal ( 2) : $num_fatal" +echo " Failure (16) : $num_failure" +echo " Informational ( 3) : $num_info" + +if [ "$num_overflow" -ne 1 ]; then + err "$LINENO" +fi +if [ "$num_fatal" -ne 2 ]; then + err "$LINENO" +fi +if [ "$num_failure" -ne 16 ]; then + err "$LINENO" +fi +if [ "$num_info" -ne 3 ]; then + err "$LINENO" +fi + +check_dmesg "$LINENO" + +modprobe -r cxl_test diff --git a/test/meson.build b/test/meson.build index a956885f6df6..a33255bde1a8 100644 --- a/test/meson.build +++ b/test/meson.build @@ -155,6 +155,7 @@ cxl_sysfs = find_program('cxl-region-sysfs.sh') cxl_labels = find_program('cxl-labels.sh') cxl_create_region = find_program('cxl-create-region.sh') cxl_xor_region = find_program('cxl-xor-region.sh') +cxl_events = find_program('cxl-events.sh') tests = [ [ 'libndctl', libndctl, 'ndctl' ], @@ -183,6 +184,7 @@ tests = [ [ 'cxl-labels.sh', cxl_labels, 'cxl' ], [ 'cxl-create-region.sh', cxl_create_region, 'cxl' ], [ 'cxl-xor-region.sh', cxl_xor_region, 'cxl' ], + [ 'cxl-events.sh', cxl_events, 'cxl' ], ] if get_option('destructive').enabled()
Previously CXL event testing was run by hand. This reduces testing coverage including a lack of regression testing. Add a CXL test as part of the meson test infrastructure. Passing is predicated on receiving the appropriate number of errors in each log. Individual event values are not checked. Signed-off-by: Ira Weiny <ira.weiny@intel.com> --- test/cxl-events.sh | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ test/meson.build | 2 ++ 2 files changed, 70 insertions(+) --- base-commit: 2fd570a0ed788b1bd0971dfdb1466a5dbcb79775 change-id: 20230726-cxl-event-dc00a2f94b60 Best regards,