Forwarding Sashiko review for doing discussions via mails.
# review url:
https://sashiko.dev/#/patchset/[email protected]
# start of sashiko.dev inline review
commit 67adf8e7b8416252faf583e176f0e8424fd4fb95
Author: SeongJae Park <[email protected]>
selftets/damon/sysfs.py: pause DAMON before dumping status
The sysfs.py test commits DAMON parameters, dumps the internal DAMON
state, and verifies the parameters. This commit pauses DAMON execution
during the state dumping to avoid races that can result in false test
results.
> selftets/damon/sysfs.py: pause DAMON before dumping status
This isn't a bug, but there's a typo (selftets) in the subject line.
> diff --git a/tools/testing/selftests/damon/sysfs.py
> b/tools/testing/selftests/damon/sysfs.py
> --- a/tools/testing/selftests/damon/sysfs.py
> +++ b/tools/testing/selftests/damon/sysfs.py
[ ... ]
> @@ -193,18 +193,55 @@ def assert_ctx_committed(ctx, dump):
> def assert_ctxs_committed(kdamonds):
> + ctxs_paused_for_dump = []
> + kdamonds_paused_for_dump = []
> + # pause for safe state dumping
> + for kd in kdamonds.kdamonds:
[ ... ]
> + # resume contexts paused for safe state dumping
> + for ctx in ctxs_paused_for_dump:
> + ctx.pause = False
> + if kd in kdamonds_paused_for_dump:
> + err = kd.commit()
Is this if statement intended to be a for loop?
Since kd is just retaining the value from the last iteration of the earlier
loop, using an if statement here means that if there are multiple kdamonds,
only the last one might have its state committed.
Could this leave other kdamonds un-resumed in the kernel?
> + if err is not None:
> + print('resume fail (%s)' % err)
> + kdamonds.stop()
> + exit(1)
[ ... ]
# end of sashiko.dev inline review
# review url:
https://sashiko.dev/#/patchset/[email protected]
#
# hkml [1] generated a draft of this mail. It can be regenerated
# using below command:
#
# hkml patch sashiko_dev --for_forwarding \
# [email protected]
#
# [1] https://github.com/sjp38/hackermail
Sent using hkml (https://github.com/sjp38/hackermail)
