Re: [PATCH ndctl] ndctl/cxl/test: Add CXL event test

[Ira Weiny <ira.weiny@intel.com>]

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