accel-config/test: Add option to turn off verbose output from test scripts

[snits]

This adds a --quiet option to the dsa_config_test_runner.sh, dsa_user_test_runner.sh, and iaa_user_test_runner.sh scripts. It still defaults to 'set -x', and passing '-v' to the test command, but will allow someone to run the test with --quiet to see the results without all of the debugging messages.

Also included is a small typo cleanup in dsa_config_test_runner.sh

Signed-off-by: Jerry Snitselaar jsnitsel@redhat.com

[aegl]

Noisy default made sense during early development. Maybe it is time to just make the default be quiet. Have some option to enable all the verbose debug.

[snits]

I'm happy with whichever Intel would like. @ramesh-thomas would that be okay? I can flip it to be --verbose, and have it default to not being so chatty. I wasn't sure if that would be desired to I left it to default to the current state.

I'm in the process of automating some testing for idxd here for cki to hopefully more quickly catch some issues in the future.

[ramesh-thomas]

It makes sense to make verbose optionally enabled with a parameter. I have a couple of questions from the patch.

1. I did not see a "set -x" anywhere so is "set +x" necessary if that is the default?
2. Is the patch also adding a "--skip-config" option? Is it related to the verbose/quiet implementation?

[snits]

It makes sense to make verbose optionally enabled with a parameter. I have a couple of questions from the patch.

1. I did not see a "set -x" anywhere so is "set +x" necessary if that is the default?

It should be at the very beginning of dsa_conf_test_runner.sh, dsa_user_test_runner.sh, and iaa_user_test_runner.sh:

#!/bin/bash -Ex

2. Is the patch also adding a "--skip-config" option? Is it related to the verbose/quiet implementation?

It keeps the --skip-config option that already existed in dsa_user_test_runner.sh, and iaa_user_test_runner.sh.

I will push the revised version for you to look at.

[snits]

I pushed the --verbose version. The difference in the amount of output with --verbose versus without:

# ./dsa_config_test_runner.sh --verbose 2>&1 | wc -l
4673
# ./dsa_config_test_runner.sh 2>&1 | wc -l
147
# ./dsa_user_test_runner.sh --verbose 2>&1 | wc -l
78301
# ./dsa_user_test_runner.sh 2>&1 | wc -l
10287
# ./iaa_user_test_runner.sh --verbose 2>&1 | wc -l
10727
# ./iaa_user_test_runner.sh 2>&1 | wc -l
1333

[ramesh-thomas]

@snits it looks good. Thanks for fixing this.

@ysun please note the change. (Yi Sun is the new accel-config test owner)

[ysun]

Thanks Jerry! The patch looks good to me!

[snits]

@ysun Is this test correct in dsa_config_test_runner.sh?

"$DSATEST" -w 0 -l 4096 -f 0x1 -o 0x9 "${VERBOSE}" || echo "should fail, but pass" || exit 1

The other fail, but pass tests use && instead of ||

or should the text be changed to should pass, but fail ?

I think the text needs to be switched it should pass because opcode is 0x9, and

printf "%x\n" "$((0x272 & (1 << 9)))"
200

[ysun]

@ysun Is this test correct in dsa_config_test_runner.sh?

"$DSATEST" -w 0 -l 4096 -f 0x1 -o 0x9 "${VERBOSE}" || echo "should fail, but pass" || exit 1

The other fail, but pass tests use && instead of ||

or should the text be changed to should pass, but fail ?

I think the text needs to be switched it should pass because opcode is 0x9, and

printf "%x\n" "$((0x272 & (1 << 9)))"
200

You correct! By going through the test, I believe the log is wrong, it should be "should pass, but fail". The result from the previous step is "00000272" which translates to (0010 0111 0010). So, bits 0, 2, 3, 7, 8 should fail, while bit 1, 4, 5, 6, 9 should pass.

And I recommend that we can cook a new patch for these fixes. Besides this, I've noticed numerous typos (like "shoudl") in the test runners. 🤕

[ysun]

@ramesh-thomas Could you please help to merge this PR as well as the other one if you don't have any other concerns? Also, could you clarify the rule for syncing commits between the pending and stable branches for this repository?

[snits]

That reminds me, should I be posting these against stable? That is what github shows as the default target when opening a merge request, but my recollection was things went into pending, and then were merged into stable.

[ramesh-thomas]

@ramesh-thomas Could you please help to merge this PR as well as the other one if you don't have any other concerns? Also, could you clarify the rule for syncing commits between the pending and stable branches for this repository?

I have merged them into pending. We can decide about the /sys/bus/iax related patch if we decide to remove that path. I will move them to stable and make a release next week along with some other fixes I plan to add. If urgent, let me know and I can make the release immediately.

The patches initially go into the pending branch. After validation is completed, we move them to stable and make a new release.

[ramesh-thomas]

That reminds me, should I be posting these against stable? That is what github shows as the default target when opening a merge request, but my recollection was things went into pending, and then were merged into stable.

Please post them to pending. You are right, we move them to stable before a release after running validation cycles.

[snits]

@ramesh-thomas it isn't urgent to get these changes on my end. I have a patch applying the fix for attempting to iterate over a devices when none exist for the device type to the package for our current development cycle, to tide us over for now. I will still have time to rebase to it if needed.

I will try to have an answer about iax by Monday. Actually I will grab a system right now, and take a look.

[snits]

I must have looked at the wrong branch the other night. The bus removal goes back to 8.5, and 8.6 is the first release to support Sapphire Rapids. I verified on a system that the iax devices are under /sys/bus/dsa/devices.

[snits]

closing since the changes was merged.

@ramesh-thomas as mentioned in comment last week, our first release that supports SPR is 8.6, and that kernel has the change that removed the iax bus type. So it isn't an issue for us if it is removed here.

[ramesh-thomas]

@snits thanks for confirming.

@ysun @davejiang @fyu1 kernel version 5.13 that had /sys/bus/iax path is not being used by any distribution. Any opinion on whether we should remove it from accel-config and the tests or is it ok to just leave the code there?

[davejiang]

@ramesh-thomas given that there are no distros using the code, may as well clean it up and drop the support.

[ramesh-thomas]

ok, thanks.

@ysun you can remove from the tests and I can clean it up in accel-config.

[ysun]

ok, thanks.

@ysun you can remove from the tests and I can clean it up in accel-config.

Sure. I'll create PR soon.