awslabs / damo

DAMON user-space tool
https://damonitor.github.io/
GNU General Public License v2.0
155 stars 26 forks source link

damo_tune: Evaluate args before committing kdamonds #94

Closed velurimithun closed 5 months ago

velurimithun commented 5 months ago

Description of changes:

Env:

[root@ip-172-31-36-123 ~]# uname -a
Linux ip-172-31-36-123.ap-south-1.compute.internal 6.8.4 #2 SMP PREEMPT_DYNAMIC Sun Apr  7 13:12:02 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
[root@ip-172-31-36-123 ~]# 

Before patch:

[root@ip-172-31-36-123 ~]# damo start "sleep 300"
[root@ip-172-31-36-123 ~]# damo tune --damos_quotas 30 --damos_quota_goal user_input 60 30 --ops vaddr --debug_damon
read '/sys/kernel/mm/damon/admin/kdamonds/nr_kdamonds': '1'
read '/sys/kernel/mm/damon/admin/kdamonds/0/state': 'on'
read '/sys/kernel/mm/damon/admin/kdamonds/nr_kdamonds': '1'
read '/sys/kernel/mm/damon/admin/kdamonds/0/contexts/nr_contexts': '1'
write 'vaddr' to '/sys/kernel/mm/damon/admin/kdamonds/0/contexts/0/operations'
write '5000' to '/sys/kernel/mm/damon/admin/kdamonds/0/contexts/0/monitoring_attrs/intervals/sample_us'
write '100000' to '/sys/kernel/mm/damon/admin/kdamonds/0/contexts/0/monitoring_attrs/intervals/aggr_us'
write '1000000' to '/sys/kernel/mm/damon/admin/kdamonds/0/contexts/0/monitoring_attrs/intervals/update_us'
write '10' to '/sys/kernel/mm/damon/admin/kdamonds/0/contexts/0/monitoring_attrs/nr_regions/min'
write '1000' to '/sys/kernel/mm/damon/admin/kdamonds/0/contexts/0/monitoring_attrs/nr_regions/max'
read '/sys/kernel/mm/damon/admin/kdamonds/0/contexts/0/targets/nr_targets': '1'
read '/sys/kernel/mm/damon/admin/kdamonds/0/contexts/0/targets/0/regions/nr_regions': '0'
read '/sys/kernel/mm/damon/admin/kdamonds/0/contexts/0/schemes/nr_schemes': '0'
write 'commit' to '/sys/kernel/mm/damon/admin/kdamonds/0/state'
[root@ip-172-31-36-123 ~]#

After patch:

[root@ip-172-31-36-123 ~]# damo start "sleep 300"
[root@ip-172-31-36-123 ~]# damo tune --damos_quotas 30 --damos_quota_goal user_input 60 30 --ops vaddr --debug_damon
read '/sys/kernel/mm/damon/admin/kdamonds/nr_kdamonds': '1'
read '/sys/kernel/mm/damon/admin/kdamonds/0/state': 'on'
Incorrect arguments: no 'damos_action' provided in arguments
[root@ip-172-31-36-123 ~]#

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

sj-aws commented 5 months ago

Thank you for this nice PR!

I agree that passing any --damos_* without --damos_action makes no sense. However, this change will force --damos_action even if no --damos_* option is given. That is, say, damo tune --monitoring_intervals 5ms 200ms 2s should be a legal command, but this change will give a unnecessary error to the user. Could we handle such cases?

velurimithun commented 5 months ago

Thanks for your review @sj-aws . I've now updated the commit to check for damo_action only when any damo_* option is present in input args.

Moving further, in order to avoid these situations (where other functionalities can break) I'll run all the tests before raising PR.

[root@ip-172-31-36-123 damo]# ./tests/run.sh 
PASS unit test_damo_records.py
PASS unit test_damo_scheme_dbgfs_conversion.py
PASS unit test_damo_schemes_input.py
PASS unit test_damo_show.py
PASS unit test_damon.py
[...]
PASS start_stop sysfs stop
PASS start_stop
PASS ALL
[root@ip-172-31-36-123 damo]# 
sj-aws commented 5 months ago

Looks good to me, thank you for this PR!