[PATCH v2 1/2] tests/intel/sysfs: Restore sysfs values on test failure
Cavitt, Jonathan
jonathan.cavitt at intel.com
Thu Oct 31 21:07:51 UTC 2024
-----Original Message-----
From: Kamil Konieczny <kamil.konieczny at linux.intel.com>
Sent: Thursday, October 31, 2024 11:16 AM
To: igt-dev at lists.freedesktop.org
Cc: Cavitt, Jonathan <jonathan.cavitt at intel.com>; Gupta, saurabhg <saurabhg.gupta at intel.com>; Zuo, Alex <alex.zuo at intel.com>; Belgaumkar, Vinay <vinay.belgaumkar at intel.com>
Subject: Re: [PATCH v2 1/2] tests/intel/sysfs: Restore sysfs values on test failure
>
> Hi Jonathan,
> On 2024-10-31 at 16:31:21 +0000, Jonathan Cavitt wrote:
> > The tests xe_sysfs_preempt_timeout and xe_sysfs_timeslice_duration
> > modify the values of preempt_timeout_us and timeslice_duration_us,
> > respectively. However, on a test failure, it is possible that these
> > values may remain in their modified states, resulting in the values
> > being used in future tests and causing unexpected behavior.
> >
> > Save the respective modified values before starting the test and attempt
> > to restore the values on test exit.
> >
> > Suggested-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> > CC: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
>
> Thank you for the patch, I have few nits, see below.
I'll attend to most nits for the next revision, though I have some
questions about the rest.
>
> > ---
> > tests/intel/xe_sysfs_preempt_timeout.c | 34 ++++++++++++++++-------
> > tests/intel/xe_sysfs_timeslice_duration.c | 33 ++++++++++++++++------
> > 2 files changed, 48 insertions(+), 19 deletions(-)
> >
> > diff --git a/tests/intel/xe_sysfs_preempt_timeout.c b/tests/intel/xe_sysfs_preempt_timeout.c
> > index 7fa0dfcdf7..1b9c26bfdc 100644
> > --- a/tests/intel/xe_sysfs_preempt_timeout.c
> > +++ b/tests/intel/xe_sysfs_preempt_timeout.c
> > @@ -183,8 +183,10 @@ igt_main
> > "preempt_timeout_min",
> > "preempt_timeout_max"}, };
> > int count = sizeof(property) / sizeof(property[0]);
> > + int gt_count = 0;
> > int fd = -1, sys_fd, gt;
> > - int engines_fd = -1, gt_fd = -1;
> > + int engines_fd[8], gt_fd[8];
> > + unsigned int pts[8];
>
> Please use define here, for example
> #define MAX_XE_ENGINE 8
>
> int engines_fd[MAX_XE_ENGINE], gt_fd[MAX_XE_ENGINE];
> unsigned int pts[MAX_XE_ENGINE];
>
> or allocate/realloc/dealloc it (it could be then a struct).
>
> >
> > igt_fixture {
> > fd = drm_open_driver(DRIVER_XE);
> > @@ -192,26 +194,38 @@ igt_main
> > sys_fd = igt_sysfs_open(fd);
> > igt_require(sys_fd != -1);
> > close(sys_fd);
> > +
> > + xe_for_each_gt(fd, gt) {
>
> Check
>
> igt_require(gt_count < MAX_XE_ENGINE);
>
> or reallocate.
>
> > + gt_fd[gt_count] = xe_sysfs_gt_open(fd, gt);
> > + igt_require(gt_fd[gt_count] != -1);
> > + engines_fd[gt_count] = openat(gt_fd[gt_count], "engines", O_RDONLY);
> > + igt_require(engines_fd[gt_count] != -1);
> > + igt_assert(igt_sysfs_scanf(engines_fd[gt_count],
>
> Why assert? imho igt_require should be enough.
Because we perform an assert on igt_sysfs_scanf during the test itself, so
I elected to use an assert here to be consistent with what the test does.
I'll change it, though.
>
> > + "preempt_timeout_us",
> > + "%u", &pts[gt_count]) == 1);
> > + gt_count++;
> > + }
> > +
> > }
> >
> > for (int i = 0; i < count; i++) {
> > for (typeof(*tests) *t = tests; t->name; t++) {
> > igt_subtest_with_dynamic_f("%s-%s", property[i][0], t->name) {
> > + int j = 0;
> > xe_for_each_gt(fd, gt) {
>
> Add
> igt_assert(gt_count < MAX_XE_ENGINE);
You mean:
igt_assert(j < MAX_XE_ENGINE)
?
The number of gts grabbed by xe_for_each_gt shouldn't be changing
mid-execution. So j should never exceed gt_count, which with the
addition of the prior assert, should itself never exceed
MAX_XE_ENGINE.
-Jonathan Cavitt
>
> > - gt_fd = xe_sysfs_gt_open(fd, gt);
> > - igt_require(gt_fd != -1);
> > - engines_fd = openat(gt_fd, "engines", O_RDONLY);
> > - igt_require(engines_fd != -1);
> > -
> > - igt_sysfs_engines(fd, engines_fd, gt, 1, property[i],
> > - t->fn);
> > - close(engines_fd);
> > - close(gt_fd);
> > + igt_sysfs_engines(fd, engines_fd[j], gt, 1, property[i],
> > + t->fn);
>
> I would prefere this in one line, or align to 'fd'.
>
> > + j++;
> > }
> > }
> > }
> > }
> > igt_fixture {
> > + for (int i = gt_count - 1; i >= 0; i--) {
> > + set_preempt_timeout(engines_fd[i], pts[i]);
> > + close(engines_fd[i]);
> > + close(gt_fd[i]);
> > + }
>
> Add newline.
>
> > drm_close_driver(fd);
> > }
> > }
> > diff --git a/tests/intel/xe_sysfs_timeslice_duration.c b/tests/intel/xe_sysfs_timeslice_duration.c
> > index cf95a3ac1c..914575a282 100644
> > --- a/tests/intel/xe_sysfs_timeslice_duration.c
> > +++ b/tests/intel/xe_sysfs_timeslice_duration.c
> > @@ -155,8 +155,10 @@ igt_main
> > "timeslice_duration_min",
> > "timeslice_duration_max"}, };
> > int count = sizeof(property) / sizeof(property[0]);
> > + int gt_count = 0;
> > int fd = -1, sys_fd, gt;
> > - int engines_fd = -1, gt_fd = -1;
> > + int engines_fd[8], gt_fd[8];
> > + unsigned int tds[8];
>
> Same here, use define instead of '8' or dynamic alloc.
>
> >
> > igt_fixture {
> > fd = drm_open_driver(DRIVER_XE);
> > @@ -164,25 +166,38 @@ igt_main
> > sys_fd = igt_sysfs_open(fd);
> > igt_require(sys_fd != -1);
> > close(sys_fd);
> > +
> > + xe_for_each_gt(fd, gt) {
>
> Same here, add guard for 'gt_count < MAX_XE_ENGINE'.
>
> > + gt_fd[gt_count] = xe_sysfs_gt_open(fd, gt);
> > + igt_require(gt_fd[gt_count] != -1);
> > + engines_fd[gt_count] = openat(gt_fd[gt_count],
> > + "engines", O_RDONLY);
>
> One line or align to 'gt_fd[...]'
> Btw use checkpatch.pl from linux kernel, here are too many spaces.
>
> > + igt_require(engines_fd[gt_count] != -1);
> > + igt_assert(igt_sysfs_scanf(engines_fd[gt_count],
>
> Same here, igt_require.
>
> > + "timeslice_duration_us",
> > + "%u", &tds[gt_count]) == 1);
> > + gt_count++;
> > + }
> > }
> >
> > for (int i = 0; i < count; i++) {
> > for (typeof(*tests) *t = tests; t->name; t++) {
> > igt_subtest_with_dynamic_f("%s-%s", property[i][0], t->name) {
> > + int j = 0;
> > xe_for_each_gt(fd, gt) {
>
> Same here, guard 'j < MAX_XE_ENGINE'
>
> > - gt_fd = xe_sysfs_gt_open(fd, gt);
> > - igt_require(gt_fd != -1);
> > - engines_fd = openat(gt_fd, "engines", O_RDONLY);
> > - igt_require(engines_fd != -1);
> > - igt_sysfs_engines(fd, engines_fd, gt, 1, property[i],
> > - t->fn);
> > - close(engines_fd);
> > - close(gt_fd);
> > + igt_sysfs_engines(fd, engines_fd[j], gt, 1, property[i],
> > + t->fn);
>
> Same here, one line or align to 'fd'.
>
> > + j++;
> > }
> > }
> > }
> > }
> > igt_fixture {
> > + for (int i = gt_count - 1; i >= 0; i--) {
> > + set_timeslice_duration(engines_fd[i], tds[i]);
> > + close(engines_fd[i]);
> > + close(gt_fd[i]);
> > + }
>
> Add newline.
>
> With above fixed
> Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
>
> > drm_close_driver(fd);
> > }
> > }
> > --
> > 2.43.0
> >
>
More information about the igt-dev
mailing list