Too many regions are shown with --numa_node 0
honggyukim opened this issue · 15 comments
Hi SeongJae,
In my personal machine, I see too many separate regions with --numa_node 0 as follows.
$ sudo ./damo fmt_json --numa_node 0
{
"kdamonds": [
{
"state": null,
"pid": null,
"contexts": [
{
"ops": "paddr",
"targets": [
{
"pid": null,
"regions": [
{
"start": "4,096",
"end": "651,264"
},
{
"start": "1,048,576",
"end": "2,259,902,488"
},
{
"start": "2,259,902,488",
"end": "2,259,959,896"
},
{
"start": "2,259,959,896",
"end": "2,259,963,928"
},
{
"start": "2,259,963,928",
"end": "2,260,093,528"
},
{
"start": "2,260,093,528",
"end": "2,293,805,056"
},
{
"start": "2,293,813,248",
"end": "2,318,991,360"
},
{
"start": "2,318,995,456",
"end": "2,324,541,440"
},
{
"start": "2,324,824,064",
"end": "2,382,479,360"
},
{
"start": "2,398,089,216",
"end": "2,398,093,312"
},
{
"start": "4,294,967,296",
"end": "19,025,362,944"
}
]
}
],
"intervals": {
"sample_us": "5 ms",
"aggr_us": "100 ms",
"ops_update_us": "1 s"
},
"nr_regions": {
"min": "10",
"max": "1,000"
},
"schemes": []
}
]
}
]
}My personal machine has a single numa node and it has only 16GB of DRAM so I feel it might be wasteful having too many regions in the configuration.
Rather than this, we might be better to keep only the two largest holes and show only 3 regions in this case just like damo already does in vaddr mode.
On the other hand, if you think that the regions are eventually split so it'd be better to keep the separate regions by physical layout rather than merging those regions, then I can close this issue without changes.
Thanks.
Hello Honggyu,
I think we'd better at least merge contiguous regions and the regions will be like this:
...
"regions": [
{
"start": "4,096",
"end": "651,264"
},
{
"start": "1,048,576",
"end": "2,293,805,056"
{
"start": "2,293,813,248",
"end": "2,318,991,360"
},
{
"start": "2,318,995,456",
"end": "2,324,541,440"
},
{
"start": "2,324,824,064",
"end": "2,382,479,360"
},
{
"start": "2,398,089,216",
"end": "2,398,093,312"
},
{
"start": "4,294,967,296",
"end": "19,025,362,944"
}
]
...
If regions are contiguous and within the same NUMA node, I don't find any region to keep them split.
I think we'd better at least merge contiguous regions
Yeah, that's what I was going to ask for changes.
Rather than this, we might be better to keep only the two largest holes and show only 3 regions in this case just like damo already does in vaddr mode.
Probably I misunderstood. I thought you wanted to change regions to be like in vaddr mode to have two large holes.
What I tried to say was that we could at least start reducing the number of regions from merging contiguous regions.
Agree that it would better to merge the contiguous regions. Nevertheless, once DAMON starts, those will be merged and split by their monitored access pattern. Hence this seems apparently better to do, but not that serious problem.
I will do this, but with only low priority for now, so might take some time. Please let me know if I'm not missing something and this needs a high priority.
Thanks for the confirmation. I also think that this is not a high priority issue, but just wanted to make it cleaner. Let's just keep this issue open and handle later.
once DAMON starts, those will be merged and split by their monitored access pattern
I'm just wondering only this part. If two regions are not actually continuous but just has a very small tiny hole, then can those regions be merged? If not, we might be better to merge them explicitly before DAMON starts running.
can those regions be merged?
Those cannot.
we might be better to merge them
Agree. But may better to have additional option for that, since that could be a behavioral change.
Below is just FYI, since this issue is for DAMO part change, while below is for DAMON change. Actually, I was thinking a similar solution as an answer to #64. In short, adding yet another DAMON operations set for dynamic physical address space monitoring.
In detail, we have two DAMON operations sets for virtual address space, namely vaddr and fvaddr. Unlike vaddr (stands for "virtual address spaces"), which check current mapping and the two big holes detection, fvaddr (stands for "fixed virtual address spaces") only monitor the regions that user specified. The operations set for physical address space, namely paddr, works like fvaddr. I'm thinking about adding another version of paddr that works like vaddr, say, dpaddr (dynamic physical address space). That would do such merging of tiny holes.
we might be better to merge them
Agree. But may better to have additional option for that, since that could be a behavioral change.
I also agree.
dpaddr (dynamic physical address space). That would do such merging of tiny holes.
I was thinking of adding an option for the size that can be merged. But the dpaddr sounds much better. Thanks!
I still see there are many contiguous regions in the generated .json config file.
Agree that it would better to merge the contiguous regions. Nevertheless, once DAMON starts, those will be merged and split by their monitored access pattern.
I'm not sure internally but it apparently shows multiple regions are split in its sysfs at /sys/kernel/mm/damon/admin/kdamonds/0/contexts/0/targets/0/regions/. I wish they could be merged at least they are contiguous without having a hole.
I'm not sure internally but it apparently shows multiple regions are split in its sysfs at
So, DAMON will not merge two adjacent regions for two reasons.
- Because the regions are having different access pattern.
- Because merging those can make the total number of regions smaller than user-defined minimum number of regions.
My previous comment was not explaining the second case. Could you please check if your finding could be explained with above two reasons?
Agree that it would better to merge the contiguous regions. Nevertheless, once DAMON starts, those will be merged and split by their monitored access pattern.
It's a late answer but I agree that the split regions by the input config file will be merged after damo start if the access patterns are similar. But what I want to say here is about the verbosity of input config files, which looks unnecessary.
I just asked to merge the contiguous regions in the config file for simplicity in terms of users' perspective.
Ok, so you just want damo fmt_json output to be changed. Sure, that makes sense. I'll try to make the change.
Thanks. That will make our config files much shorter and compact!
Just pushed a commit for this issue. I tested the feature using a unit test that I pushed together: d8beb81 ("tests/unit/test_damon_args: Add a test for merge_cont_ranges()")
However, my test machine has no such multiple memory blocks, so cannot confirm if it really fix your problem. Please test and let me know if it doesn't work for you.
Thanks. I've just confirmed that it makes the config file much more compact. It reduces the node0's region count from 21 to 5.
Thank you for confirming!